Alternative FixItHints

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?

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

Sound like a killer feature for YouCompleteMe!

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 :slight_smile:

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 :slight_smile:

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?)

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?

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…

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) {}
          ^

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.

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.