Shared/Unified: Hoist members in generated Ast.qll - #22507
Conversation
Co-authored-by: aschackmull <28296824+aschackmull@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The generated hierarchy and consumer update are consistent, and no unresolved correctness issues were found.
Review tier: Balanced
Findings: None
What changed in this PR
Hoists shared generated AST predicates onto common superclasses, enabling polymorphic access without subtype casts.
Changes:
- Adds abstract predicate generation and override detection.
- Hoists
Callable.getBody(). - Simplifies control-flow body lookup.
| File | Description |
|---|---|
shared/tree-sitter-extractor/src/generator/ql.rs |
Supports abstract predicates. |
shared/tree-sitter-extractor/src/generator/ql_gen.rs |
Computes and hoists common predicates. |
unified/ql/lib/codeql/unified/internal/Ast.qll |
Adds generated Callable.getBody() hierarchy. |
unified/ql/lib/codeql/unified/internal/ControlFlowGraph.qll |
Uses polymorphic body access. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Looks great!
Did you try regenerating the AST for Ruby and QL4QL? Were they unaffected by this?
Not blocking for this PR, but I think in the future we'll want the ability to manually declare fields on super-types to account for a few situations:
- There are subtypes where the field has a more precise type thus failing an exact signature match
- Adding a new subtype that lacks a previously-common field can cause a breaking AST change.
- Sometimes it's just nice to have getters for fields that are present in the common case, but some subtypes are lacking it. Defaulting to
none()in the base class.
| let node = nodes.get(type_name); | ||
| let class_name = node.map_or(type_name.kind.as_str(), |node| node.ql_class_name.as_str()); | ||
| if !cache.contains_key(class_name) { | ||
| let exposed = match node.map(|node| &node.kind) { |
There was a problem hiding this comment.
This leads to infinite recursion if there are super-types including one another, which is unlikely but technically possible. Can we just insert a sentinel in the cache up before recursion, to detect cycles and bail out with a nicer error message?
There was a problem hiding this comment.
Done. I initially asked copilot to do this, but it wrote a lot of overly complicated code. So I went with a minimal handwritten sentinel. It doesn't bail out, though, it just fails to hoist anything for a cycle (i.e. it's now essentially doing smallest fixpoint recursion).
| } = &node.kind | ||
| { | ||
| if fields.is_empty() { | ||
| panic!("Encountered node '{}' with no fields", type_name.kind); |
There was a problem hiding this comment.
Why panic when there are no fields?
There was a problem hiding this comment.
I don't know. This piece of code was merely moved, so I/copilot didn't change it.
I admit I did not. But copilot claimed that it did, and that they were unaffected.
Completely agree! |
This hoists identical predicates shared by all subclasses to a root-def on their superclass. The second commit shows the effect.
Copilot wrote most of the Rust code, but I've tweaked it a bit and I've read through it all and convinced myself that it does the right thing.