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

> In D115106#3218000 <https://reviews.llvm.org/D115106#3218000>, @aaron.ballman wrote:
>
>>> 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?
>
> Use, if I'm understanding your question.
> If we don't expose this as a matcher with some name, it's a recipe the person writing the matcher has to remember.
> There's no good way to look it up, and it's easy to write something that seems good but isn't.
>
> (This problem is pervasive with AST and matchers, and this isn't the worst example.)

Ah, I see what you're driving at better now, thank you!

>>> 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).
>
> The clang AST is the best tool we have to describe syntax *and* to describe semantics.
> Maybe by design it captures "all" syntax and "enough" semantics, but users reasonably care less about the design than about their alternatives and problems. The clang AST is useful for answering syntax questions (better than regex!) but it's _irreplaceable_ for answering semantics questions (better than... err?).
>
> So we have syntax and semantics in one namespace, and static is the worst because it means like 3 syntactic and 5 semantic things :-(

Heh, those are fair points. I especially appreciate the clarity about whether we're matching syntax or semantics, that's really the crux of my concerns when you distill it.

>> 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.
>
> Definitely, this highlights the disconnect really well.
> And some of the accessors on a redecl will delegate to the canonical while others won't. (Can we call this "syntax semantics" vs "semantics semantics"?!)
>
> I definitely think we *should* have a redecl matcher - I just don't think we should make users use it to answer common semantic questions.
> That leaves matcher authors remembering and reciting the rules about how syntax aggregates over redecls to produce semantics. We'll make mistakes.

Ahhhh, okay, I thought you were opposed to the notion entirely. I agree with you that we need to be very careful not to add undue user burdens for common semantic questions!

>>> 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.
>
> Oh, I totally agree. I just meant specifically for this case, as a way to avoid the dilemma of ambiguous vs unwieldy.

Now that I understand better, +1 -- that would unblock the needs for this patch without having to solve the larger problem, which is great.

>> 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
>
> I'm not totally surprised by that, because "storage duration" sounds like a semantic property to me, while "storage class" (specifier) is a syntactic feature.
> But it's not a distinction I'd pick up on if I wasn't looking for it, and different people are going to read things different ways.
>
> My favorite naming convention I've seen in the AST is e.g. `getBlob()` vs `getBlobAsWritten()`.
> Maybe `getEffectiveBlob()` would be even clearer for the semantic version.
> But I'm not sure how much it's possible to realign matcher names around that sort of thing if the AST names aren't going to change.

Yeah, there's definitely some tension between our desire for AST matchers to use the same terminology as used by the AST itself and picking names that are usable and clear for users not steeped in compiler internals lore. Personally, I am very comfortable with the idea of renaming some of the AST functionality within Clang in this area (at least regarding storage duration, linkage, etc) because this problem comes up outside of AST matchers as well. It's an NFC change and there will be some churn from it, but I think the churn is well motivated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115106