Alternative FixItHints

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Alternative FixItHints

Neil Nelson via cfe-dev
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Wed, May 25, 2016 at 5:24 PM Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

Will be interesting to expose that in libclang. Given that it seems hard for an editor to decide how to insert #includes correctly (unless we somehow also expose clang-format).

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Given the complexity of the current typo correction code (yikes!), introducing this for an alternative warning might be a good idea.
 
Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?

An open question is: for more interesting replacements, like includes, where do we want the decision on how to format / where exactly to insert the #includes to be made; if we don't start with supporting #include insertion and instead focus on alternatives first, that might not actually be a problem, though

 
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
Sound like a killer feature for YouCompleteMe!

2016-06-01 16:43 GMT+02:00 Manuel Klimek via cfe-dev <[hidden email]>:
On Wed, May 25, 2016 at 5:24 PM Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

Will be interesting to expose that in libclang. Given that it seems hard for an editor to decide how to insert #includes correctly (unless we somehow also expose clang-format).

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Given the complexity of the current typo correction code (yikes!), introducing this for an alternative warning might be a good idea.
 
Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?

An open question is: for more interesting replacements, like includes, where do we want the decision on how to format / where exactly to insert the #includes to be made; if we don't start with supporting #include insertion and instead focus on alternatives first, that might not actually be a problem, though

 
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Wed, Jun 1, 2016 at 5:14 PM Piotr Padlewski <[hidden email]> wrote:
Sound like a killer feature for YouCompleteMe!

+ben jackson, who said claimed that ycm is well prepared for this in principle.
 

2016-06-01 16:43 GMT+02:00 Manuel Klimek via cfe-dev <[hidden email]>:
On Wed, May 25, 2016 at 5:24 PM Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

Will be interesting to expose that in libclang. Given that it seems hard for an editor to decide how to insert #includes correctly (unless we somehow also expose clang-format).

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Given the complexity of the current typo correction code (yikes!), introducing this for an alternative warning might be a good idea.
 
Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?

An open question is: for more interesting replacements, like includes, where do we want the decision on how to format / where exactly to insert the #includes to be made; if we don't start with supporting #include insertion and instead focus on alternatives first, that might not actually be a problem, though

 
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev

2016-06-01 17:19 GMT+02:00 Manuel Klimek <[hidden email]>:
On Wed, Jun 1, 2016 at 5:14 PM Piotr Padlewski <[hidden email]> wrote:
Sound like a killer feature for YouCompleteMe!

+ben jackson, who said claimed that ycm is well prepared for this in principle.
 

I still prefer regular IDE when developing something big, but this feature would convince me to open vim more frequently :)

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
In reply to this post by Neil Nelson via cfe-dev
Hopefully it isn't just bluster. We're very keen for refactoring etc. tools and more developer   utilities in YCM so I've sort of promised to implement it if it goes into libclang :)

On 1 Jun 2016, at 11:19, Manuel Klimek <[hidden email]> wrote:

On Wed, Jun 1, 2016 at 5:14 PM Piotr Padlewski <[hidden email]> wrote:
Sound like a killer feature for YouCompleteMe!

+ben jackson, who said claimed that ycm is well prepared for this in principle.
 

2016-06-01 16:43 GMT+02:00 Manuel Klimek via cfe-dev <[hidden email]>:
On Wed, May 25, 2016 at 5:24 PM Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

Will be interesting to expose that in libclang. Given that it seems hard for an editor to decide how to insert #includes correctly (unless we somehow also expose clang-format).

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Given the complexity of the current typo correction code (yikes!), introducing this for an alternative warning might be a good idea.
 
Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?

An open question is: for more interesting replacements, like includes, where do we want the decision on how to format / where exactly to insert the #includes to be made; if we don't start with supporting #include insertion and instead focus on alternatives first, that might not actually be a problem, though

 
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
In reply to this post by Neil Nelson via cfe-dev
I think the general idea in Clang has been that fixits on a warning must be behavior-preserving (because we must recover from errors (and warnings can be errors) as if the fixit were applied, so that downstream diagnostics/fixits are applicable). We use notes to express other fixit alternatives (or any non-behavior preserving fixits if there's no behavior-preserving fixit we care to give). Should we not be using notes as alternatives in the include fixer tool? Is there some semantic information missing from multiple notes with fixits that we could improve instead? (that would maintain/improve the existing command line usability) to accurately describe which notes form the set of alternative solutions? (so that an IDE, etc, could collapse/render them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev <[hidden email]> wrote:
I think the general idea in Clang has been that fixits on a warning must be behavior-preserving

Typo-correction is not behavior preserving, though?
 
(because we must recover from errors (and warnings can be errors) as if the fixit were applied, so that downstream diagnostics/fixits are applicable). We use notes to express other fixit alternatives (or any non-behavior preserving fixits if there's no behavior-preserving fixit we care to give). Should we not be using notes as alternatives in the include fixer tool? Is there some semantic information missing from multiple notes with fixits that we could improve instead? (that would maintain/improve the existing command line usability) to accurately describe which notes form the set of alternative solutions? (so that an IDE, etc, could collapse/render them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev <[hidden email]> wrote:
On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev <[hidden email]> wrote:
I think the general idea in Clang has been that fixits on a warning must be behavior-preserving

Typo-correction is not behavior preserving, though?

We do not issue typo-correction FixItHints on warnings. (Well, actually, I think we have a single warning that does this, but it's a bug.)

(because we must recover from errors (and warnings can be errors) as if the fixit were applied, so that downstream diagnostics/fixits are applicable). We use notes to express other fixit alternatives (or any non-behavior preserving fixits if there's no behavior-preserving fixit we care to give). Should we not be using notes as alternatives in the include fixer tool?

+1

This is our usual approach for the case where there are multiple possible fixes: one note per possible approach, with the FixItHints for that approach attached to that note. Is there some reason why this doesn't apply in your case?
 
Is there some semantic information missing from multiple notes with fixits that we could improve instead? (that would maintain/improve the existing command line usability) to accurately describe which notes form the set of alternative solutions? (so that an IDE, etc, could collapse/render them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Wed, Jun 1, 2016 at 8:59 PM Richard Smith <[hidden email]> wrote:
On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev <[hidden email]> wrote:
On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev <[hidden email]> wrote:
I think the general idea in Clang has been that fixits on a warning must be behavior-preserving

Typo-correction is not behavior preserving, though?

We do not issue typo-correction FixItHints on warnings. (Well, actually, I think we have a single warning that does this, but it's a bug.)

(because we must recover from errors (and warnings can be errors) as if the fixit were applied, so that downstream diagnostics/fixits are applicable). We use notes to express other fixit alternatives (or any non-behavior preserving fixits if there's no behavior-preserving fixit we care to give). Should we not be using notes as alternatives in the include fixer tool?

+1

This is our usual approach for the case where there are multiple possible fixes: one note per possible approach, with the FixItHints for that approach attached to that note. Is there some reason why this doesn't apply in your case?

No, that seems fine - the main thing is that what we want is basically very similar to typo correction, so the idea was to add multiple fixits to the typo correction, however that is currently handled... 
 
 
Is there some semantic information missing from multiple notes with fixits that we could improve instead? (that would maintain/improve the existing command line usability) to accurately describe which notes form the set of alternative solutions? (so that an IDE, etc, could collapse/render them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev <[hidden email]> wrote:
Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
  1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
  2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
  3. Add methods to add alternative FixItHints to a diagnostic
  4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
In reply to this post by Neil Nelson via cfe-dev
Ben, would it make sense for YCM to provide the user with choice when
there are notes attached to a diagnostic? You can see this in action
for -Wparentheses:

$ cat t.c
void f(int foo, int bar) {
  if (foo = bar) {}
}

$ clang t.c
t.c:2:11: warning: using the result of an assignment as a condition without
      parentheses [-Wparentheses]
  if (foo = bar) {}
      ~~~~^~~~~
t.c:2:11: note: place parentheses around the assignment to silence this
      warning
  if (foo = bar) {}
          ^
      (        )
t.c:2:11: note: use '==' to turn this assignment into an equality comparison
  if (foo = bar) {}
          ^
          ==

On Wed, Jun 1, 2016 at 8:59 PM, Richard Smith <[hidden email]> wrote:

> On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev
> <[hidden email]> wrote:
>>
>> On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev
>> <[hidden email]> wrote:
>>>
>>> I think the general idea in Clang has been that fixits on a warning must
>>> be behavior-preserving
>>
>>
>> Typo-correction is not behavior preserving, though?
>
>
> We do not issue typo-correction FixItHints on warnings. (Well, actually, I
> think we have a single warning that does this, but it's a bug.)
>
>>> (because we must recover from errors (and warnings can be errors) as if
>>> the fixit were applied, so that downstream diagnostics/fixits are
>>> applicable). We use notes to express other fixit alternatives (or any
>>> non-behavior preserving fixits if there's no behavior-preserving fixit we
>>> care to give). Should we not be using notes as alternatives in the include
>>> fixer tool?
>
>
> +1
>
> This is our usual approach for the case where there are multiple possible
> fixes: one note per possible approach, with the FixItHints for that approach
> attached to that note. Is there some reason why this doesn't apply in your
> case?
>
>>>
>>> Is there some semantic information missing from multiple notes with
>>> fixits that we could improve instead? (that would maintain/improve the
>>> existing command line usability) to accurately describe which notes form the
>>> set of alternative solutions? (so that an IDE, etc, could collapse/render
>>> them differently, perhaps?)
>>>
>>> On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev
>>> <[hidden email]> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> one of the longer-term goals for the include fixer tool is integration
>>>> into the editor. This could piggy-back on the existing support for
>>>> FixIts but there is one big missing feature. It is often not perfectly
>>>> clear what #include a user wants or if it's just a typo that needs
>>>> correcting, we cannot express that with the current FixIts. A Clang
>>>> diagnostic can have multiple FixIts but they are meant to be applied
>>>> together, for this case we'd need alternative groups of FixIts.
>>>>
>>>> There's also a fair number of existing clang warnings that could
>>>> benefit from this, typo correction is an obvious one but there's also
>>>> a bunch of disambiguation warnings that can have fix-its in both
>>>> directions, for example '= in if statement' (turn into '==' or add
>>>> double parens) or '&& within ||' where parens can be added both ways.
>>>> The functionality-preserving FixIt is the canonical one for those
>>>> warnings but having an alternative would be useful, too.
>>>>
>>>> Of course this won't work on the terminal. FixIts are already hardly
>>>> usable there, so this will be a feature for IDEs, code review tools
>>>> etc.
>>>>
>>>> My current plan is:
>>>>   1. Have DiagnosticEngine and friends keep a vector of vector of
>>>> FixItHint instead of a flat FixIt vector
>>>>   2. Thread the change through clang, using alternative '0' in most
>>>> places to avoid breaking existing functionality.
>>>>   3. Add methods to add alternative FixItHints to a diagnostic
>>>>   4. Expose this via a new libclang API. Any diagnostic rendering code
>>>> in clang will stay the same.
>>>>
>>>> There's interest from YouCompleteMe for providing an interface for
>>>> this that lets the user pick a fix out of multiple ones, and I guess
>>>> other IDEs can put it to great use too.
>>>>
>>>> Any input on this approach or concerns?
>>>> _______________________________________________
>>>> cfe-dev mailing list
>>>> [hidden email]
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>>
>>>
>>> _______________________________________________
>>> cfe-dev mailing list
>>> [hidden email]
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
Just following up on this thread: Yes it totally makes sense.

Previously we were ignoring any fixit hints attached to “child” diagnostics (in libclang parlance). I’ve made a quick change to YCM which:
 - extracts fixit hints from chid diagnostics (typically notes)
 - provides a UI in Vim to select between them when multiple fixits are available for a given diag.

It works nicely for the case you mention below: https://files.gitter.im/Valloric/YouCompleteMe/l6FD/YCM-fixit-multiple-choice2.gif. Incidentally, this also improves the workflow when there are multiple diagnostics on the line.

With regard to Manuel’s question about who should be responsible for choosing the insertion location for, say includes, I’d be keen for clang to make that decision, as it has better knowledge about code than the IDE (consider objective-c vs. c vs…). It also requires less work from each client implementation.

That said, in YCM we already have some stuff for working out where to put automatically generated import declarations for c-sharp, and we even provide hooks for the user to tune to their taste, so there is an argument that where the decision is arbitrary, the client should offer the user more flexibility.

On 2 Jun 2016, at 12:00, Benjamin Kramer <[hidden email]> wrote:

Ben, would it make sense for YCM to provide the user with choice when
there are notes attached to a diagnostic? You can see this in action
for -Wparentheses:

$ cat t.c
void f(int foo, int bar) {
 if (foo = bar) {}
}

$ clang t.c
t.c:2:11: warning: using the result of an assignment as a condition without
     parentheses [-Wparentheses]
 if (foo = bar) {}
     ~~~~^~~~~
t.c:2:11: note: place parentheses around the assignment to silence this
     warning
 if (foo = bar) {}
         ^
     (        )
t.c:2:11: note: use '==' to turn this assignment into an equality comparison
 if (foo = bar) {}
         ^
         ==

On Wed, Jun 1, 2016 at 8:59 PM, Richard Smith <[hidden email]> wrote:
On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev
<[hidden email]> wrote:

On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev
<[hidden email]> wrote:

I think the general idea in Clang has been that fixits on a warning must
be behavior-preserving


Typo-correction is not behavior preserving, though?


We do not issue typo-correction FixItHints on warnings. (Well, actually, I
think we have a single warning that does this, but it's a bug.)

(because we must recover from errors (and warnings can be errors) as if
the fixit were applied, so that downstream diagnostics/fixits are
applicable). We use notes to express other fixit alternatives (or any
non-behavior preserving fixits if there's no behavior-preserving fixit we
care to give). Should we not be using notes as alternatives in the include
fixer tool?


+1

This is our usual approach for the case where there are multiple possible
fixes: one note per possible approach, with the FixItHints for that approach
attached to that note. Is there some reason why this doesn't apply in your
case?


Is there some semantic information missing from multiple notes with
fixits that we could improve instead? (that would maintain/improve the
existing command line usability) to accurately describe which notes form the
set of alternative solutions? (so that an IDE, etc, could collapse/render
them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev
<[hidden email]> wrote:

Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
 1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
 2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
 3. Add methods to add alternative FixItHints to a diagnostic
 4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Alternative FixItHints

Neil Nelson via cfe-dev
On Mon, Jun 20, 2016 at 1:24 AM Ben Jackson <[hidden email]> wrote:
Just following up on this thread: Yes it totally makes sense.

Previously we were ignoring any fixit hints attached to “child” diagnostics (in libclang parlance). I’ve made a quick change to YCM which:
 - extracts fixit hints from chid diagnostics (typically notes)
 - provides a UI in Vim to select between them when multiple fixits are available for a given diag.

It works nicely for the case you mention below: https://files.gitter.im/Valloric/YouCompleteMe/l6FD/YCM-fixit-multiple-choice2.gif. Incidentally, this also improves the workflow when there are multiple diagnostics on the line.

This is awesome \o/ Thanks!
 
With regard to Manuel’s question about who should be responsible for choosing the insertion location for, say includes, I’d be keen for clang to make that decision, as it has better knowledge about code than the IDE (consider objective-c vs. c vs…). It also requires less work from each client implementation.

I guess the work for us for the future is that we'll need to somehow expose a way to format code replacements, but that'll need to be designed first.
 
That said, in YCM we already have some stuff for working out where to put automatically generated import declarations for c-sharp, and we even provide hooks for the user to tune to their taste, so there is an argument that where the decision is arbitrary, the client should offer the user more flexibility.

On 2 Jun 2016, at 12:00, Benjamin Kramer <[hidden email]> wrote:

Ben, would it make sense for YCM to provide the user with choice when
there are notes attached to a diagnostic? You can see this in action
for -Wparentheses:

$ cat t.c
void f(int foo, int bar) {
 if (foo = bar) {}
}

$ clang t.c
t.c:2:11: warning: using the result of an assignment as a condition without
     parentheses [-Wparentheses]
 if (foo = bar) {}
     ~~~~^~~~~
t.c:2:11: note: place parentheses around the assignment to silence this
     warning
 if (foo = bar) {}
         ^
     (        )
t.c:2:11: note: use '==' to turn this assignment into an equality comparison
 if (foo = bar) {}
         ^
         ==

On Wed, Jun 1, 2016 at 8:59 PM, Richard Smith <[hidden email]> wrote:
On Wed, Jun 1, 2016 at 11:46 AM, Manuel Klimek via cfe-dev
<[hidden email]> wrote:

On Wed, Jun 1, 2016 at 6:59 PM David Blaikie via cfe-dev
<[hidden email]> wrote:

I think the general idea in Clang has been that fixits on a warning must
be behavior-preserving


Typo-correction is not behavior preserving, though?


We do not issue typo-correction FixItHints on warnings. (Well, actually, I
think we have a single warning that does this, but it's a bug.)

(because we must recover from errors (and warnings can be errors) as if
the fixit were applied, so that downstream diagnostics/fixits are
applicable). We use notes to express other fixit alternatives (or any
non-behavior preserving fixits if there's no behavior-preserving fixit we
care to give). Should we not be using notes as alternatives in the include
fixer tool?


+1

This is our usual approach for the case where there are multiple possible
fixes: one note per possible approach, with the FixItHints for that approach
attached to that note. Is there some reason why this doesn't apply in your
case?


Is there some semantic information missing from multiple notes with
fixits that we could improve instead? (that would maintain/improve the
existing command line usability) to accurately describe which notes form the
set of alternative solutions? (so that an IDE, etc, could collapse/render
them differently, perhaps?)

On Wed, May 25, 2016 at 8:24 AM, Benjamin Kramer via cfe-dev
<[hidden email]> wrote:

Hi all,

one of the longer-term goals for the include fixer tool is integration
into the editor. This could piggy-back on the existing support for
FixIts but there is one big missing feature. It is often not perfectly
clear what #include a user wants or if it's just a typo that needs
correcting, we cannot express that with the current FixIts. A Clang
diagnostic can have multiple FixIts but they are meant to be applied
together, for this case we'd need alternative groups of FixIts.

There's also a fair number of existing clang warnings that could
benefit from this, typo correction is an obvious one but there's also
a bunch of disambiguation warnings that can have fix-its in both
directions, for example '= in if statement' (turn into '==' or add
double parens) or '&& within ||' where parens can be added both ways.
The functionality-preserving FixIt is the canonical one for those
warnings but having an alternative would be useful, too.

Of course this won't work on the terminal. FixIts are already hardly
usable there, so this will be a feature for IDEs, code review tools
etc.

My current plan is:
 1. Have DiagnosticEngine and friends keep a vector of vector of
FixItHint instead of a flat FixIt vector
 2. Thread the change through clang, using alternative '0' in most
places to avoid breaking existing functionality.
 3. Add methods to add alternative FixItHints to a diagnostic
 4. Expose this via a new libclang API. Any diagnostic rendering code
in clang will stay the same.

There's interest from YouCompleteMe for providing an interface for
this that lets the user pick a fix out of multiple ones, and I guess
other IDEs can put it to great use too.

Any input on this approach or concerns?
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev