Rust: add some `toString` implementations by redsun82 · Pull Request #18035 · github/codeql

redsun82 · 2024-11-19T16:21:38Z

This sets up better toString implementations to give more meaningful test expectations and AST printing results.

This is by no means systematic and complete. Most notably almost all type representations are still missing, and some less frequent Exprs, but it should get us started.

The strategy is to make toString as close as possible to the rust syntax, but

skipping things like attributes altogether
replacing "complex" subnodes with ...

What is considered a "simple" subnode (causing it to be part of the parent toString) can be tweaked by implementing toAbbreviatedString(), which returns ... by default. Currently simple subnodes are:

literal expressions and patterns
_ expressions and patterns
path expressions and patterns (i.e. name references)

This means for example that

a call with an identifier target will have that identifier in its toString (i.e. source(...))
same goes for operations (i.e. x + ....) or statements (if cond {...})

rust/ql/lib/codeql/rust/elements/internal/CallExprImpl.qll

Also make `--learn` work with the nested qltest tests.

rust/ql/test/query-tests/diagnostics/does_not_compile.rs

rust/ql/lib/codeql/rust/elements.qll

rust/ql/lib/codeql/rust/elements/LoopingExpr.qll

+
+private import internal.LoopingExprImpl
+import codeql.rust.elements.BlockExpr
+import codeql.rust.elements.LabelableExpr


rust/ql/lib/codeql/rust/elements/internal/OrPatImpl.qll

redsun82 · 2024-11-20T16:37:22Z

rust/ql/lib/codeql/rust/elements/internal/LiteralExprImpl.qll

  class LiteralExpr extends Generated::LiteralExpr {
    override string toString() { result = this.getTextValue() }
+
+    override string toAbbreviatedString() { result = this.getTextValue() }


I now thought we might want to trim long literals here

geoffw0 · 2024-11-20T18:48:23Z

rust/ql/test/library-tests/variables/variables.expected

-| variables.rs:332:9:332:9 | v | variables.rs:332:13:332:41 | RefExpr |
+| variables.rs:155:9:155:10 | p2 | variables.rs:155:14:155:37 | Point {...} |
+| variables.rs:169:9:169:11 | msg | variables.rs:169:15:169:38 | Message::Hello {...} |
+| variables.rs:189:9:189:14 | either | variables.rs:189:18:189:33 | Either::Left(...) |


I'm probably not the right person to review the extractor changes right now but I will say this: these look good, much better than before.

hvitved

Thanks a lot for taking on this task! My main concern is that ideally toString should not be recursive, since (a) it takes longer to evaluate, (b) the DIL becomes more complex, and (c) it prevents magic sets optimizations.

hvitved · 2024-11-21T08:14:05Z

rust/ql/lib/codeql/rust/elements/internal/BreakExprImpl.qll

      )
    }
+
+    override string toString() { result = concat(int i | | this.toStringPart(i), " " order by i) }


May as well use strictconcat.

hvitved · 2024-11-21T08:18:32Z

rust/ql/lib/codeql/rust/elements/internal/CastExprImpl.qll

-  class CastExpr extends Generated::CastExpr { }
+  class CastExpr extends Generated::CastExpr {
+    override string toString() {
+      result = this.getExpr().toAbbreviatedString() + " as " + this.getTy().toString()


We should avoid, if possible, to make toString recursive. Could we instead have a predicate, getName, on TypeRef?

hvitved · 2024-11-21T08:20:20Z

rust/ql/lib/codeql/rust/elements/internal/ContinueExprImpl.qll

+    private string toStringPart(int index) {
+      index = 0 and result = "continue"
+      or
+      index = 1 and result = this.getLifetime().toString()


Again, better if we can avoid recursion.

hvitved · 2024-11-21T08:21:07Z

rust/ql/lib/codeql/rust/elements/internal/EnumImpl.qll

   */
-  class Enum extends Generated::Enum { }
+  class Enum extends Generated::Enum {
+    override string toString() { result = "enum " + this.getName().toString() }


getText instead of toString.

hvitved · 2024-11-21T08:21:21Z

rust/ql/lib/codeql/rust/elements/internal/FieldExprImpl.qll

+    override string toString() {
+      exists(string abbr, string name |
+        abbr = this.getExpr().toAbbreviatedString() and
+        name = this.getNameRef().toString() and


Again, better if we can avoid recursion.

hvitved · 2024-11-21T08:30:37Z

rust/ql/lib/codeql/rust/elements/internal/RecordExprFieldImpl.qll

+    override string toString() { result = concat(int i | | this.toStringPart(i) order by i) }
+
+    private string toStringPart(int index) {
+      index = 0 and result = this.getNameRef().toString()


hvitved · 2024-11-21T08:30:45Z

rust/ql/lib/codeql/rust/elements/internal/RecordExprImpl.qll

   */
-  class RecordExpr extends Generated::RecordExpr { }
+  class RecordExpr extends Generated::RecordExpr {
+    override string toString() { result = this.getPath().toString() + " {...}" }


hvitved · 2024-11-21T08:30:51Z

rust/ql/lib/codeql/rust/elements/internal/RecordPatImpl.qll

   */
-  class RecordPat extends Generated::RecordPat { }
+  class RecordPat extends Generated::RecordPat {
+    override string toString() { result = this.getPath().toString() + " {...}" }


hvitved · 2024-11-21T08:31:42Z

rust/ql/lib/codeql/rust/elements/internal/TraitImpl.qll

   */
-  class Trait extends Generated::Trait { }
+  class Trait extends Generated::Trait {
+    override string toString() { result = "trait " + this.getName() }


hvitved · 2024-11-21T08:31:50Z

rust/ql/lib/codeql/rust/elements/internal/UnionImpl.qll

   */
-  class Union extends Generated::Union { }
+  class Union extends Generated::Union {
+    override string toString() { result = "union " + this.getName().toString() }


…`toString`

rust/ql/consistency-queries/DataFlowConsistency.ql

…ype`

hvitved

Great work on making toString non-recursive, thanks!

Rust: add some toString implementations

a709fc6

github-actions bot added the Rust Pull requests that update Rust code label Nov 19, 2024

Paolo Tranquilli added 2 commits November 19, 2024 17:50

Rust: accept test changes

fce13ae

Rust: accept integration test changes

e4f982c

paldepind reviewed Nov 20, 2024

View reviewed changes

rust/ql/lib/codeql/rust/elements/internal/CallExprImpl.qll Outdated Show resolved Hide resolved

Paolo Tranquilli added 3 commits November 20, 2024 09:45

Rust: accept last integration test changes

4fb028c

Also make `--learn` work with the nested qltest tests.

Rust: make CallExpr.toString use call identifier if present

57973df

Rust: fix non-existent string representations

4d04c5a

redsun82 commented Nov 20, 2024

View reviewed changes

rust/ql/test/query-tests/diagnostics/does_not_compile.rs Show resolved Hide resolved

Rust: more advanced toString

b4af5a6

github-advanced-security bot found potential problems Nov 20, 2024

View reviewed changes

Paolo Tranquilli added 2 commits November 20, 2024 16:49

Rust: accept test changes

fd45e11

Rust: fix OrPat.toString

d609c1b

redsun82 marked this pull request as ready for review November 20, 2024 16:00

redsun82 commented Nov 20, 2024

View reviewed changes

geoffw0 reviewed Nov 20, 2024

View reviewed changes

hvitved requested changes Nov 21, 2024

View reviewed changes

Paolo Tranquilli added 2 commits November 21, 2024 11:13

Rust: fix Path.toString and address some review comments

5012332

Rust: add consistency query printing AST classes of nodes with wrong …

d8b453f

…`toString`

hvitved reviewed Nov 21, 2024

View reviewed changes

rust/ql/consistency-queries/DataFlowConsistency.ql Outdated Show resolved Hide resolved

Paolo Tranquilli added 5 commits November 21, 2024 11:42

Rust: add class printouts to AstConsistency.ql

36d8a6d

Merge branch 'main' into redsun82/rust-str

6a61549

Rust: accept test changes

c2b426d

Rust: tweak PathType string representations and add one for `InferT…

6776b31

…ype`

Rust: accept test changes

a82d267

redsun82 requested a review from hvitved November 22, 2024 09:31

Paolo Tranquilli added 2 commits November 22, 2024 14:44

Merge branch 'main' into redsun82/rust-str

e7c10c6

Rust: accept test changes

b32e578

hvitved approved these changes Nov 23, 2024

View reviewed changes

hvitved merged commit 4b0b038 into main Nov 25, 2024

hvitved deleted the redsun82/rust-str branch November 25, 2024 08:29

Conversation

redsun82 commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

Check warning

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hvitved left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

redsun82 commented Nov 19, 2024 •

edited

Loading