unified: Add facade AST - #22258
Merged
Merged
Conversation
Users are not supposed to 'import' these files directly, so putting them into 'internal'.
The split between private, non-final Impl classes and public final classes ultimately prevented the facade AST from instrumenting base classes like 'Expr' and have its subclasses actually inherit those members. This commit takes a step towards fixing that by moving the final aliases into a separate module and making the other classes public and stripping their Impl suffix. It is up to each language to avoid leaking the non-final classes (which Ruby and QL4QL don't do anyway).
Previously each class just used 'AstNode' as its base class. Now it mentions each of the supertypes it is part of.
Now that final classes are factored out, base classes can refer to the facade now. Also covers a few other missed cases.
ConditionalPattern was not added to getEnclosingOrPattern() Update the test to detect the bug
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a façade layer for extending generated unified AST classes while preserving final public aliases.
Changes:
- Extends the AST generator with façade-aware imports, inheritance, and final aliases.
- Adds hand-written unified AST helpers for modifiers and enclosing patterns.
- Updates variable-binding tests and generator consumers.
Show a summary per file
| File | Description |
|---|---|
unified/scripts/create-extractor-pack.sh |
Generates the internal AST library. |
unified/ql/test/library-tests/variables/variables.ql |
Adds ambiguity validation. |
unified/ql/test/library-tests/variables/variables.expected |
Records expected empty failure sets. |
unified/ql/test/library-tests/variables/test.swift |
Tests multi-pattern bindings. |
unified/ql/test/library-tests/BasicTest/test.ql |
Tests the public import. |
unified/ql/lib/unified.qll |
Exposes the final façade AST. |
unified/ql/lib/codeql/unified/internal/Variables.qll |
Uses generalized pattern traversal. |
unified/ql/lib/codeql/unified/internal/FacadeAst.qll |
Defines hand-written façade members. |
unified/ql/lib/codeql/unified/internal/Ast.qll |
Updates generated unified AST output. |
unified/extractor/src/generator.rs |
Enables façade generation. |
shared/tree-sitter-extractor/src/node_types.rs |
Makes type names cloneable. |
shared/tree-sitter-extractor/src/generator/ql.rs |
Adds private imports and façade types. |
shared/tree-sitter-extractor/src/generator/ql_gen.rs |
Generates façade-aware class hierarchies. |
shared/tree-sitter-extractor/src/generator/mod.rs |
Produces façade and final modules. |
ruby/extractor/src/generator.rs |
Selects non-façade generation. |
ql/ql/src/codeql_ql/ast/internal/TreeSitter.qll |
Updates generated QL AST output. |
ql/extractor/src/generator.rs |
Selects non-façade generation. |
Review details
- Files reviewed: 16/18 changed files
- Comments generated: 0
- Review effort level: Medium
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
When not using a facade, the self-import could accidentally resolve to a file instead of the enclosing module.
asgerf
marked this pull request as ready for review
July 31, 2026 08:22
tausbn
previously approved these changes
Jul 31, 2026
tausbn
left a comment
Contributor
There was a problem hiding this comment.
One minor comment, otherwise this looks good to me. 👍
Comment on lines
+5
to
+10
| type TypeCheckContext<'a> = ( | ||
| &'a Schema, | ||
| Option<&'a [crate::schema::NodeType]>, | ||
| Option<(&'a str, &'a str)>, | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
Maybe this ought to be upgraded to a named struct? It's not immediately obvious to me what the various parts of this tuple represent.
Contributor
Author
There was a problem hiding this comment.
Fair enough. Fixed in the next commit.
tausbn
approved these changes
Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds support for a "Facade AST" layer in the tree-sitter-extractor's QL generator, and a few other related tasks.
getA/getAn-getters for plural fields, so we can writegetAnArgument()instead ofgetArgument(_).unifiedto exercise the new feature a bit.Augmenting AST node classes
To augment an AST node, define a same-named class in the facade AST module, subclassing and shadowing the original class.
For example:
The facade module publicly imports the generated AST module, so any classes that have not been augmented will be exposed as-is.
Changes to the generated AST
In the generated AST, references to AST node classes now target the facade (which may resolve back to the generated AST itself, for classes that have not been augmented):
Final aliases
This trick mentioned above was initially obstructed by the final aliases, making it impossible to augment a base class of another class. For example, the generated AST for
AstNodewas actuallyIf we were to augment the
AstNodeclass in the facade AST, the subclasses would not actually inherit its members, because they extendAstNodeImpl, which was private and could thus not be augmented.To fix this, the layer of final aliases needs to happen after the facade AST, not in-between. There is now a separate module with all the final aliases:
In
unifiedwe expose theAstFinalthroughunified.qll. For Ruby and QL4QL it doesn't really matter which one is exposed since the AST is hidden from end-users anyway.