[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 a comment.

In D115106#3172949 <https://reviews.llvm.org/D115106#3172949>, @simon.giesecke wrote:

> Thanks a lot for addressing this issue! I am just trying it on our codebase.
>
>> The problem actually has nothing to do with the out-of-line definition being inline; the problem is that hasDeclaration() of the memberExpr() will match the out-of-line definition, which obviously isn't marked static, so isStaticStorageClass() won't match.
>
> Hm, an out-of-line definition *cannot* have the `static` keyword. I wonder if it's actually a bug (in the AST? or just the matcher?) that `isStaticStorageClass` doesn't match here? I guess that other checks that use `isStaticStorageClass` might be affected by this too?

`isStaticStorageClass()` calls through to `FunctionDecl::getStorageClass()` which reports the storage as written on that declaration in source. So this isn't a bug in the AST, it's a more of a misunderstanding of whether we're querying the storage duration/linkage for the method or whether we're querying whether it was written with the static keyword.



================
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(); }
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115106