Codegen: improve implementation of generated parent/child relationship #19866
Conversation
This improves the implementation of the generated parent/child relationship by adding a new `all_children` field to `ql.Class` which lists all children (both direct and inherited) of a class, carefully avoiding duplicating children in case of diamond inheritance. This: * simplifies the generated code, * avoid children ambiguities in case of diamond inheritance. This only comes with some changes in the order of children in the generated tests (we were previously sorting bases alphabetically there). For the rest this should be a non-functional change.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances codegen for parent/child relationships by introducing an all_children list on ql.Class, ensuring both direct and inherited children are included without duplication. It refactors generators to a Resolver class, updates templates to iterate over all_children, and aligns tests with the new API.
- Add
all_childrensupport and replaceprev_childlogic across lib, templates, and generators - Refactor
qlgeninto a statefulResolverwith caching and diamond-safe traversal - Update tests in
test_qlgen.py/test_ql.pyand checksums in generated file lists
Reviewed Changes
Copilot reviewed 8 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| swift/ql/.generated.list | Updated checksum for ParentChild.qll reflecting template changes |
| rust/ql/.generated.list | Updated checksum for ParentChild.qll reflecting template changes |
| misc/codegen/test/test_qlgen.py | Adjust tests to assert all_children and drop prev_child args |
| misc/codegen/test/test_ql.py | Replace properties-based child checks with all_children usage |
| misc/codegen/templates/ql_parent.mustache | Refactor getImmediateChildOf to loop over all_children |
| misc/codegen/lib/ql.py | Add Child class, is_child flag, and all_children on Class |
| misc/codegen/generators/rusttestgen.py | Use Resolver.should_skip_qltest instead of previous skip logic |
| misc/codegen/generators/qlgen.py | Introduce Resolver class, caching, and new property/class methods |
Comments suppressed due to low confidence (4)
misc/codegen/generators/qlgen.py:254
- [nitpick] Add a docstring to
_get_all_propertiesto explain its traversal order, deduplication strategy, and how it handles inherited properties.
def _get_all_properties(
misc/codegen/test/test_qlgen.py:474
- Consider adding a dedicated test case for a diamond inheritance scenario to verify that
all_childrencorrectly avoids duplicate entries when multiple bases introduce the same child property.
[
misc/codegen/generators/rusttestgen.py:63
- The
qlgenmodule is referenced here but not imported; please addimport qlgenat the top ofrusttestgen.pyto avoid a NameError.
resolver = qlgen.Resolver(schema.classes)
misc/codegen/templates/ql_parent.mustache:12
- [nitpick] This deeply nested logic around
getImmediateChildOf{{name}}can be hard to follow. Consider refactoring into smaller sections or adding clarifying comments to improve readability.
{{#final}}
|
Hmm, I'm a bit surprised there were some changes in results highlighted by DCA on the rust experiment. @hvitved is there maybe some instability there? |
Yes, I have also seen some wobble when databases are not cached. |
I see. I've tried locally slapping a |
|
In order to be sure I'm rerunning a DCA experiment for rust with database caching (I have it off by default because I usually work on the extractor). |
|
@hvitved even with cached databases there was a small variation, but looking better than without cached DBs. I'm willing to accept that, even if I can't fully explain it. What do you think? |
I see your DCA run still uses |
aha, I see! Let me try disabling that as well then. |
|
@hvitved that's it, no changes at all this time around, thanks for the support! |
This improves the implementation of the generated parent/child relationship by adding a new
all_childrenfield toql.Classwhich lists all children (both direct and inherited) of a class, carefully avoiding duplicating children in case of diamond inheritance. This:This only comes with some changes in the order of children in the generated tests (we were previously sorting bases alphabetically there). For the rest this is a non-functional change, particularly highlighted by the fact that no
PrintAsttests changed their results. It also only changes a private module, so no change note is even required.While we currently have no diamond inheritance of child fields, we will have it shortly in Rust.