[PATCH] D115106: [clang-tidy] Fix `readability-static-accessed-through-instance` false negative for static methods

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: sammccall, klimek, hokein.
aaron.ballman added a comment.

Adding some more reviewers, as this is starting to touch on AST matcher design more deeply and needs a wider audience.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:6017
+/// cxxMethodDecl(isStatic()) matches A::foo() but not A::bar()
+AST_MATCHER(CXXMethodDecl, isStatic) { return Node.isStatic(); }
+
----------------
simon.giesecke wrote:
> aaron.ballman wrote:
> > I would prefer we not expose this AST matcher as a public one. There's sufficient confusion around "as written in source" vs "computed" vs "linkage" in this area that I think we should be careful when introducing matchers around storage classes and storage durations. Then again, the water is already pretty muddy here, so maybe this ship sailed...
> > 
> > Other potential solutions to this issue would be to expose an AST matcher to traverse to the canonical decl or only matches the canonical decl, then we could use that to wrap the existing call to `isStaticStorageClass()`. (Of course, we could make this matcher local to just that source file, as well.)
> > 
> > Thoughts?
> I think if we have a `isStaticStorageClass` matcher, then we can also have `isStatic` matcher.
> 
> But maybe we shouldn't have either of those in the public header, but maybe add a different one as you suggested, under a suitable name with as little ambiguity as possible.
> 
> And maybe keep the `isStaticStorageClass` matcher but rename it to reduce the chance of misunderstanding?
> 
> Then, as mentioned in my original top-level comment, I can imagine that there are more uses of the `isStaticStorageClass` matcher in clang-tidy rules (and maybe elsewhere) that actually should use the new matcher.
My current thinking on this is that I'm not opposed to exposing the functionality, but I have Opinions about the names of how we expose these and what they model. There are quite a few ways to think about "is static" (with some overlap between them):

1) Was this declaration marked with the `static` keyword?
2) Does this declaration have static storage duration? http://eel.is/c++draft/basic.stc.static#def:storage_duration,static
3) Does this declaration have internal linkage? http://eel.is/c++draft/basic.link#3.1
4) Does this declaration have a static array extent? C2x 6.7.6.3p6
5) Does the definition a member function have access to a `this` pointer/does the definition of a data member contribute to the subobjects of the class? http://eel.is/c++draft/class.static
6) All of the above questions, but instead of specific to one declaration, is about all (re)declarations of the entity.

I don't think we want to cover #4 as part of this patch (that's more about types and less about declarations), and I think #6 is a general matcher question.

FWIW, we currently have matchers for:

`isStaticLocal`
`hasStaticStorageDuration`
`isStaticStorageClass`

So I don't think `isStatic` is viable -- it's just too ambiguous given all the many uses of the keyword and the existing set of matchers.

My intuition is that we might want a new kind of traversal matcher for matching over a set of redeclarations. e.g., `functionDecl(isStaticStorageClass())` vs `anyRedeclaration(functionDecl(isStaticStorageClass()))`:
```
static void func(); // #1
void func(); // #2
void func() {} // #3
```
where `functionDecl(isStaticStorageClass())` would match #1 and `anyRedeclaration(functionDecl(isStaticStorageClass))` would match #1, #2, and #3. I think if we had this sort of facility, we could probably make use of our existing matchers (perhaps with improved names) to achieve the same goal as you're going for, but I'm curious what other folks think as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115106/new/

https://reviews.llvm.org/D115106