This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add a Standalone diagnostics mode to clang-tidy
ClosedPublic

Authored by njames93 on Feb 20 2021, 11:10 AM.

Details

Summary

Adds a flag to ClangTidyContext that is used to indicate to checks that fixes will only be applied one at a time.
This is to indicate to checks that each fix emitted should not depend on any other fixes emitted across the translation unit.
I've currently implemented the IncludeInserter, LoopConvertCheck and PreferMemberInitializerCheck to use these support these modes.

Reasoning behind this is in use cases like clangd it's only possible to apply one fix at a time.
For include inserter checks, the include is only added once for the first diagnostic that requires it, this will result in subsequent fixes not having the included needed.

A similar issue is seen in the PreferMemberInitializerCheck where the : will only be added for the first member that needs fixing.

Fixes emitted in StandaloneDiagsMode will likely result in malformed code if they are applied all together, conversely fixes currently emitted may result in malformed code if they are applied one at a time.
For this reason invoking clang-tidy from the binary will always with StandaloneDiagsMode disabled, However using it as a library its possible to select the mode you wish to use, clangd always selects StandaloneDiagsMode.

This is an example of the current behaviour failing

struct Foo {
  int A, B;
  Foo(int D, int E) {
    A = D;
    B = E; // Fix Here
  }
};

Incorrectly transformed to:

struct Foo {
  int A, B;
  Foo(int D, int E), B(E) {
    A = D;
     // Fix Here
  }
};

In StandaloneDiagsMode, it gets transformed to:

struct Foo {
  int A, B;
  Foo(int D, int E) : B(E) {
    A = D;
     // Fix Here
  }
};

Diff Detail

Event Timeline

njames93 created this revision.Feb 20 2021, 11:10 AM
njames93 updated this revision to Diff 325238.Feb 20 2021, 1:36 PM

Disable formatting causing test to fail and document it.

njames93 updated this revision to Diff 325239.Feb 20 2021, 1:38 PM

Remove unintended change.

njames93 published this revision for review.Feb 21 2021, 5:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2021, 5:39 AM

I just tried to apply this, but I think it conflicts with your other change to PreferMemberInitializerCheck.cpp. Please rebase it.

steveire added inline comments.Feb 22 2021, 3:33 PM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
421

Very minor, but I think this should be run.

clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
81

The comment and the code seem to me to be opposites. The code says "if SingleFixMode is true and we have already inserted the header, don't insert it again." The comment says that "SingleFixMode does not prevent inserting it again" -- The comment seems to be the opposite of the code?

njames93 added inline comments.Feb 22 2021, 3:50 PM
clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
81

That't not what the code is saying, the magic is clear in the else branch. If SingleFixMode is enabled we are only querying the set, If its disabled then we are updating the set.
The real shortfall is the map is called InsertedHeaders which is not a good reflection of what it actually contains. It actually contains all the headers included in each file. Which in Multi fix mode gets updated with each include insertion.

steveire added inline comments.Feb 22 2021, 4:25 PM
clang-tools-extra/clang-tidy/utils/IncludeInserter.cpp
81

Ah, the "and we have already inserted the header, don't insert it again" is not true because we never do an insert in SingleFixMode.

Perhaps update the comment to say that the InsertedHeaders is never added to in SingleFixMode, so it only contains the headers that are already there? If something like that is true.

clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
95

I think the true here points to this being a boolean trap. Consider using an enum for the parameter instead.

287

I still find it really confusing that the "single inserter" mode results in multiple of the same header being added. Perhaps the names should be along the lines of "duplicating" and "deduplicating" instead of "single" and "multi"?

njames93 updated this revision to Diff 325608.Feb 22 2021, 4:29 PM
njames93 marked 2 inline comments as done.

Add test cases to clangd showing the improved behaviour of fix-its.
Rebased.
Update some comments.

njames93 updated this revision to Diff 325622.Feb 22 2021, 5:06 PM

Add include insertion test for clangd showing it attaching the include for multiple warnings.

steveire added inline comments.Feb 28 2021, 4:47 PM
clang-tools-extra/clang-tidy/ClangTidyCheck.h
423

I find the naming of this as "single fix" confusing.

Something along the lines of "self contained diagnostics" or so I think would be better.

njames93 updated this revision to Diff 328134.Mar 4 2021, 5:09 AM

Rename mode to self contained diags.
Added support for modernize-loop-convert. Currently transforming a loop will prevent any nested loops being transformed if they are affected by the first transformation. This is counter productive in self contained diags mode.

njames93 retitled this revision from [clang-tidy] Add a single fix mode to clang-tidy to [clang-tidy] Add a Standalone diagnostics mode to clang-tidy.Mar 4 2021, 8:26 AM
njames93 edited the summary of this revision. (Show Details)
njames93 added a project: Restricted Project.
steveire added inline comments.Mar 14 2021, 7:25 AM
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
178

I don't think this Value should be = true. It makes call sites confusing.

There is precedent though according to

git grep "void set.*bool.*=.*)"

so consider this a nit.

217

The order of members here is starting to look suspicious, size-wise. Not something for this change though.

clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp
803–804

Can you update this comment to not refer to "single fix" mode?

clang-tools-extra/clang-tidy/utils/IncludeInserter.h
63

Should the "single fix" naming here also be changed?

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
522

More "single fix" naming here.

clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
34

"Single fix" naming here too.

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2021, 7:25 AM
njames93 marked 6 inline comments as done.Mar 14 2021, 8:14 AM
njames93 added inline comments.
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
217

Not something I'm prepared to worry about in this patch.

clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
95

I don't see a point in defining an enum for this when its only a test case, though a parameter comment should be added.

287

The name of the test is InsertMultipleIncludesNoDeduplicate, Is that not sufficient?

njames93 updated this revision to Diff 331023.Mar 16 2021, 9:54 AM

Turns out we don't need to worry about InsertedHeaders at all in IncludeInserter.

steveire added inline comments.Mar 16 2021, 3:45 PM
clang-tools-extra/unittests/clang-tidy/IncludeInserterTest.cpp
287

Yes, the comment you're responding to here is quite old and predates the renaming.

njames93 marked 2 inline comments as done.Mar 16 2021, 3:47 PM
njames93 marked an inline comment as done.
njames93 updated this revision to Diff 341167.Apr 28 2021, 5:46 AM

Rebase and fix up new checks added and changes to tests.

njames93 marked an inline comment as done.Apr 28 2021, 5:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 12:14 PM

I'd like to hear from @sammccall as well, but I think the changes here LGTM. Can you please add a release note for the fix?

sammccall accepted this revision.Apr 13 2022, 12:05 PM

Thank you!

(I'm sorry, somehow I had misunderstood what this patch was about, and wasn't paying attention)

This revision is now accepted and ready to land.Apr 13 2022, 12:05 PM

I'd like to hear from @sammccall as well, but I think the changes here LGTM. Can you please add a release note for the fix?

Is this a clang-tidy, clangd or both release notes.
The changes aren't visible in the clang-tidy binary, but are when using it as a library.
However these changes do have noticable effects in clangd with some clang-tidy checks running.

I'd like to hear from @sammccall as well, but I think the changes here LGTM. Can you please add a release note for the fix?

Is this a clang-tidy, clangd or both release notes.
The changes aren't visible in the clang-tidy binary, but are when using it as a library.
However these changes do have noticable effects in clangd with some clang-tidy checks running.

I'd probably put the note under clangd as that's going to be most relevant for users, WDYT?

njames93 updated this revision to Diff 423230.Apr 16 2022, 1:17 AM

Added clangd release notes.