[WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables

[vingeldal]

Summary

There was a post merge comment about a possible false negative for this check:
https://reviews.llvm.org/D70265

[vingeldal]

After looking in to it I got less certain of where the error lies and eventually I got uncertain if this even is a false negative, so I started out with just posting a change where the unit test is updated to detect the issue.
This is just to have a start for a discussion of how this should actually work and what is the best way forward.

The purpose of the rule is to avoid code which causes hidden dependencies. The given example of a potential false positive is a free function with a static variable.
Now, wouldn't a static variable inside a function body make the function stateful thereby risking that the function gives different results for the same input, in ways that might look arbitrary or random to the caller.
I think that might actually be a good example of what the rule is meant to prevent so maybe this isn't a false positive after all?

[gribozavr2]

I expect different style guides to have a different opinion on this, depending on the justification for the rule.

The purpose of the rule is to avoid code which causes hidden dependencies.

That's one justification, which would prohibit such static variables.

Another justification is to avoid "spooky action-at-a-distance" (one piece of code communicating with another piece of code through a global variable), which would allow such static variables, because they are encapsulated.

[lebedev.ri]

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

[vingeldal]

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

It is arguably unconventional in C++ to make a free function statefull, if one wants to make a function stateful there is the obvious alternative of making it a member function of a class -which would allow you to achieve the same thing but with more explicitly expressed statefullness in the interface and stronger encapsulation of the state.

Not sure if it makes any difference but note that this check also covers rule R.6 which is the exact same rule but in a different context.
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rr-global

[aaron.ballman]

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

[lebedev.ri]

FWIW i've posted https://github.com/isocpp/CppCoreGuidelines/issues/1599 asking for clarification, but no replies so far..

[vingeldal]

In D77461#1977639, @aaron.ballman wrote:

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

A caller of this function will clearly be affected by other callers if there are any, so there clearly is a dependency added between callers of this function. At the same time the function doesn't provide any hints toward the fact that it is statefull or any way for the caller achieve observability of this state, hence the state causing the dependency is hidden and the dependency is hidden.

I must agree that the enforcement section goes against my interpretation but I would prefer to try to simply change the enforcement section.

This function with a static variable can almost always be replaced with a solution where the free function is instead made a member function of a class keeping the state as a non-static, private member variable -which would more clearly convey that the function may have state and would provide stronger encapsulation.
The only exception would be if we are talking about a C-interface, in which case one could still use a class, as I just described, instead of a static variable; you would just have to call the member function via a free function.
Can anyone explain to me why one would ever *have* to rely on functions with static variables? Because before I can see a legitimate use case for this potential false positive I think it would be a bad idea to consider it a false positive since there are obvious issues with hidden dependencies in the discussed example code.

[vingeldal]

I'm also adding to this discussion the exception-part of the rule:
"A global object is often better than a singleton."

I would argue that the example given above, while not strictly follwong the definition of a singleton, is behaviorally consistent with a singleton.

[aaron.ballman]

In D77461#1977687, @vingeldal wrote:

In D77461#1977639, @aaron.ballman wrote:

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

A caller of this function will clearly be affected by other callers if there are any, so there clearly is a dependency added between callers of this function. At the same time the function doesn't provide any hints toward the fact that it is statefull or any way for the caller achieve observability of this state, hence the state causing the dependency is hidden and the dependency is hidden.

I must agree that the enforcement section goes against my interpretation but I would prefer to try to simply change the enforcement section.

This function with a static variable can almost always be replaced with a solution where the free function is instead made a member function of a class keeping the state as a non-static, private member variable -which would more clearly convey that the function may have state and would provide stronger encapsulation.
The only exception would be if we are talking about a C-interface, in which case one could still use a class, as I just described, instead of a static variable; you would just have to call the member function via a free function.
Can anyone explain to me why one would ever *have* to rely on functions with static variables?

To avoid the static initialization order fiasco, statistics or timing counters, etc -- look through the LLVM code base for function local statics, they're not uncommon.

Because before I can see a legitimate use case for this potential false positive I think it would be a bad idea to consider it a false positive since there are obvious issues with hidden dependencies in the discussed example code.

If we don't hear back from the C++ Core Guidelines authors quickly on their thoughts or adjustments, there are at least two reviewers who consider it a false positive based on the current rule text, which is sufficient post-review feedback to warrant a change IMO.

[vingeldal]

In D77461#1977703, @aaron.ballman wrote:

In D77461#1977687, @vingeldal wrote:

In D77461#1977639, @aaron.ballman wrote:

In D77461#1977503, @vingeldal wrote:

In D77461#1963166, @lebedev.ri wrote:

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-global is in "Interfaces" section, it only covers inter-procedural stuff.
Diagnosing function-local static non-const variable is a plain false-positive diagnostic.

I don't follow your train of thought. Could you please elaborate on why you think this must be a false positive?

I think it must be a false positive because the rule is about global variables where "global" refers to the scope of the variable. This is a locally scoped variable. Also, the rule's enforcement section is pretty clear that this does not apply to local statics.

My reason for hesitating to call this a false positive is that this pattern does cause a hidden dependency between users of the function, hence it clearly goes against the short and simple rationale given for this rule:
"Non-const global variables hide dependencies and make the dependencies subject to unpredictable changes."

I don't see it hiding a dependency between users of the function. The local static could be swapped out for something else (an actual global, linker magic, etc) and the caller would be unaware. The same is not true for a globally scoped object where the identifier is exposed because someone else could be trying to link to that symbol (and they would break if the symbol changed).

A caller of this function will clearly be affected by other callers if there are any, so there clearly is a dependency added between callers of this function. At the same time the function doesn't provide any hints toward the fact that it is statefull or any way for the caller achieve observability of this state, hence the state causing the dependency is hidden and the dependency is hidden.

I must agree that the enforcement section goes against my interpretation but I would prefer to try to simply change the enforcement section.

This function with a static variable can almost always be replaced with a solution where the free function is instead made a member function of a class keeping the state as a non-static, private member variable -which would more clearly convey that the function may have state and would provide stronger encapsulation.
The only exception would be if we are talking about a C-interface, in which case one could still use a class, as I just described, instead of a static variable; you would just have to call the member function via a free function.
Can anyone explain to me why one would ever *have* to rely on functions with static variables?

To avoid the static initialization order fiasco, statistics or timing counters, etc -- look through the LLVM code base for function local statics, they're not uncommon.

Because before I can see a legitimate use case for this potential false positive I think it would be a bad idea to consider it a false positive since there are obvious issues with hidden dependencies in the discussed example code.

If we don't hear back from the C++ Core Guidelines authors quickly on their thoughts or adjustments, there are at least two reviewers who consider it a false positive based on the current rule text, which is sufficient post-review feedback to warrant a change IMO.

You can also avoid static initialization order fiasco by not using static as much, but fine, I'll just do it so I don't have to think about this discussion again.
Before I do that I'm just adding to the record that if there was an implementation for rule F.8 this example would definitely been reported by that check instead so it's clearly something to avoid according to the C++ Core Guidelines.

[vingeldal]

Modified the check to not report static variables in function scope.

[vingeldal]

Ran automatic formatting on the patch

[aaron.ballman]

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

58

I think this should be handled in the matcher rather than here. I'd add a local matcher for isLocalVarDecl() and add it to the unless() matcher. WDYT?

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

234–236

I would like this example moved out of the section suggesting this is a false positive.

[vingeldal]

Moved previously added condition into the actual matcher.

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

234–236

I don't follow I thought you said this was a false positive and that's why I'm now making the check not catch this piece of code?

[aaron.ballman]

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

58

You should be able to remove this change now.

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

234–236

I think I just misread the comment as the opposite (false negatives), sorry for the noise!

[vingeldal]

Removed redundant condition in check

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

58

Right, sorry about that

[aaron.ballman]

LGTM! Thank you!

[lebedev.ri]

Thanks!

[aaron.ballman]

I've commit on your behalf in b2d8c89ea48beb83e0392b1f00c9eafa33c09ca8, thank you for the patch!

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

 #include "clang/ASTMatchers/ASTMatchers.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
+namespace {
+AST_MATCHER(VarDecl, isLocalVarDecl) { return Node.isLocalVarDecl(); }
+} // namespace
+
 void AvoidNonConstGlobalVariablesCheck::registerMatchers(MatchFinder *Finder) {
   auto GlobalVariable = varDecl(
       hasGlobalStorage(),
       unless(anyOf(
-          isConstexpr(), hasType(isConstQualified()),
+          isLocalVarDecl(), isConstexpr(), hasType(isConstQualified()),
           hasType(referenceType())))); // References can't be changed, only the
                                        // data they reference can be changed.
 
   auto GlobalReferenceToNonConst =
       varDecl(hasGlobalStorage(), hasType(referenceType()),
               unless(hasType(references(qualType(isConstQualified())))));
 
   auto GlobalPointerToNonConst =
void AvoidNonConstGlobalVariablesCheck::check(
   if (const auto *Variable =
           Result.Nodes.getNodeAs<VarDecl>("non-const_variable")) {
     diag(Variable->getLocation(), "variable %0 is non-const and globally "
                                   "accessible, consider making it const")
         << Variable; // FIXME: Add fix-it hint to Variable
     // Don't return early, a non-const variable may also be a pointer or
     // reference to non-const data.
   }
 
   if (const auto *VD =
           Result.Nodes.getNodeAs<VarDecl>("indirection_to_non-const")) {
     diag(VD->getLocation(),
          "variable %0 provides global access to a non-const object; consider "
          "making the %select{referenced|pointed-to}1 data 'const'")
         << VD
         << VD->getType()->isPointerType(); // FIXME: Add fix-it hint to Variable
   }
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

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

 
 // CHECKING FOR NON-CONST GLOBAL TEMPLATE VARIABLE ////////////////////////////
 template <class T>
 constexpr T templateVariable = T(0L);
 
 // CHECKING AGAINST FALSE POSITIVES INSIDE FUNCTION SCOPE /////////////////////
 int main() {
   for (int i = 0; i < 3; ++i) {
+    static int staticNonConstLoopVariable = 42;
     int nonConstLoopVariable = 42;
-    nonConstInt = nonConstLoopVariable + i;
+    nonConstInt = nonConstLoopVariable + i + staticNonConstLoopVariable;
   }
 }