[cfe-dev] Clarifying the roles and requirements on fixit hints in diagnostics

[Douglas Gregor]

On Apr 26, 2011, at 5:20 AM, Richard Smith wrote:

> On Sat, 23 Apr 2011 19:27, Nick Lewycky <nlewycky at google.com> wrote:
>> On 10 April 2011 01:37, Chandler Carruth <chandlerc at google.com> wrote:
>>> 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