Page MenuHomePhabricator

[clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check
ClosedPublic

Authored by vingeldal on Nov 14 2019, 11:32 AM.

Details

Summary

Cpp Core Guideline I.2, a.k.a "Avoid non-const global variables"
For detailed documentation, see:
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#i2-avoid-non-const-global-variables

Diff Detail

Unit TestsFailed

TimeTest
100 msClang.clang-rename::Unknown Unit Message ("")
Script: -- : 'RUN: at line 20'; clang-rename -offset=62 -new-name=Bar /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/clang-rename/Ctor.cpp -- | sed 's,//.*,,' | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/clang-rename/Ctor.cpp
10 msLLVM.Bindings/Go::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; llvm-go test llvm.org/llvm/bindings/go/llvm

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
JonasToth added inline comments.Dec 21 2019, 3:22 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
33

The matchers here are not pointers/references. Therefore, the llvm guidelines say they should not be const.

65

each of those ifs should return early, or could multiple match?
If only one can match the structure could be changed a bit to

if (const auto* Variable = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
    diag(...);
    return;
}

If you change the order of the ifs in the order of matches you expect to happen most, this should be a bit faster as well, as retrieving the matches introduces some overhead that is not relevant in the current situation.

If you keep only one statement within the if you should ellide the braces, per coding convention.

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

The official name is "C++ Core Guidelines", i think we should stick to that.
And please ellide the new empty line above.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
7

Please add a link to the affected section and provide a small example (can be the example from the guidelines) what is diagnosed and why.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
47

diagnosing those might be undesired. maybe having an option to enable/disable this would be nice?
We should try to allow reducing the noise level of clang-tidy.

vingeldal marked 7 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Added an option to not check member variables.
  • Add checking for member variables referencing or pointing to non-const data.
  • Added some early returns in check implementation.
  • Expanded documentation, with an example and link to C++ Core Guidelines
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
65

There could be multiple matches but there could still be some early returns.
An example of a multi match would be:

namespace {
int i = 0;
}
int * i_ptr = &i;

There would be two warnings for i_ptr, since it is:

  1. A global non-const variable it self and...
  2. because it globally exposes the non-const variable i.

I'll add early returns where possible.

...Now that I think about it I realize I'v missed checking for member variables referencing or pointing to non-const data,
I'll add that tigether with some more testing.

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

I'll remove the empty line, I must ask about this formatting though.
I see multiple examples both with and without the empty line, so I just tried to be consistent with what most people seem to do.
What's the rule for knowing when to use one and when not to?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables.cpp
47

I can see your point and I agree that an option would be a good idea.
It isn't obvious that members variable could be considered global variables.
I'll make the current behavior the default though, since I got the impression
that in most aligned with the intent of the guideline.

aaron.ballman added inline comments.Dec 26 2019, 9:09 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

Why are you filtering out anonymous namespaces?

65

Based on my reading of the C++ core guideline, I think there should be a different two diagnostics. One for the declaration of i and one for the declaration of i_ptr, because of this from the rule:

The rule against global variables applies to namespace scope variables as well.

90

I'd sink the local variables into the if statement:

if (const auto *Var = Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
  ...
}

(If you don't want to wrap lines, you can make the string literals a bit shorter.)

107

pointed to -> pointed-to

121

Spurious semicolon

124–125

Can re-flow this string literal.

131

Spurious semicolon.

134

Can re-flow this string literal.

135

pointed to -> pointed-to

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
10

Add backticks around things that should be in code font like NoMembers.

45

Backticks here as well.

53

Rather than NoMembers, how about going with IgnoreDataMembers?

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-non-const-global-variables-NoMembers.cpp
25 ↗(On Diff #235176)

Please add a newline to the end of the file.

sylvestre.ledru added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
7

list.rst changed, you should update this!
Thanks

vingeldal updated this revision to Diff 235474.Dec 28 2019, 5:31 AM
vingeldal marked 12 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Updated list.rst
  • changed: "pointed to" -> "pointed-to"
  • CHanged option name: NoMembers -> IgnoreDataMembers
  • Moved matcher declaration into if-statement
  • Fixed some formatting issues
  • Removed forgotten FIXME-comments that were no longer relevant
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

If it's in an anonymous namespace it's no longer globally accessible, it's only accessible within the file it is declared.

65

If the variable i was in a named namespace it would be matched by a diagnostic, i_ptr is matched by another diagnostic for pointer providing global access to non-const data (as well as the matcher for global non-const variables)

124–125

Sorry, I don't understand what you mean by re-flow.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
53

Yes, that's much better, thanks.

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

I'm a bit concerned with this previous change of of list.rst.

Now that I need to add a check to this file, I don't know what severity to give it. I can't find any definition of severity levels and I really want to avoid getting into a long discussion, or having different reviewers not agreeing on my patch, concerning what severity we should give this check.
Is there any way I can find some kind of guideline on how to set the severity to avoid an opinionated discussion about severity level?

aaron.ballman added inline comments.Dec 28 2019, 6:09 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

It is, however, at namespace scope which is what the C++ Core Guideline suggests to diagnose. Do you have evidence that this is the interpretation the guideline authors were looking for (perhaps they would like to update their suggested guidance for diagnosing)?

There are two dependency issues to global variables -- one is surprising linkage interactions (where the extern nature of the symbol is an issue across module boundaries) and the other is surprising name lookup behavior within the TU (where the global nature of the symbol is an issue). e.g.,

namespace {
int ik;
}

void f() {
  for (int ij = 0; ik < 10; ij++) { // Oops, typo, but still compiles
  }
}

That's why I think the guideline purposefully does not exclude things like anonymous namespaces.

124–125

Sorry for not being clear -- the string literal spans three lines when it only needs to span two by re-writing the literal. e.g.,

"member variable %0 provides global access to non-const type, "
"consider making the pointed-to data const"
clang-tools-extra/docs/clang-tidy/checks/list.rst
7

I'd like to follow that discussion so that I can be consistent with my review advice, too.

vingeldal marked an inline comment as done.Dec 28 2019, 7:37 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
124–125

Ah, yes that's looks better of course. Will fix that in coming update.

vingeldal marked an inline comment as done.Dec 28 2019, 8:04 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

I don't have any evidence. To me the guideline still looks a bit draft-like, so I just tried my best guess as to the intent of the guideline.
In reading the guideline I get the impression that the intent is to avoid globally accessible data which is mutable,
mainly for two reason:

  • It's hard to reason about code where you are dependent upon state which can be changed from anywhere in the code.
  • There is a risk of race conditions with this kind of data.

Keeping the variable in an anonymous namespace seems to deals with the issues which I interpret to be the focus of this guideline.
Consider that the guideline is part of the interface section. If the variable is declared in an anonymous namespace there is no risk that a user of some service interface adds a dependency to that variable, since the variable will be a hidden part of the provider implementation.

Admittedly the wording doesn't mention an exception for anonymous namespaces, and the sentence you have referenced seems to suggest that any non-const variable in namespace scope should be matched.
I'm guessing though, that was intended to clarify that a variable is still global even if in a (named) namespace, because that would not have been an obvious interpretation otherwise.
Without the referenced sentence one might interpret only variables outside of namespace scope as global.
Arguably a variable in namespace scope isn't globally declared, though it is globally accessible, via the namespace name. I think the global accessibility is the reason for dragging in variables from namespace scope and if that is the case then we shouldn't also need to worry about anonymous namespaces.
This should probably be clarified in the actual guideline.

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

Good point!

I propose this patch to explain
https://reviews.llvm.org/D71963

this is a lazy copy and paste of
https://github.com/Ericsson/codechecker/blob/master/config/config.md

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

It should not be there. It won't be referenced on the list.

If you are uncomfortable setting a severity, leave it empty but it should clearly move above, not on the toctree

vingeldal updated this revision to Diff 235483.Dec 28 2019, 9:50 AM

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Adjusted the line breaks of some string literals.
  • Modified list.rst
vingeldal marked 2 inline comments as done.Dec 28 2019, 9:54 AM
vingeldal added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
7

Great, will make sure to contribute some feedback to that.

aaron.ballman added inline comments.Dec 28 2019, 12:45 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

This should probably be clarified in the actual guideline.

This sort of thing comes up with coding guidelines from time to time and the way we usually handle it is to ask the guideline authors for guidance. If they come back with an answer, then we go that route (while the authors fix the text) and if they don't come back with an answer, we muddle our way through. Would you mind filing an issue with the C++ Core Guideline folks to see if they can weigh in on the topic? If they don't respond in a timely fashion, I think it would make more sense to err on the side of caution and diagnose declarations within anonymous namespaces. This matches the current text from the core guideline, and we can always relax the rule if we find it causes a lot of heartache in the wild. (If you have data about how often this particular aspect of the check triggers on large real-world code bases, that could help us make a decision too.)

vingeldal marked an inline comment as done.Dec 29 2019, 2:59 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

I sent a pull request to the guide lines, suggesting a clarification. If that is addressed in the near future I guess we can go on what they say about the pull request. If it takes to long I'll just modify the code to warn in anonymous namespace as well.
https://github.com/isocpp/CppCoreGuidelines/pull/1553

aaron.ballman added inline comments.Dec 29 2019, 11:51 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
30

Thank you, I think that approach makes sense.

JonasToth added inline comments.Dec 30 2019, 6:04 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

missing return?

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
7

I would prefer if the I.2 of C++ Core Guidelines is a link and to remove the link-string below this paragraph.

30

This is technically not valid code, as the pointer needs to be initialized and a ; is missing. I think that should be fixed to not mislead inexperienced programmers.

vingeldal updated this revision to Diff 235600.Dec 30 2019, 6:59 AM
vingeldal marked 2 inline comments as done.

Updating D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

  • Fixed code example in documentation to make it valid code.
  • Fixed the documentation link to I.2 C++ Core Guidelines.
vingeldal marked an inline comment as done.Dec 30 2019, 7:03 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

No, one of the below matchers may also be valid, in case we have a non-const member variable which is also a pointer or reference to non-const data.

There are many comments not marked as done. could you please do that with addressed issues? It is hard to otherwise see whats still left.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
111

I see. Please add a FALLTHROUGH comment or the like, as it breaks the pattern a lot and is not obvious from local code.

vingeldal marked 10 inline comments as done.Dec 30 2019, 8:08 AM
vingeldal marked 2 inline comments as done.
vingeldal marked an inline comment as done.Jan 6 2020, 1:53 AM
vingeldal added inline comments.
clang-tools-extra/docs/clang-tidy/checks/list.rst
7

It seems to me like this list is still generated the old way if one uses add_new_check.py to add a new check.
@sylvestre.ledru could you please also adapt that script to generate the table instead of the list?

lebedev.ri retitled this revision from [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines to [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check.Jan 6 2020, 2:03 AM
vingeldal updated this revision to Diff 243757.Feb 11 2020, 1:07 AM

Updating D70265: [clang-tidy] Add CppCoreGuidelines I.2 "Avoid non-const global variables" check

Created the requested pull request to C++ Core guidelines and since it is now merged this patch is updated accordingly.

  • Anonymous namespaces are no longer ignored by the check as guideline editors explained that was not wanted.
  • The option to ignore data members was reomved, they are now always ignored.
  • Documentation was updated to mention that this check also covers C++ COre Guideline R.6, since R.6 is simply a duplication of I.2.

The pull request to C++ Core guidelines have been merged: https://github.com/isocpp/CppCoreGuidelines/pull/1553

Herb set me straight regarding the anonymous namespaces so I had to change that.
While waiting for the pull request to be reviewed I looked at more guidelines and also changed my mind about optionally ignoring data members, so I removed that option and made this check always ignore data members.
Data members are probably better dealt with in separate rules after all.

Sorry for the rather significant changes coming in this late when you all have already put so much time into reviewing, at least the check changed to be smaller in scope -which felt like the right way to go.

aaron.ballman added inline comments.Mar 6 2020, 7:07 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
56–72

I think these cases should be combined to avoid duplication.

if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("whatever")) {
  diag(VD->getLocation(), "variable %0 provides global access to a non-const object; considering making the %select{referenced|pointed-to}1 data 'const'") << VD << VD->getType()->isReferenceType();
  return;
}

the matcher needs to be changed to bind to the same id for both cases.

Note, this rewords the diagnostic slightly to avoid type/object confusion (the variable provides access to an object of a non-const type, not to the type itself).

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
21

This isn't legal code.

34

This isn't legal code either.

You may want to run the example through the compiler to catch these sort of things.

vingeldal marked an inline comment as done.Mar 10 2020, 11:02 AM
vingeldal added inline comments.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
56–72

You mean just the pointer and reference cases right? That matcher seems to get much more complicated, I'm having some trouble accomplishing that. Are you sure that's necessary? What would the cases of duplication be?

aaron.ballman added inline comments.Mar 10 2020, 1:21 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
56–72

You mean just the pointer and reference cases right?

Yup!

Are you sure that's necessary? What would the cases of duplication be?

Not strictly necessary, so if this turns out to be hard, I'm fine skipping it. However, I was thinking it would be something like:

// Registering checks
Finder->addMatcher(GlobalReferenceToNonConst.bind("qual-non-const"), this);
Finder->addMatcher(GlobalPointerToNonConst.bind("qual-non-const"), this);

// In check
if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("qual-non-const")) {
  diag(VD->getLocation(),
          "variable %0 provides global access to a non-const object; considering making the %select{pointed-to|referenced}1 data 'const'") << VD << VD->getType()->isReferenceType();
  return;
  }
vingeldal marked 6 inline comments as done.

Made example code in documentation legal C++

Changed implementation according to comments so that both the pointer matcher
and the reference matcher now bind to the same id

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp
56–72

Oh, I didn't understand I could just keep the two matchers and use the same binding string. I should have read more carefully.
Sure I'll do that, its no effort and it looks nicer.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-non-const-global-variables.rst
34

Oops, will make sure and do that next time to save us all some time and me some embarrassment.

aaron.ballman accepted this revision.Mar 12 2020, 2:25 PM

LGTM! Do you need someone to commit on your behalf?

This revision is now accepted and ready to land.Mar 12 2020, 2:25 PM

LGTM! Do you need someone to commit on your behalf?

Yes please.
Do I need to, or can I, do anything about the failing builds (https://reviews.llvm.org/B49012) first? They seem unrelated to my change but I'm not sure what caused that.

aaron.ballman closed this revision.Mar 13 2020, 7:07 AM

LGTM! Do you need someone to commit on your behalf?

Yes please.

Happy to do so -- I've commit on your behalf in 512767eb3fe9c34c655a480d034147c54f1d4f85. Thank you for the new check!

Do I need to, or can I, do anything about the failing builds (https://reviews.llvm.org/B49012) first? They seem unrelated to my change but I'm not sure what caused that.

They're unrelated to your change, so there's nothing for you to do.

Hello.
2 points:

rG512767eb3fe9c34c655a480d034147c54f1d4f85 doesn't reference this review,
so it's a bit hard to find. Please try to reference it next time.

Is this check really intentionally complaining about function-scope static variables?
How are they global?
https://godbolt.org/z/2QMemL

void bar(int*);

void foo() {
  static int a;
  bar(&a);
}
<source>:4:14: warning: variable 'a' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
  static int a;
             ^

Hello.
2 points:

rG512767eb3fe9c34c655a480d034147c54f1d4f85 doesn't reference this review,
so it's a bit hard to find. Please try to reference it next time.

Will do

Is this check really intentionally complaining about function-scope static variables?
How are they global?
https://godbolt.org/z/2QMemL

void bar(int*);

void foo() {
  static int a;
  bar(&a);
}
<source>:4:14: warning: variable 'a' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
  static int a;
             ^

I think that's a false positive, I think it's caught because the variable is static, I'll submit a patch where that doesn't happen ASAP.

After looking more closely at the code I think the issue is within hasLocalStorage() which is called in hasGlobalStorage(). My expectation would be that anything inside of function scope would be considered local but I'm not very certain.
Any thoughts on whether hasLocalStorage() should be modified or if I should change the check and use some more ad-hoc implementation, instead of hasGlobalStorage(), to determine if the variable is local or global?

After looking more closely at the code I think the issue is within hasLocalStorage() which is called in hasGlobalStorage(). My expectation would be that anything inside of function scope would be considered local but I'm not very certain.
Any thoughts on whether hasLocalStorage() should be modified or if I should change the check and use some more ad-hoc implementation, instead of hasGlobalStorage(), to determine if the variable is local or global?

It's not ideal to have known-false-positive for long, so please post some fix, i think we'll figure it out there.

After looking more closely at the code I think the issue is within hasLocalStorage() which is called in hasGlobalStorage(). My expectation would be that anything inside of function scope would be considered local but I'm not very certain.
Any thoughts on whether hasLocalStorage() should be modified or if I should change the check and use some more ad-hoc implementation, instead of hasGlobalStorage(), to determine if the variable is local or global?

Not everything at function scope has local storage -- for instance, a variable declared with static or extern at local scope would not have local storage (storage != scope). isLocalVarDeclOrParm() or isLocalVarDecl() may help you out here.