Rust/Swift: Make all public AST classes final

[hvitved]

This PR changes the code generator used by Rust and Swift so that all public classes are final. Commit-by-commit review is strongly recommended.

Before this PR

An entry such as Expr in schema.py would give rise to two classes:

generated/Expr.qll

// generated, do not edit
module Generated {
   class Expr extends Synth::TExpr, AstNode { }
}

elements/Expr.qll

// generated, remove this comment if you wish to edit this file
class Expr extends Generated::Expr { }

Any customizations needed on Expr, such as the addition of new predicates or overriding of existing predicates, would be done in elements/Expr.qll and this would also be the public version of Expr.

After this PR

We want to avoid unintended predicate dispatch, i.e. make the public Expr class final. However, we cannot simply make the Expr class in elements/Expr.qll final, because other customization classes may want to extend it. This PR therefore adds an additional layer:

generated/Expr.qll

// generated, do not edit
module Generated {
   class Expr extends Synth::TExpr, Impl::AstNode { } // note the `Impl::` prefix
}

elements/ExprImpl.qll

// generated, remove this comment if you wish to edit this file
module Impl {
  class Expr extends Generated::Expr { }
}

elements/Expr.qll

// generated, do not edit
final class Expr = Impl::Expr;

Now the exposed Expr class is final, and all customizations are done in elements/ExprImpl.qll.

[redsun82]

// generated, remove this comment if you wish to edit this file
module {
  class Expr extends Generated::Expr { }
}

meta-review comment: a module Generated is missing in the PR description there 😆

[hvitved]

meta-review comment: a module Generated is missing in the PR description there 😆

Thanks; actually it should be module Impl.

[redsun82]

I really like this, thanks for the hard work!

I think we should move all stubs to a separate directory (impl or internal), but I saw on slack you agree with this for a follow-up PR, so I'm perfectly happy with that.

[redsun82]

well spotted! 😅

[redsun82]

ah, that pesky autopep again 😅

Ah, forgot to mention, the Doc coverage failure is pretty normal, because the baseline situation (with uncovered defs) gets lost in the renames. I'm ok with ignoring it.

[hvitved]

Ah, forgot to mention, the Doc coverage failure is pretty normal, because the baseline situation (with uncovered defs) gets lost in the renames. I'm ok with ignoring it.

Yes, it was also my impression that this was because of the renamed files.

[geoffw0]

You've checked in a merge artifact CommandInjection.expected.orig. Could you do a follow-up to remove it please (I appreciate it's easy to miss stuff like this with so many other files touched by this PR).

Also why is no change note required for Swift - are we confident users can't or have been told they shouldn't be overriding these classes?

[hvitved]

You've checked in a merge artifact CommandInjection.expected.orig. Could you do a follow-up to remove it please (I appreciate it's easy to miss stuff like this with so many other files touched by this PR).

Yes, sorry, thanks for noticing.

Also why is no change note required for Swift - are we confident users can't or have been told they shouldn't be overriding these classes?

I'll add a change note.