Page MenuHomePhabricator

[clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables
Needs RevisionPublic

Authored by JonasToth on Nov 27 2018, 4:04 AM.

Details

Summary

This patch connects the check for const-correctness with the new general
utility to add const to variables.
The code-transformation is only done, if the detected variable for const-ness
is not part of a group-declaration.

The check allows to control multiple facets of adding const, e.g. if pointers themself should be
marked as const if they are not used.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth marked an inline comment as done.Jan 30 2022, 11:47 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
16 ↗(On Diff #404297)

CHECK-FIXES?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

Shouldn't this validate that the const was placed in the correct position?
e.g. const double * is a different meaning from double *const

Apply to all the other CHECK-FIXES as well

0x8000-0000 added inline comments.Jan 31 2022, 6:17 PM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

Can we have the checker merged in first, then we can worry about the automatic fixer?

JonasToth marked 2 inline comments as done.Feb 1 2022, 1:49 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
16 ↗(On Diff #404297)

see my comment in the other test.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

the checker is its own utility with its own tests and proper test coverage.
yes const double* and double* const are different and are correctly inserted, but that is not tested here, but here: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp

Similar to the actual const analysis. That has its own test-suite (https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp)

The tests here are concerned with the diagnostics and "real" code. Afterall, this functionality is too complex to have all of it checked with these kind of tests.
I think the separate testing in specialized unit-tests (as is now) for the specialized functions is the right approach and the CHECK-FIXES are not necessary in this instance, maybe even bad, because it makes the tests unclearer.

JonasToth marked 2 inline comments as done.Feb 1 2022, 1:52 AM
JonasToth added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

sry: The _fixer_ is its own utility ...

Additionally: The test is run on big projects with transformation (LLVM, some Qt Stuff).
First everything is transformed and then recompiled. The compiler tells what was incorrectly inserted :)

Thats part of the testing too.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

The tests here are concerned with the diagnostics and "real" code.

OK, great! I didn't realize it was covered by unit tests,
which is perfectly fine with me :)

JonasToth added inline comments.Feb 2 2022, 7:07 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
12 ↗(On Diff #404297)

Perfect!

Yeah, the check and patches here are a bit all over the place, which is another reason why a merge would be really great :D

@aaron.ballman - can this land for Clang14, or does it have wait for 15?

Are there any other reviewers that can approve this?

@aaron.ballman - can this land for Clang14, or does it have wait for 15?

Are there any other reviewers that can approve this?

Sadly, cherry-picking to 14 is very unlikely, as this is not a bugfix. :/
On the other hand, it missed so many releases so lets stay positive :)

Aaron did the review and I think he should accept too.

@aaron.ballman - can this land for Clang14, or does it have wait for 15?

Are there any other reviewers that can approve this?

Sadly, cherry-picking to 14 is very unlikely, as this is not a bugfix. :/
On the other hand, it missed so many releases so lets stay positive :)

Aaron did the review and I think he should accept too.

This won't get backported now, besides giving it a few months to test(and potentially iron out) on the dev branch isn't such a bad thing.

clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
33 ↗(On Diff #404297)

Will this work under clangd as I don't think that links in the clangAnalysis libraries @sammccall?

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
27 ↗(On Diff #404297)

nit

84 ↗(On Diff #404297)

allOf is unnecessary here as declStmt is implicitly an allOf matcher.
Also findAll isn't the right matcher in this case, you likely want forEachDescendant. findAll will try and match on the CompoundStmt which is always going to fail.

102–105 ↗(On Diff #404297)

This sounds like a bug that should be raised with ASTMatchers, if its reproducible.

160 ↗(On Diff #404297)

This is unnecessary, llvm::Optional already has an alias inside the clang namespace.

162–168 ↗(On Diff #404297)

DiagnosticBuilders accept optional fixits as syntactic sugar.
Same goes for below.

clang-tools-extra/docs/ReleaseNotes.rst
108–112

This'll need rebasing after branching.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
3–6 ↗(On Diff #404297)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
3–5 ↗(On Diff #404297)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
3–5 ↗(On Diff #404297)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
3–5 ↗(On Diff #404297)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
3–5 ↗(On Diff #404297)
JonasToth added inline comments.Feb 7 2022, 12:25 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
33 ↗(On Diff #404297)

I did not check if it works and clangd does not explicitly link to Analysis, but only to the checks.
Doesn't the LINK_LIBS induce transitive dependency resolution for linking?

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
102–105 ↗(On Diff #404297)

I found no way to reproduce it (yet).
The crashes occured during verification on big projects and run-clang-tidy kind of obfuscated where the crash happened.

This is something for the iron out part of the check I think.

sammccall added inline comments.Feb 7 2022, 1:13 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
33 ↗(On Diff #404297)

I don't know whether you need to add it everywhere, I suspect transitive is OK.

In any case this isn't a new dependency, Sema depends on Analysis and clangd depends on Sema.

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
29 ↗(On Diff #404297)

This returns an iterator (i.e. a pointer), which is being converted to a boolean.

i.e. it's always returning true. I think this is why you're seeing nullptr crashes on Variable.

29 ↗(On Diff #404297)

this is depending on ::internal details, which doesn't seem OK.
I think you'd need to find another way to do it, or move this to ASTMatchers (in a separate patch).

102–105 ↗(On Diff #404297)

This is a bug in a helper added in this patch, I've added a comment above.

njames93 added inline comments.Feb 7 2022, 7:29 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
102–105 ↗(On Diff #404297)

Ahh, hopefully once D118520 lands it should be easier to track down these bugs and create reproducers

JonasToth updated this revision to Diff 410179.Feb 20 2022, 1:08 PM
JonasToth marked 14 inline comments as done.
  • address reviews comments, part 1
  • Merge branch 'main' into feature_rebase_const_transform_20210808
  • fixing iterator-to-bool conversion and addressing other more review comments
JonasToth marked 3 inline comments as done.Feb 20 2022, 1:10 PM
JonasToth added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
33 ↗(On Diff #404297)

i verified that the check works with clangd without any additional build settings.

clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
29 ↗(On Diff #404297)

it is common to write custom matchers in clang-tidy first and use the internal matcher apis as well in clang-tidy.

$ cd llvm-project/clang-tools-extra/clang-tidy
$ git grep "internal::" | wc -l
86

Usually, they are extracted once they are generally useful. And many checks introduce custom matchers for better readability.
I can extract this matcher later if you want, but right now with the longevity of the patch and it being common practice in clang-tidy I am opposed to it.

29 ↗(On Diff #404297)

thank you for that hint! that would explain the issues indeed. I try to verify your proposed fix by re-transforming LLVM.
some other subscribers checked their own codebases as well. this would give us more confidence too.

if the crash is still there, I would like to keep the 'safety-if' in check and try to find the issue with the matcher.
The path @njames93 linked seemed like it would help there a lot.

84 ↗(On Diff #404297)

switching to forEachDescendant, both seem to work.
I am unsure why I used findAll, but I know that I started out with forEachDescendant for some bits of the check and it was insufficent.
But it could very well be, that these pieces are now in the ExprMutAnalyzer.

102–105 ↗(On Diff #404297)

I am rerunning the whole analysis on LLVM to see if the crash is still there after the proposed fix of sam (thanks!).
I will remove the code completely if the crash is gone. Otherwise I'd rather keep the safety in and try to fix the underlying issue afterwards.

JonasToth marked an inline comment as done.Feb 20 2022, 1:30 PM
JonasToth added a comment.EditedFeb 21 2022, 5:10 AM

Another full run on LLVM with:

$ ./clang-tools-extra/clang-tidy/tool/run-clang-tidy.py \
      -clang-tidy-binary ~/software/llvm-project/build_clang_tidy/bin/clang-tidy \
      -checks="-*,cppcoreguidelines-const-correctness" \
      -config "{CheckOptions: [{key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, {key: 'cppcoreguidelines-const-correctness.TransformReferences', value: 1}, {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: true}, {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: true}]}" \
      -fix 2>&1 | tee ../transformation.log

Yields to this: 3822 files changed, 159256 insertions(+), 159256 deletions(-)
and requires manual changes:


23 files changed, 45 insertions(+), 45 deletions(-)

the issues were:

  • missing {} initialization after a variable became const if a destructor was missing
  • the analysis got confused for some insert(llvm::Function* f) calls and made the objects const. Those are false positives, but with a really low rate

after applying the patch ninja check-all is successfull.

Branch Compare

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 11:42 PM
aaron.ballman resigned from this revision.Mar 21 2022, 9:43 AM

Sorry, for my own sanity, I've stopped reviewing C++ Core Guideline reviews for their diagnostic behavior until the guideline authors put effort into specifying realistic enforcement guidance.

Sorry, for my own sanity, I've stopped reviewing C++ Core Guideline reviews for their diagnostic behavior until the guideline authors put effort into specifying realistic enforcement guidance.

And how can we continue now? I fear that this is effectively the death of this check. :/
Isn't the "make this const" good enough?

Sorry, for my own sanity, I've stopped reviewing C++ Core Guideline reviews for their diagnostic behavior until the guideline authors put effort into specifying realistic enforcement guidance.

And how can we continue now? I fear that this is effectively the death of this check. :/
Isn't the "make this const" good enough?

Our rule of thumb in clang-tidy when checking against a coding standard is that the coding standard is the law as to how the check should work (what it diagnoses, what it doesn't diagnose). When the coding standard gives guidance that can reasonably be handled another way, we give config options so that users can pick the behavior they need to enforce. The trouble is: the C++ Core Guidelines rarely have a useful enforcement to suggest and the rules themselves are often not sufficiently precise to tease out what to enforce. This puts the onus on *us* to decide what the behavior should be, which is inappropriate unless the guideline authors are involved in the discussions and reflect the decisions in the guidelines. To date, that has (almost?) never happened with any C++ Core Guideline checks.

In this specific case, the guideline being checked is https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#es25-declare-an-object-const-or-constexpr-unless-you-want-to-modify-its-value-later-on and its enforcement guidance is:

Look to see if a variable is actually mutated, and flag it if not. Unfortunately, it might be impossible to detect when a non-const was not intended to vary (vs when it merely did not vary).

This is not useful enforcement guidance, so as a code reviewer, I have to spend *considerable* time trying to figure out what's reasonable and what's not. I've gone through this with basically each C++ Core Guideline check (many of them don't even TRY to give enforcement, like this: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#enforcement-345), so this is not at all specific to you or something that's your fault. But that's why I no longer am willing to review C++ Core Guideline checks; they're a burden to support without the guideline authors active engagement, which has never materialized.

So how can we continue now is definitely a good question. I think there's plenty of utility in this check already, and so my recommendation would be to pull out all mentions of the C++ Core Guidelines (so we don't have to stick specifically to what they recommend) and AUTOSAR (see https://reviews.llvm.org/D112730#3332366 for details) and move the check to misc- (I don't see a better module for it at the moment as it's not really bugprone and it's not really readability, but I'm not strongly tied to which module it lives in). From there, I hope we can converge on the check MUCH faster because I think the "make this const" is good enough (and SUPER USEFUL). However, I don't know what your goals or requirements are (if you need this to adhere to the C++ Core Guidelines specifically), so another option is for the other reviewers to sign off on it; I won't actively block the addition of new C++ Core Guideline checks.

(Personally, I'm of the opinion we should pull the C++ Core Guidelines modules out of clang-tidy and distribute the existing checks amongst the other modules. Then we no longer have to worry about what the guidelines say, we can use them purely as inspiration for things we feel may be useful to check, but without tying ourselves to supporting their document. However, that's a decision which requires a far wider audience and is way outside the scope of this check.)

I share @aaron.ballman option that given the flakiness of the guidelines, the check shouldn't mention that it addresses the c++core guidelines. Instead moving the check into another module(misc) would still provide immense value, with the side effect that it is still able to help teams enforce parts of the core guidelines.

ok. then i will continue under misc-const-correctness. thank you for the clarifications!

JonasToth updated this revision to Diff 424787.Apr 24 2022, 9:45 AM
  • refactor: rename check to 'misc-const-correctness' and adjust the tests accordingly
  • docs: adjust release notes and adjust check docs slightly
JonasToth retitled this revision from [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness to [clang-tidy] implement const-transformation for misc-const-correctness.Apr 24 2022, 9:46 AM
JonasToth updated this revision to Diff 424788.Apr 24 2022, 9:48 AM
  • fix: remove clangAnalysis link in cppcoreguidelines and add it in misc
JonasToth retitled this revision from [clang-tidy] implement const-transformation for misc-const-correctness to [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables.Apr 24 2022, 9:50 AM
JonasToth edited the summary of this revision. (Show Details)
njames93 added inline comments.Apr 24 2022, 11:12 PM
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
104

Hasn't this already been addressed, if so can this block just be removed.

clang-tools-extra/docs/ReleaseNotes.rst
103

Unrelated change.

clang-tools-extra/docs/clang-tidy/checks/list.rst
87

This should be committed separately.

clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-values.cpp
28

This check directive isn't going to be effective.
Try CHECK-FIXES: const int p_local0 = 2;
Same goes for all the ones below.

JonasToth updated this revision to Diff 427423.May 5 2022, 1:13 PM
JonasToth marked 3 inline comments as done.
  • addressing review comments and remove unrelated changes
JonasToth marked an inline comment as done.May 5 2022, 1:19 PM

Thank you for the review! I adjusted the patch.

JonasToth added inline comments.May 6 2022, 5:55 AM
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
25

missing change to new docs

JonasToth updated this revision to Diff 427629.May 6 2022, 7:06 AM
  • fix documentation reference

friendly ping :) (maybe @njames93 ?)

Pretty much good to go, just a few nits with the tests.
Can you add CHECK-FIXES directives for all warnings if there should be a fixit.
If there shouldn't be one could you either add a comment saying there shouldn't be one, or put a CHECK-FIXES-NOT directive.

JonasToth updated this revision to Diff 432774.May 29 2022, 2:48 AM
  • addded CHECK-FIXES in clang-tidy tests
  • merged latest main into branch

ping :)
@njames93 I added more CHECK-FIXES and CHECK-FIXES-NOT statements in the tests.

njames93 accepted this revision.Wed, Jun 8, 12:43 AM

LGTM, just a couple points but not essential.

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
184–187
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
35

It may be worth adding some validation to these. If AnalyzeValues, AnalyzeReferences and WarnPointersAsValues are all false, this whole check is basically a no-op.

This revision is now accepted and ready to land.Wed, Jun 8, 12:43 AM
JonasToth added inline comments.Wed, Jun 8, 6:18 AM
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
35

Whats the proper way to react? Short-circuit the registerMatchers, similar to language support?
I think thats the way I would go about this.

njames93 added inline comments.Wed, Jun 8, 10:23 AM
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
35

ClangTidyCheck::configurationDiag is the way to warn about issues with config.

LegalizeAdulthood requested changes to this revision.Wed, Jun 29, 8:30 AM

Clang-tidy tests and docs have been moved to subdirectories by module, please rebase to main:HEAD

This revision now requires changes to proceed.Wed, Jun 29, 8:30 AM