[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#3217968 <https://reviews.llvm.org/D115106#3217968>, @sammccall wrote:

> Yeah, this is a thorny question. The language hasn't chosen clear names and neither has the AST in many cases.
> (Just yesterday I lost a lot of time to CXXRecordDecl->getBodyRBrace()...)

Yeah, I've run into this problem with attribute subject lists as well, so it's definitely an AST-level problem with how we expose the information in the AST.

> I think `anyRedeclaration(functionDecl(isStaticStorageClass))`is too difficult to get right and too easy to get wrong for such a common/simple concept.

Get right in terms of implementation, or in terms of use, or both?

> I know what a static method is, but I would have made the same mistake here. In general asking people to describe syntax to answer a semantic question invites this.

Because our AST is intended to reflect the syntax, our AST matchers kind of *do* match syntax -- I think it's the reason we're in the problem (and this is not the first time it's come up). There's a cognitive disconnect between the AST matchers traversing the AST and applying a match on all node and the C & C++ concept of redeclarations. People writing the matchers are thinking exactly what you said -- I know what a static method is -- but they don't realize they get this behavior with the most straight-forward matching code (https://godbolt.org/z/rWW48MT8T):

  static void f(); // Get a match for this
  void f() {} // But not this, despite it being static



> Here's a hack: CXXMethodDecl has both `isStatic()` and `isInstance()`. I agree that `isStatic` is a risky name, but I don't think `isInstance` is. Can we just spell the matcher you want `not(isInstance())`?

This could work, but I don't think it's particularly satisfying in situations that aren't binary. Like asking "does this have static storage duration" (as opposed to thread, automatic, or dynamic storage duration). And it turns out that the behavior there is opposite to asking about the storage class specifier: https://godbolt.org/z/hqserfxzs (the `extern` redeclaration is still matched because that declaration retains the internal linkage from the first declaration). So we sometimes take prior declarations into account and we sometimes don't. :-(


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115106