Page MenuHomePhabricator

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

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
106

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

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
266–267

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
106

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

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
266–267

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.Dec 21 2020, 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`.Dec 21 2020, 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.Dec 22 2020, 2:43 AM

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

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

Comment seems stale.

clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
186 ↗(On Diff #313272)

Should this change be a separate patch?

clang-tools-extra/clang-tidy/readability/InconsistentDeclarationParameterNameCheck.cpp
222 ↗(On Diff #313272)

Same for these changes?

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

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

njames93 updated this revision to Diff 326779.Feb 26 2021, 1:00 PM
njames93 marked 6 inline comments as done.

Rebased and address comments

njames93 added inline comments.Feb 27 2021, 3:36 AM
clang-tools-extra/clang-tidy/misc/UnusedUsingDeclsCheck.cpp
186 ↗(On Diff #313272)

It's either modify the check to put the fix in the warning, or modify the test so that fix notes is specified.

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

I think this is a discrepancy with the clang format in using (truck) and the pre merge. Mine isn't complaining about format

This revision is now accepted and ready to land.Mar 1 2021, 6:48 AM
This revision was landed with ongoing or failed builds.Mar 1 2021, 2:07 PM
This revision was automatically updated to reflect the committed changes.