This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] remove duplicate fixes of alias checkers
ClosedPublic

Authored by Daniel599 on May 28 2020, 12:18 PM.

Details

Summary

when both a check and its alias are enabled, we should only take the fixes of one of them and not both.
This patch fixes bug 45577
https://bugs.llvm.org/show_bug.cgi?id=45577

Diff Detail

Event Timeline

Daniel599 created this revision.May 28 2020, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 12:18 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
751

Please don't use auto when type is not spelled in same statement or not iterator.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
4

cstdio. Please also separate with newline.

40

Please add newline.

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
40

New line

Daniel599 marked 4 inline comments as done.May 28 2020, 3:08 PM
Daniel599 added inline comments.
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
4

I wanted to use cstdio but the test fails with 'file not found', don't know if it's specific on my host, anyway I`ll remove this include (and the printf)

Daniel599 updated this revision to Diff 267056.May 28 2020, 3:09 PM
Daniel599 marked an inline comment as done.
Daniel599 marked an inline comment as done.
Daniel599 updated this revision to Diff 267058.May 28 2020, 3:12 PM
Daniel599 marked an inline comment as not done.
Daniel599 marked an inline comment as done.

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

I think this is a nice approach. It also helps the case where a user sees a false positive diagnostic and tries to disable it in their config file. Under the removing duplicates behavior, the user would have a harder time discovering what tests to disable, but under the above approach, they'd have all the information they need up front.

clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
4

<cstdio> would have been wrong anyway -- clang-tidy (like clang) tests should not include system headers (unless they're faked system header so they're consistent across build bots).

Daniel599 marked an inline comment as done.May 29 2020, 11:00 AM
Daniel599 added a comment.EditedMay 29 2020, 1:58 PM

It may be worth verifying that the fix-its are identical too, multiple versions of a check could be running with differing options resulting in different fix-its generated, in that case we should let clang-tidy disable any conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, thinking either of these ways

CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace, modernize-use-emplace]
CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that note.

I think this is a nice approach. It also helps the case where a user sees a false positive diagnostic and tries to disable it in their config file. Under the removing duplicates behavior, the user would have a harder time discovering what tests to disable, but under the above approach, they'd have all the information they need up front.

Regarding different fix-its generated:
you are right, in order to fix it, I had to add StringMap::operator == (soon I`ll update the diff), I hope it's OK.
I added the option cppcoreguidelines-pro-type-member-init.UseAssignment to check it, but the result I got was something like:

int _num2 = 0{};

I think that if the fix-its aren't the same, both of them should be disabled with warning about it, what do you think?

Regarding placing all errors of all aliases in one error:
My original goal was to fix the bug that was introduce since clang-tidy-9, what you are suggesting sounds really cool, but I am still new to this code,
Any idea how can I implement such feature? also what should I do if the options of the alias checks aren't the same?

Daniel599 updated this revision to Diff 267455.May 30 2020, 6:50 AM

I have made the needed changes in order to detect a conflict in duplicated fix-it of aliased checkers and also added a test for it.
Now I`ll try to work on combining aliased errors together, any tips regarding this feature?

Herald added a project: Restricted Project. · View Herald TranscriptMay 30 2020, 6:50 AM

I have made the needed changes in order to detect a conflict in duplicated fix-it of aliased checkers and also added a test for it.
Now I`ll try to work on combining aliased errors together, any tips regarding this feature?

Here is a quick draft I built up for this ClangTidyDiagnosticsConsumer.cpp lmk what you think.
Some of the handling probably can be changed for conflicting fix its.

Daniel599 updated this revision to Diff 267478.May 30 2020, 1:56 PM

Thank you @njames93, I already started and took a bit different approach.
In case of a conflict, leaving it to clang-tidy itself didn't help as it added both of the fix-its together resulting in = 0{};, so I thought it will be better to disable both of them.
Sadly I didn't find 3 aliased checkers which can be configured to produce different code so I can't check this edge case.
Please let me know if another changes are needed for this patch

Thank you @njames93, I already started and took a bit different approach.
In case of a conflict, leaving it to clang-tidy itself didn't help as it added both of the fix-its together resulting in = 0{};, so I thought it will be better to disable both of them.

Fair enough, your solution looks a lot nicer than what I suggested anyway.

Sadly I didn't find 3 aliased checkers which can be configured to produce different code so I can't check this edge case.
Please let me know if another changes are needed for this patch

You could create a unit test for 3 aliased checks with different options, however if there isn't a check that users can use to reproduce that edge case there is no point in worrying about it.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
766–779

s/(*Inserted.first)->/ExistingError.

780
ExistingError.Notes.emplace_back(llvm::formatv(
    "cannot apply fix-it because an alias checker has "
    "suggested a different fix-it, please remove one of the checkers "
    "or make sure they are both configured the same. "
    "Aliased checkers: '{0}', '{1}'",
    ExistingError.DiagnosticName, Error.DiagnosticName).str());
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
51

Just ignore this, but do you ever get so bored and feel the need to save 24 bytes... https://gist.github.com/njames93/aac662ca52d345245e5e6f5dbc47d484 :)

Daniel599 marked 2 inline comments as done.May 30 2020, 9:49 PM
Daniel599 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
51

That's a really cool trick, good to know :)

Daniel599 updated this revision to Diff 267489.May 30 2020, 9:50 PM
Daniel599 marked an inline comment as done.
Daniel599 marked 2 inline comments as done.
Daniel599 added inline comments.May 30 2020, 9:53 PM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
250

maybe I need to rename this method since now it's removing the errors also, I`ll do it when I get back from work.

njames93 added inline comments.May 30 2020, 11:46 PM
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
5–9

This isn't needed for the test case and can safely be removed.

14–22

The 2 constructors can be removed as well as the destructor.

llvm/include/llvm/ADT/StringMap.h
251–268

This needs unit tests llvm/unittests/ADT/StringMapTest.cpp, also a small nit but could you add the corresponding operator!=

Daniel599 updated this revision to Diff 268055.Jun 2 2020, 10:17 PM

Added StringMap::operator!= and also unit tests

Daniel599 marked 4 inline comments as done.Jun 2 2020, 10:17 PM
njames93 added inline comments.Jun 6 2020, 3:42 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
777

You don't really need the Aliased checkers note as that is wrapped in the initial diagnostic message.
Also if there was a check that could spew out 3 different fix-its for the same diagnostic, this would result in the duplication note being made twice, maybe the notes should be cleared too?

Daniel599 marked an inline comment as done.Jun 6 2020, 4:06 AM
Daniel599 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
777

regarding your comment about 3 fix-it, as we walked before, there isn't a case like that (I didn't find any) as I wanted to make a test out of it.
I added the last line as it would show who are the two that conflict (supporting the future case of more than 2 aliased checkers),
I can remove it if it doesn't help, let me know.

maybe the notes should be cleared too?

I am open for suggestions about how to re-write this message :)
I also think it could be better

Any additional changes/fixes regarding this patch?
Thanks.

Daniel599 marked an inline comment as done.Jun 15 2020, 10:57 AM

ping?

aaron.ballman accepted this revision.Jun 18 2020, 5:09 AM

LGTM aside from a few small nits, but please wait a bit for @njames93 in case they have final thoughts.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
758

Add full stops to the end of your comments (here and elsewhere).

777

I would reword it to be a run-on sentence: cannot apply fix-it because an alias checker has suggested a different fix-it; please remove one of the checkers ('{0}', '{1}') or ensure they are both configured the same

This also nicely removes the nit about terminating punctuation in a diagnostic.

clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
250

I think this should still be renamed. How about removeDuplicatedDiagnosticsOfAliasCheckers()?

This revision is now accepted and ready to land.Jun 18 2020, 5:09 AM
njames93 accepted this revision.Jun 18 2020, 8:11 AM

LG, I have nothing else to add here.

Daniel599 updated this revision to Diff 272052.Jun 19 2020, 6:47 AM
Daniel599 marked 3 inline comments as done.Jun 19 2020, 6:50 AM

I fixed all as your comments suggested, thanks :)
If this patch is ready, can someone commit it on my behalf please? (I don't have write permissions)
Thank you.

This revision was automatically updated to reflect the committed changes.