Ruby: generate overlay discard predicates #19719
Conversation
fb89f22 to
576f0d2
Compare
b74138e to
8a33cb4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for overlay-based discard predicates in the Ruby Tree-sitter extractor, enabling selective filtering of AST nodes and files when running on overlay databases.
- Introduces
OverlayAnnotationandPragmaAnnotationin the QL generator to emit overlay/pragma directives. - Extends
ql_gento produce new predicates (isOverlay,getRawFile,discardFile,discardableAstNode,discardAstNode). - Updates the code generator to conditionally include overlay predicates and adds static definitions in the Ruby QLL.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| shared/tree-sitter-extractor/src/generator/ql_gen.rs | Added functions to generate overlay discard predicates. |
| shared/tree-sitter-extractor/src/generator/ql.rs | Added TopLevel::Predicate, PragmaAnnotation, OverlayAnnotation, and Expression::Negation. |
| shared/tree-sitter-extractor/src/generator/mod.rs | Renamed add_metadata_relation to overlay_support and wired in overlay predicates. |
| ruby/ql/lib/codeql/ruby/ast/internal/TreeSitter.qll | Added overlay/pragma annotations and new predicates for Ruby and ERB modules. |
Comments suppressed due to low confidence (2)
shared/tree-sitter-extractor/src/generator/ql.rs:271
- Add a space or newline after the closing doc comment for readability, e.g.:
write!(f, "/** {} */ ", qldoc)?;so thatoverlay[...]is clearly separated.
write!(f, "/** {} */", qldoc)?;
shared/tree-sitter-extractor/src/generator/mod.rs:63
- Consider adding unit or integration tests to verify that
overlay_supportcorrectly causes the metadata relation and overlay predicates to be written.
if overlay_support {
| @@ -48,6 +53,38 @@ module Ruby { | |||
| final override string getAPrimaryQlClass() { result = "ReservedWord" } | |||
| } | |||
|
|
|||
| /** Gets the file path of the given `node`. */ | |||
There was a problem hiding this comment.
[nitpick] The overlay predicate blocks for Ruby and ERB are nearly identical; consider extracting common definitions (e.g., via a shared include) to reduce duplication.
Copilot uses AI. Check for mistakes.
| /** Holds if `node` should be discarded, because it is part of the overlay base and is in a file that was also extracted as part of the overlay database. */ | ||
| overlay[discard_entity] | ||
| pragma[nomagic] | ||
| predicate discardAstNode(@ruby_ast_node node) { |
There was a problem hiding this comment.
I think all the new predicates should be private since they're not intended to be called from the outside.
| /** Gets the file path of the given `node`. */ | ||
| overlay[local] | ||
| pragma[nomagic] | ||
| string getRawFile(@ruby_ast_node node) { |
There was a problem hiding this comment.
IIRC the name getRawFile comes from the Java discard predicates and makes sense there because Java has a translation layer in QL where we wanted to get file names on the raw/input side of it. It doesn't necessarily make sense for Ruby.
There was a problem hiding this comment.
With the right local annotations elsewhere, we could remove this predicate entirely, and instead make calls like node.(Ruby::AstNode).getLocation().getFile(), but I'd like to minimise my changes for now. I'll change its name to getNodeFile.
| /** Gets the file path of the given `node`. */ | ||
| overlay[local] | ||
| pragma[nomagic] | ||
| string getRawFile(@ruby_ast_node node) { |
There was a problem hiding this comment.
Could we use @file instead of string in these predicates? AFAICT it would make the code simpler and also clearer because the types will be more precise.
There was a problem hiding this comment.
Sure. I suppose I was just cargo-culting the use of string.
8a33cb4 to
acf0fe3
Compare
576f0d2 to
4029b89
Compare
acf0fe3 to
4875dcb
Compare
|
@aibaars if you assume the generated QL is correct, would you be able to review the generator changes? |
Stacked on top of #19684