Page MenuHomePhabricator

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

Authored by vingeldal on Apr 4 2020, 7:45 AM.

Details

Summary

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

Diff Detail

Event Timeline

vingeldal created this revision.Apr 4 2020, 7:45 AM

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?

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.

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.

lebedev.ri retitled this revision from [clang-tidy] Remove false positive in AvoidNonConstGlobalVariables to [WIP][clang-tidy] Remove false positive in AvoidNonConstGlobalVariables.Apr 7 2020, 12:26 AM

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

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).

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

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.

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.

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.

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.

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

Ran automatic formatting on the patch

Harbormaster failed remote builds in B52970: Diff 257058!
aaron.ballman added inline comments.Apr 14 2020, 4:26 AM
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 updated this revision to Diff 257335.Apr 14 2020, 7:54 AM
vingeldal marked 3 inline comments as done.

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 added inline comments.Apr 14 2020, 8:17 AM
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 marked an inline comment as done.

Removed redundant condition in check

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

Right, sorry about that

aaron.ballman accepted this revision.Apr 15 2020, 4:15 AM

LGTM! Thank you!

This revision is now accepted and ready to land.Apr 15 2020, 4:15 AM

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