I know I have been confused on more than one occasion about the precise
requirements for issuing a fixit hint. After several IRC conversations
(thanks to those that participated) I think I've got a decent
understanding, and I'd like to add some sections to the Clang
documentation which spell this out for future reference. A brief summary
of my understanding lest I have gotten confused again:
1) Fixit hints attached to an error diagnostic *must* recover the
parse/semantic analysis/etc as if the fix had been applied. This won't
ever miscompile code as the compilation cannot succeed after issuing an
error unless Clang has been asked to automatically apply these fixes, in
which case the compile will succeed, and accurately reflect the (newly
updated) code.
2) Fixit hints on notes *must not* have any impact on the compilation.
The
also are not automatically applied to the code by the -fixit flag.
3) There can be only one hint attached to a diagnostic, and thus if the
hint is attached to the error or warning diagnostic it must be an
extremely confident fix with no other viable candidates. When there are
multiple viable candidate fixes, they should be presented as multiple
fixit hint bearing notes.
The one area this doesn't cover are fixit hints attached to warnings.
These
are trickier. Previously, it has been suggested that they should follow
the same rules as those attached to errors, but that has some serious
problems. Suppose this is the approach is used for -Wmismatched-tags:
---- x.cc ----
class X; struct X { X() {}
};
X x;
----
This code is well formed, but the warning will suggest replacing
'struct'
with 'class'. Doing so makes the code ill-formed. If we recover as-if
the fixit hint were applied, we would error on this code when the
warning is turned on, which seems rather surprising. ;] If we don't
recover as if the fixit hint were applied, and run Clang with -fixit, we
accept the code, but then alter the code to a text sequence which if we
recompile is rejected. Again, rather surprising. Currently, Clang's
warnings which have fixit hints attached have a mixture of these
policies, but more often follow the latter policy of no recovery. For
some, this is a moot point -- the code parses the same either way and
the hint merely removes redundant text or adds clarifying text. The
question is what *should* the rest of the warnings with fixit hints do?
One option would be to require that warnings with direct fixit hints
*can*
recover as if the hint were applied, but provide a hook so that the
recovery is only performed when that warning is promoted to an error or
when running with -fixit in effect.
A second option is to require that recovery not be performed for
warning fixit hints and that -fixit not apply them automatically.
Essentially treat
them the same as note fixit hints.
I prefer the second option as it is simpler, and I think provides a
better user experience. There are several warnings with a note attached
to them purely to provide a fixit hint without recover or automatic
application. The output would be simpler and more clear if these could
be directly attached to the warnings.
Thoughts? Once this is a bit more clear, I'll start fixing
documentation.
I like the second option with a refinement: the warning fixit must be one
that removes this warning and does not change the semantics or validity
of the program (it may change the AST). If it may change the program then
it needs to be a note. Then, -fixit should apply warning
I like this option, with a refinement. 
We should only apply fixits with -fixit if we are supremely confident that
the fixed code does the right thing. Therefore, fixits on warnings should
only be allowed to fix cosmetic issues. If we think the code might be
wrong, then suggested fixes (including one to remove the warning) should
be placed in notes separate from the warning.
That way, we never do recovery that follows the fixits except on errors,
the fixits like -Wparenthesis will still fire to add the parens in a way
that doesn't actually change anything, -Wmismatched-tags would use a note
fixit to suggest its change.
-Wparenthesis is a great example of a warning which I think -fixit should
not fix, since it frequently finds real bugs. It would be great if we
could produce two notes with the warning suggesting parentheses around
(for instance) the && or the ||.
Actually -Wmismatched-tags should also add public: / private: so that we
could upgrade its fixit from a note to a warning and let clang -fixit fix
things.
I think it'd be much better for -Wmismatched-tags to fix the declaration
and leave the definition alone, but I agree in principle: this is a
cosmetic issue, not a 'code is wrong' issue, and it'd be nice if -fixit
fixed it.
Richard
I know I have been confused on more than one occasion about the precise
requirements for issuing a fixit hint. After several IRC conversations
(thanks to those that participated) I think I've got a decent
understanding, and I'd like to add some sections to the Clang
documentation which spell this out for future reference. A brief summary
of my understanding lest I have gotten confused again:
1) Fixit hints attached to an error diagnostic *must* recover the
parse/semantic analysis/etc as if the fix had been applied. This won't
ever miscompile code as the compilation cannot succeed after issuing an
error unless Clang has been asked to automatically apply these fixes, in
which case the compile will succeed, and accurately reflect the (newly
updated) code.
2) Fixit hints on notes *must not* have any impact on the compilation.
The
also are not automatically applied to the code by the -fixit flag.
3) There can be only one hint attached to a diagnostic, and thus if the
hint is attached to the error or warning diagnostic it must be an
extremely confident fix with no other viable candidates. When there are
multiple viable candidate fixes, they should be presented as multiple
fixit hint bearing notes.
The one area this doesn't cover are fixit hints attached to warnings.
These
are trickier. Previously, it has been suggested that they should follow
the same rules as those attached to errors, but that has some serious
problems. Suppose this is the approach is used for -Wmismatched-tags:
---- x.cc ----
class X; struct X { X() {}
};
X x;
----
This code is well formed, but the warning will suggest replacing
'struct'
with 'class'. Doing so makes the code ill-formed. If we recover as-if
the fixit hint were applied, we would error on this code when the
warning is turned on, which seems rather surprising. ;] If we don't
recover as if the fixit hint were applied, and run Clang with -fixit, we
accept the code, but then alter the code to a text sequence which if we
recompile is rejected. Again, rather surprising. Currently, Clang's
warnings which have fixit hints attached have a mixture of these
policies, but more often follow the latter policy of no recovery. For
some, this is a moot point -- the code parses the same either way and
the hint merely removes redundant text or adds clarifying text. The
question is what *should* the rest of the warnings with fixit hints do?
One option would be to require that warnings with direct fixit hints
*can*
recover as if the hint were applied, but provide a hook so that the
recovery is only performed when that warning is promoted to an error or
when running with -fixit in effect.
A second option is to require that recovery not be performed for
warning fixit hints and that -fixit not apply them automatically.
Essentially treat
them the same as note fixit hints.
I prefer the second option as it is simpler, and I think provides a
better user experience. There are several warnings with a note attached
to them purely to provide a fixit hint without recover or automatic
application. The output would be simpler and more clear if these could
be directly attached to the warnings.
Thoughts? Once this is a bit more clear, I'll start fixing
documentation.
I like the second option with a refinement: the warning fixit must be one
that removes this warning and does not change the semantics or validity
of the program (it may change the AST). If it may change the program then
it needs to be a note. Then, -fixit should apply warning
I like this option, with a refinement.
We should only apply fixits with -fixit if we are supremely confident that
the fixed code does the right thing. Therefore, fixits on warnings should
only be allowed to fix cosmetic issues. If we think the code might be
wrong, then suggested fixes (including one to remove the warning) should
be placed in notes separate from the warning.
I agree with this. I don't want the behavior of the "-fixit" mode to different for warnings vs. errors. Rather, I want the warnings to follow the same principle as errors: only fix if you *know* you're right, which means we should only fix cosmetic issues.
That way, we never do recovery that follows the fixits except on errors,
the fixits like -Wparenthesis will still fire to add the parens in a way
that doesn't actually change anything, -Wmismatched-tags would use a note
fixit to suggest its change.
-Wparenthesis is a great example of a warning which I think -fixit should
not fix, since it frequently finds real bugs. It would be great if we
could produce two notes with the warning suggesting parentheses around
(for instance) the && or the ||.
At present, we just produce one note that tells how to silence the warning.
Actually -Wmismatched-tags should also add public: / private: so that we
could upgrade its fixit from a note to a warning and let clang -fixit fix
things.
I think it'd be much better for -Wmismatched-tags to fix the declaration
and leave the definition alone, but I agree in principle: this is a
cosmetic issue, not a 'code is wrong' issue, and it'd be nice if -fixit
fixed it.
Sure, but in doing so, we shouldn't change "struct" to "class", since we have no idea whether the code is in a header that will also be included in C.
The -Wmismatched-tags Fix-It is a real mess 
- Doug