QL-for-QL: Add a could-be-cast query

[erik-krogh]

Inspired by #5160

I haven't fixed the issues identified by this query.

---

This query identifies exists(..)/any(..) that can be simplified to a type-cast:
exists(Type t | t = foo()) can be simplified to foo() instanceof Type.
And any(Type t | t = foo()) can be simplified to foo().(Type).

[aschackmull]

I disagree with simplifying the exists case in general. There are many cases where it's nice to name an intermediate result to make code more readable. Changing a single-use exists-variable to an instanceof is probably a good thing, though.

[aschackmull]

The exists case catches too much.

[erik-krogh]

The exists case catches too much.

I see. Yes, a decent amount of those warning are better as is.

I've done two things:

1: Change the severity to recommendation.

2: Restrict the flagged exists clauses to only those where the implicit type-cast is also redundant.
E.g. this code which can be changed like:

// old
exists(State state | state = getAState(this, str.length() - 1, str, _) |
  epsilonSucc*(state) = Accept(_)
)
// refactored 
epsilonSucc*(getAState(this, str.length() - 1, str, _)) = Accept(_)

[aschackmull]

Note: an exists(..) is actually a qualifier and not an aggregate, but I'm unsure what else to call something that is either an exists(..) or an any(..).

I think it's fine to just call it an "exists" in the query name, and then use either "exists" or "any" in the message. In any case, calling it an aggregate when it definitely isn't an aggregate is somewhat problematic, I think.

[aschackmull]

There's a FP here: https://github.com/github/codeql/security/code-scanning/5199?query=ref%3Arefs%2Fpull%2F7472%2Fmerge+ref%3Arefs%2Fpull%2F7472%2Fhead

[aschackmull]

There are a couple of those FPs - when the single use is in just one branch of an or.

[aschackmull]

Looking through some more results, I find that I generally agree with the "can replaced with an instanceof expression." suggestions, but I mostly wouldn't change any of the exists - the additional variable name typically helps by breaking up an otherwise large expression and documents an intermediate value through a variable name.

[aschackmull]

So I think we should limit the query to the cases where the entire exists can be replaced by instanceof.

[erik-krogh]

So I think we should limit the query to the cases where the entire exists can be replaced by instanceof.

I'll do that, and rewrite the query into a could be cast query.

[aschackmull]

Much better! Looking through a bunch of the alerts, they now all look like something I'd want to fix.

[erik-krogh]

Much better! Looking through a bunch of the alerts, they now all look like something I'd want to fix.

Nice 🎉
And don't worry about fixing the issues, I'll write something to fix them all.

---

Can I get a QL-for-QL reviewer to sign off on this?
/ping @github/codeql-ql-for-ql-reviewers