C++: Add missing parent scope cases

[jketema]

This is mostly used to detect the shadowing of variable declarations.

DCA shows no changes. This does break some coding standards tests. I'll update the test results there after this is merged, and open an issue that informs the coding standards people of this. I briefly looked at fixing the query, but (a) it's not clearly documented what the query should do, and (b) this requires digging through about 400 lines of query code, which I find very hard to follow.

Pull Request checklist

All query authors

• A change note is added if necessary. See the documentation in this repository.
• All new queries have appropriate .qhelp. See the documentation in this repository.
• QL tests are added if necessary. See Testing custom queries in the GitHub documentation.
• New and changed queries have correct query metadata. See the documentation in this repository.

Internal query authors only

• Autofixes generated based on these changes are valid, only needed if this PR makes significant changes to .ql, .qll, or .qhelp files. See the documentation (internal access required).
• Changes are validated at scale (internal access required).
• Adding a new query? Consider also adding the query to autofix.

[geoffw0]

It feels like the parent of e should be something associated with the catch block on line 11, rather than the entire function. I can see that this wouldn't fit the current definition of what getParentScope can return. And what you have is probably good enough for what getParentScope is used for.

[jketema]

Yeah, agreed. I struggled a bit with that one initially. What I do now seems to be most in line with all the other cases that are there, and what we want when looking for shadowing.

[geoffw0]

I briefly looked at fixing the query, but (a) it's not clearly documented what the query should do, and (b) this requires digging through about 400 lines of query code, which I find very hard to follow.

Which query is this?

[jketema]

This query: https://github.com/github/codeql-coding-standards/blob/b476450bfb3bf1801c96159580ec66a577ead214/cpp/common/src/codingstandards/cpp/rules/identifierhidden/IdentifierHidden.qll and the associated Scope library: https://github.com/github/codeql-coding-standards/blob/b476450bfb3bf1801c96159580ec66a577ead214/cpp/common/src/codingstandards/cpp/Scope.qll

[geoffw0]

Yep, discussing the intent with one of the authors might help to untangle what's going on there.

[jketema]

I've reported the issue on the coding standards repo. I assume they'll have a look once they upgrade to CodeQL 2.19.3 or later.

[lcartey]

@jketema Could you explain the rationale behind making the parent scope of a catch block parameter the parent scope of the TryStmt? Surely it should be the catch block itself?

This is what is breaking the Coding Standards IdentifierHiding.ql query. That query tries to find pairs of identifiers where the second hides the first. By considering to the parameter of a catch block to have a parent scope of the parent of the TryStmt we erroneously report cases like this:

  {
    int i; // COMPLIANT
  }

  try {

  } catch (int i) { // COMPLIANT
  }

[jketema]

Could you explain the rationale behind making the parent scope of a catch block parameter the parent scope of the TryStmt? Surely it should be the catch block itself?

This seemed to be most consistent with the other cases here. For example, for functions the parent scope is the whole function, and not the statement block that forms the body of the function. The latter would correspond with the catch block in the case of a try-catch.

[lcartey]

For example, for functions the parent scope is the whole function

Just checking, but I presume you mean "for parameters the parent scope is the whole function"?

If so, I think it would be reasonable on that basis to report the Handler of the CatchBlock as the parent scope of the parameter. But reporting the parent of the TryStmt as the "scope" erroneously puts it in the same scope as other variables declared outside the TryStmt. Given the main purpose of this predicate is to support identifier hiding queries, this seems like it's not ideal.

[jketema]

The problem is that Handler is not exposed anywhere else in the QL API, and it's only really visible in the AST, so that's far from ideal either.