[clang] [clang][StaticAnalyzer] Add support for variables and fields in GetEnclosingDeclContextSignature (PR #214102)

[Donát Nagy via cfe-commits]

NagyDonat wrote:

> > This commit uses `setDeclWithIssue` in a way that is not consistent with the intended meaning and other usage of this method. Its (admittedly a bit vague, but still understandable) doc-comment says that its role is
> > ```c++
> > // Specifically set the Decl where an issue occurred. This isn't necessary
> > // for BugReports that cover a path as it will be automatically inferred. 
> > ```
> 
> This is true for PathSensitive issues.

@steakhal Note that I copied the quoted comment from the method of the non-path-sensitive `BugReport` class.

> However, webkit checkers piggyback on the CSA infra but only use the `runCheckersOnASTDecl` parts. This is where is gets tricky because there is no good decl CSA could set the `DeclWithIssue` to.

Yes, the case of reports generated from `runCheckersOnASTDecl` is troublesome because there we don't have a good automatic solution.

For path-sensitive reports "This [=calling `setDeclWithIssue`] isn't necessary" because there the analyzer can automatically set a good `DeclWithIssue`. On the other hand, for non-path-sensitive reports (reports that do not "cover a path"), I think the unsaid implication of the doc-comment is that `setDeclWithIssue` must be called manually because (as you nicely explain) the engine cannot select a good `Decl` automatically.

> [...] For any other Decl. This is where we should set the `DeclWithIssue` to the decl we visit. This is not done. This will likely use the TUDecl it set when visiting the TUDecl. This is correct, but likely we should set the Decl we pass to the callback.

I also suspect that it would be probably better to set `DeclWithIssue` to a more concrete decl when we visit it. If we think that this is important, an experienced contributor should inspect the implications of this (what kinds of `Decl`s can appear instead of the TUDecl, are we prepared to handle those, how does this tweak impact the result set on real-world projects etc.). As the status quo seems to be mostly stable, I'm not convinced that this is worth the required effort (there is a code smell, but nothing is on fire, unlike in other areas of the analyzer).

> So ultimately checkers have 2 options: [... use the `DeclWithIssue` from the callback or pick their own `Decl` manually]

I agree, and I agree that these two options are valid.

> So all in all, I don't see any problem here.

@steakhal The problem (which is already fixed by @ziqingluo-90) was that the original version of the PR (which was swept away by a force push) passed "small" decls (fields, local variables etc.) to `setDeclWithIssue` and extended `GetEnclosingDeclContextSignature` with the counter-intuitive behavior that when `DeclWithIssue` is-a such "small" desc, then the hash will include the qualified name [or signature] of the _decl that encloses `DeclWithIssue`_ (which breaks the usual pattern that the hash includes the qualname or signature of `DeclWithIssue` itself).

https://github.com/llvm/llvm-project/pull/214102