Rust: Implement toString for struct fields and visibility
#20924
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements custom toString methods for StructField and Visibility AST nodes in the Rust CodeQL library, improving their string representations from generic class names to meaningful syntax-based descriptions.
- Implements
toStringforVisibilityto displaypuborpub(path)instead ofVisibility - Implements
toStringforStructFieldto display field signatures likefield: typeorpub field: type - Updates all test expectations to reflect the new string representations
Reviewed changes
Copilot reviewed 7 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/elements/internal/VisibilityImpl.qll |
Implements toStringImpl and toAbbreviatedString for Visibility to render as pub or pub(path) |
rust/ql/lib/codeql/rust/elements/internal/StructFieldImpl.qll |
Implements toStringImpl for StructField to render field signatures with optional visibility, name, and type |
rust/ql/.gitattributes |
Removes VisibilityImpl.qll from linguist-generated list since it's now hand-modified |
rust/ql/.generated.list |
Removes VisibilityImpl.qll from the generated files list |
rust/ql/test/extractor-tests/utf8/ast.expected |
Updates test expectations for Visibility and StructField toString output |
rust/ql/test/extractor-tests/macro-in-library/PrintAst.expected |
Updates test expectations for Visibility toString output |
rust/ql/test/extractor-tests/macro-expansion/PrintAst.expected |
Updates test expectations for Visibility and StructField toString output |
rust/ql/test/extractor-tests/generated/Visibility/Visibility.expected |
Updates test expectations for Visibility instances |
rust/ql/test/extractor-tests/generated/Trait/Trait.expected |
Updates test expectations for Visibility in trait declarations |
rust/ql/test/extractor-tests/generated/StructFieldList/StructFieldList.expected |
Updates test expectations for StructField instances in field lists |
rust/ql/test/extractor-tests/generated/StructField/StructField.expected |
Updates test expectations for StructField instances |
rust/ql/test/extractor-tests/generated/Module/Module.expected |
Updates test expectations for Visibility in module declarations |
rust/ql/test/extractor-tests/generated/MacroDef/MacroDef.expected |
Updates test expectations for Visibility in macro definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Holds if this record field is named `name` and belongs to the struct `s`. */ | ||
| predicate isStructField(Struct s, string name) { this = s.getStructField(name) } | ||
|
|
||
| override string toStringImpl() { result = concat(int i | | this.toStringPart(i) order by i) } |
There was a problem hiding this comment.
Done. But I don't really understand why. It doesn't make a difference here does it? Should we use strictconcat in all toStringImpls (like this one)?
There was a problem hiding this comment.
strictconcat produces slightly simpler DIL; it is generally always preferable to use strict aggregates when we know that the range is non-empty.
There was a problem hiding this comment.
Thanks. So the answer to the second question is: yes, we should also use strictconcat in the other toStringImpls.
2 participants
Implements
toStringforStructFieldandVisibility.