Page MenuHomePhabricator

[clang-tidy] Added command line option `fix-notes`
Needs ReviewPublic

Authored by njames93 on Jul 30 2020, 2:22 AM.

Details

Summary

Added an option to control whether to apply the fixes found in notes attached to clang tidy errors or not.
Diagnostics may contain multiple notes each offering different ways to fix the issue, for that reason the default behaviour should be to not look at fixes found in notes.
Instead offer up all the available fix-its in the output but don't try to apply the first one unless -fix-notes is supplied.

Diff Detail

Event Timeline

njames93 created this revision.Jul 30 2020, 2:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 2:22 AM
njames93 requested review of this revision.Jul 30 2020, 2:22 AM
njames93 retitled this revision from [clang-tidy] Added command line option `fix-notes` to [clang-tidy][WIP] Added command line option `fix-notes`.EditedJul 30 2020, 2:22 AM
njames93 added a project: Restricted Project.

This is very much a work in progress
Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

Eugene.Zelenko added inline comments.Jul 30 2020, 9:06 AM
clang-tools-extra/clang-tidy/ClangTidy.cpp
110

It'll be reasonable to use member initialization for TotalFixes, AppliedFixes, WarningsAsErrors.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
268–269

It'll be reasonable to use member initialization for LastErrorRelatesToUserCode, LastErrorPassesLineFilter, LastErrorWasIgnored.

njames93 marked 2 inline comments as done.Jul 30 2020, 9:30 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidy.cpp
110

It would be, but that's a refactoring change unrelated to this patch

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
268–269

Ditto

njames93 updated this revision to Diff 304145.Nov 10 2020, 5:06 AM
njames93 marked 2 inline comments as done.

Made -fix-notes imply -fix

njames93 updated this revision to Diff 308153.Nov 28 2020, 4:59 AM

Fix documentation.

Ping??

Fix documentation.

Ping??

Ah, sorry, this fell to the bottom of my review list because the title still says [WIP] and so I thought there was more work being done on it. You may want to remove that bit from the title.

This is very much a work in progress
Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

I was wondering the same thing here myself. If there's exactly one fix, then it's unambiguous as to what behavior you get. One (minor) concern I have about the current approach is with nondeterminism in diagnostic ordering where different runs may pick different fixes for the same code. I don't *think* we have any instances of this in Clang or clang-tidy, but users can add their own plugins (for instance to the clang static analyzer) that may introduce some small risk there. Do you have a reason why you picked one approach over the other?

clang-tools-extra/clang-tidy/ClangTidy.h
103

I think you should add some comments explaining the difference between Fix and AnyFix because it's not super clear. Alternatively, if these are strongly related, should this be an enumeration rather than two boolean parameters? e.g., enum FixitBehavior { NoFixes, ApplyNonNoteFixes, ApplyAllFixes }; or some such (note, this suggestion may or may not make sense, I'm still reviewing).

clang-tools-extra/test/clang-tidy/checkers/misc-definitions-in-headers.hpp
1

Hmm, should that be --fix-notes? (Note the number of dashes, which differs from the documentation)

clang-tools-extra/test/clang-tidy/infrastructure/alternative-fixes.cpp
1

Same question here as above.

njames93 updated this revision to Diff 313080.Mon, Dec 21, 4:06 AM
njames93 marked 2 inline comments as done.

Use enum for handling fix behaviour.
Fix tests using --fix-notes instead of -fix-notes.

njames93 retitled this revision from [clang-tidy][WIP] Added command line option `fix-notes` to [clang-tidy] Added command line option `fix-notes`.Mon, Dec 21, 4:07 AM

This is very much a work in progress
Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

I was wondering the same thing here myself. If there's exactly one fix, then it's unambiguous as to what behavior you get. One (minor) concern I have about the current approach is with nondeterminism in diagnostic ordering where different runs may pick different fixes for the same code. I don't *think* we have any instances of this in Clang or clang-tidy, but users can add their own plugins (for instance to the clang static analyzer) that may introduce some small risk there. Do you have a reason why you picked one approach over the other?

Part of the reason for this approach is from this bug report https://bugs.llvm.org/show_bug.cgi?id=47971, where its pointed out that clang diags gets fix-its added in notes if the fixes will change behaviour or they aren't sure its going to actually fix the issue.
As clang-tidy also applies fixes reported from clang, it is wise to adhere to a similar level caution.

This is very much a work in progress
Another direction I was thinking was only apply the fixes found in notes if there is exactly one fix attached to the notes in a diagnostic, instead of just applying the first one we find

I was wondering the same thing here myself. If there's exactly one fix, then it's unambiguous as to what behavior you get. One (minor) concern I have about the current approach is with nondeterminism in diagnostic ordering where different runs may pick different fixes for the same code. I don't *think* we have any instances of this in Clang or clang-tidy, but users can add their own plugins (for instance to the clang static analyzer) that may introduce some small risk there. Do you have a reason why you picked one approach over the other?

Part of the reason for this approach is from this bug report https://bugs.llvm.org/show_bug.cgi?id=47971, where its pointed out that clang diags gets fix-its added in notes if the fixes will change behaviour or they aren't sure its going to actually fix the issue.
As clang-tidy also applies fixes reported from clang, it is wise to adhere to a similar level caution.

Agreed, but I don't think the current behavior in this patch is cautious enough. I like that we have to specify "please apply fixits from notes" explicitly. I think that's the right behavior we want. But if there are multiple fixits attached to the notes for a diagnostic, I don't think we should assume that the first fix-it is the correct one to apply -- I think the user should have to manually pick the variant they want in that case. So I think the behavior should be to apply the fixits from notes if there's only a single fixit to be applied. WDYT?

Agreed, but I don't think the current behavior in this patch is cautious enough. I like that we have to specify "please apply fixits from notes" explicitly. I think that's the right behavior we want. But if there are multiple fixits attached to the notes for a diagnostic, I don't think we should assume that the first fix-it is the correct one to apply -- I think the user should have to manually pick the variant they want in that case. So I think the behavior should be to apply the fixits from notes if there's only a single fixit to be applied. WDYT?

I can agree with that, One thing I like about using clangd is it will show you all available fixes, letting you pick and choose.

njames93 updated this revision to Diff 313272.Tue, Dec 22, 2:43 AM

Only apply fixes from notes if there is exactly one fix found in them.

aaron.ballman added inline comments.Tue, Dec 22, 5:15 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
231

Comment seems stale.

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
186

Should this change be a separate patch?

clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
222

Same for these changes?

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
500

Might as well fix this clang-format warning.

clang-tools-extra/docs/clang-tidy/index.rst
176

This sort of reads like it may apply fixes from multiple notes. How about: If a warning has no fix, but a single fix can be found through an associated diagnostic note, apply the fix. Specifying this flag will implicitly enable the '--fix' flag.

(Note, I also quietly switched from using `` to using '' for quoting -- that seems to be the consistent style here.)