Rust: Add codegen for generating a toStringImpl that delegates to getAPrimaryQlClass - #22603
paldepind wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The broad generated-code migration and performance-sensitive QL dispatch behavior warrant final human validation.
Review effort: Balanced
Findings: None
Adds an inherited codegen option that generates concrete Rust toStringImpl fallbacks while preserving specialized implementations.
Changes:
- Adds and tests the inherited schema pragma.
- Enables fallback generation under Element, excluding LabelableExpr.
- Regenerates Rust QL classes and restores eligible wrappers as generated files.
| File | Description |
|---|---|
| misc/codegen/lib/{ql.py,schemadefs.py} | Defines the new codegen option and pragma. |
| misc/codegen/generators/qlgen.py | Propagates the pragma into QL generation. |
| misc/codegen/templates/ql_class.mustache | Emits the concrete toStringImpl. |
| misc/codegen/test/{test_qlgen.py,test_schemaloader.py} | Tests generation and inherited overrides. |
| rust/schema/{prelude.py,annotations.py} | Enables the fallback and excludes the final custom implementation hierarchy. |
| rust/ql/lib/codeql/rust/elements/internal/generated/*.qll | Adds generated concrete fallbacks. |
| rust/ql/lib/codeql/rust/elements/internal/*Impl.qll | Removes now-generated manual fallbacks. |
| rust/ql/.gitattributes | Reclassifies regenerated wrappers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
hvitved
left a comment
There was a problem hiding this comment.
Nice. I think this means we can get rid of the override string toStringImpl() { none() } in ElementImpl.qll? Did you verify that Element.toString() still doesn't require evaluating path resolution?
Sorry, something went wrong.
|
Element.toString() did in fact require path resolution. I think the problem was that we generated toStringImpl for all classes, even those where the manual wrapper added a manual toStringImpl. For instance, PathExpr has a manual toStringImpl, but we also created one on the generated class that called out to getAPrimaryQlClass (which for PathExpr depends on path resolution). With the latest commit the generator checks for a manual toStringImpl the existence of which suppresses the generated one. The DIL when evaluating toString now shows no path resolution predicates as far as I can see. Removing the none() for toStringImpl in ElementImpl.qll results in this error: Failed to run query: ERROR: Addressable must implement abstract predicate ElementImpl::Impl::Element.toStringImpl() I haven't investigated any further, since things seem to otherwise work :) |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
This PR adds an additional option to codegen for generating a toStringImpl that delegates to getAPrimaryQlClass.
This reverts the files that #22520 made manual into generated once.
The option is rather Rust specific, but oh well.