Ruby: CFG: make all expressions "post-order" nodes

[aibaars]

No description provided.

[nickrolfe]

I haven't reviewed the test output, but the changes in ControlFlowGraphImpl.qll all look sensible.

[hvitved]

Looks great, it even simplified some existing logic. I suggest we rebase this once #7393 has been merged, and then get rid of the last commit.

[hvitved]

Did you consider

import codeql.ruby.controlflow.internal.Completion
import codeql.ruby.controlflow.internal.ControlFlowGraphImplShared

query predicate nonPostOrderExpr(Expr e, string cls) {
  cls = e.getPrimaryQlClasses() and
  not exists(e.getDesugared()) and
  exists(AstNode last, Completion c |
    last(e, last, c) and
    last != e and
    c instanceof NormalCompletion
  )
}

instead?

[aibaars]

That gives quite a few results : https://github.com/github/codeql/runs/4549392461?check_suite_focus=true

[hvitved]

That made me realize that the modelling of InClause is actually not like (I think) it should be. There is a subtle difference: before, the node for the InClause itself was visited regardless of a match, but now it is only visited if there is a match.

There are two options: Revert 1343ed5 or use CFG splitting to make sure that InClauses are truly post-order. I don't think splitting is worth the effort, so I suggest a revert.

With a revert, we can update the check above to

query predicate nonPostOrderExpr(Expr e, string cls) {
  cls = e.getPrimaryQlClasses() and
  not exists(e.getDesugared()) and
  not e instanceof InClause and
  not e instanceof BeginExpr and
  not e instanceof Namespace and
  not e instanceof Toplevel and
  exists(AstNode last, Completion c |
    last(e, last, c) and
    last != e and
    c instanceof NormalCompletion
  )
}

[aibaars]

That made me realize that the modelling of InClause is actually not like (I think) it should be. There is a subtle difference: before, the node for the InClause itself was visited regardless of a match, but now it is only visited if there is a match.

I think this is the correct behaviour; if there is no match then the InClause has no value (not even nil).

[aibaars]

As discussed, perhaps InClause shouldn't be an Expr in the first place.

[hvitved]

I think this should be c.(MatchingCompletion).getValue() = true.

[aibaars]

A pattern may also end with a simple completion, for example a *var never fails.

[aibaars]

Could write the following instead c instanceof MatchingCompletion implies c.(MatchingCompletion).getValue() = true

[hvitved]

LGTM. I have started a final DCA run; let's wait for that before we merge.