Rust: Data flow through overloaded operators #19685

paldepind · 2025-06-06T12:21:08Z

This PR adds data flow through (simple) overloaded operators.

It does this by:

Adding a new Call class that abstracts away the different ways in which a call can occur.
Uses Call in type inference which chops down some LOCs.
Uses Call in data flow which then causes overloaded operators to be handled as calls.

Future work:

Support operators with more complicated desugaring (assignment operators like += and the deref operator). In the tests writing out the desugaring doesn't work either, which seem to indicate this is blocked on other missing features in data flow.
I think we could try and merge Call with CallExprBase. That seems doable but also has larger ramifications.
MaD for overloaded operators. This might make more sense to do after or together with using QL created canonical paths for MaD.

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

hvitved

Overall LGTM; mostly a question about whether to use codegen for the new Call class. PR also needs to be rebased to resolve merge conflict.

hvitved · 2025-06-11T11:26:09Z

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

+   *
+   * This class abstract over the different ways in which a function can be called in Rust.
+   */
+  abstract class Call extends ExprImpl::Expr {


I wonder if this class should instead be added via annotations.py and suitable replace_bases?

It seems to me that it would make sense to try an merge Call and CallExprBase into one class. But that will require adapting a lot of places, so I think it makes sense to defer that to a follow up PR.

hvitved · 2025-06-11T11:32:25Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

-
-    override Declaration getTarget() {
-      result = inferMethodCallTarget(this) // mutual recursion; resolving method calls requires resolving types and vice versa
+      result = CallExprImpl::getResolvedFunction(this)
    }
  }

  predicate accessDeclarationPositionMatch(AccessPosition apos, DeclarationPosition dpos) {
    apos.isSelf() and


The body of this predicate can now simply be apos = dpos.

paldepind · 2025-06-12T07:01:19Z

The slowdown seems to be fixed in the most recent DCA run. adjustAccessType was mistakenly being used also for non-method calls and that caused the slowdown.

hvitved · 2025-06-12T07:56:19Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

  ) {
-    if apos.isSelf()
+    if apos.isSelf() and a.receiverImplicitlyBorrowed()


I'm not too fond of having a in adjustAccessType, it should be sufficient to know the position and not the actual access. That would require changing TDeclarationPosition and TAccessPosition back to how they were, which I also prefer anyway (that makes positions a purely syntactic property).

What are the downside of having a in adjustAccessType? Given that they're all in the bindingset I thought it wouldn't make a difference?

You are right that it doesn't matter performance-wise, I just prefer the the simpler interface, which forces type-adjustment to be a property of the position alone.

Ok. I'll make it more as before then. It just requires more boiler plate which I don't think is super nice.

For instance the binary `&` is overloadable but the prefix `&` is not. Similarly, `*` has a different target depending on if it's prefix or infix.

The exact same problem occurs in Ruby, hence the `multipleArgumentCallExclude` implementation is adapted from Ruby's.

hvitved · 2025-06-12T10:12:38Z

rust/ql/lib/codeql/rust/internal/TypeInference.qll

@@ -829,7 +767,7 @@ private Type inferCallExprBaseType(AstNode n, TypePath path) {
    n = a.getNodeAt(apos) and
    result = CallExprBaseMatching::inferAccessType(a, apos, path0)
  |
-    if apos.isSelf()
+    if apos.isSelf(_)


Shouldn't this be true?

I thought so as well, but for some reason it didn't work for operator calls. Extra & types where inferred around operator calls so I left it at _ which preserves type inference as it was.

I think it would make sense to investigate this further in follow up work.

hvitved

LGTM, some final questions.

hvitved · 2025-06-12T11:08:15Z

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

@@ -0,0 +1,7 @@
+/**
+ * Call.


Not the most usable QL doc ;-)

hvitved · 2025-06-12T11:08:33Z

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

+  /**
+   * An expression that calls a function.
+   *
+   * This class abstract over the different ways in which a function can be called in Rust.


hvitved · 2025-06-12T11:09:00Z

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

+   * This class abstract over the different ways in which a function can be called in Rust.
+   */
+  abstract class Call extends ExprImpl::Expr {
+    Call() { this.fromSource() }


Which calls happen in non-source code?

Oh, no. That was for debugging.

paldepind · 2025-06-12T14:24:24Z

Latest DCA run looks fine. The improvement to missing call targets is now more pronounced after the this.fromSource() was removed 🙈

github-actions bot added the Rust Pull requests that update Rust code label Jun 6, 2025

github-advanced-security bot found potential problems Jun 6, 2025

View reviewed changes

rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll Fixed Show fixed Hide fixed

paldepind force-pushed the rust/df-operator-overloading branch 2 times, most recently from 7947dbf to 0953436 Compare June 6, 2025 12:40

paldepind marked this pull request as ready for review June 6, 2025 12:49

paldepind requested a review from a team as a code owner June 6, 2025 12:49

paldepind changed the title ~~Rust: Data flow through operator overloading~~ Rust: Data flow through overloaded operators Jun 6, 2025

paldepind force-pushed the rust/df-operator-overloading branch from 0953436 to c8746ba Compare June 9, 2025 09:38

hvitved requested changes Jun 11, 2025

View reviewed changes

paldepind force-pushed the rust/df-operator-overloading branch 2 times, most recently from 4d73871 to 5bdf989 Compare June 11, 2025 12:42

paldepind added the no-change-note-required This PR does not need a change note label Jun 11, 2025

paldepind requested a review from hvitved June 12, 2025 07:01

hvitved reviewed Jun 12, 2025

View reviewed changes

paldepind force-pushed the rust/df-operator-overloading branch from c749269 to 436961f Compare June 12, 2025 09:08

paldepind added 10 commits June 12, 2025 11:12

Rust: Add data flow tests for operator overloading

1858355

Rust: Account for arity in operator overloading

ce1c9fb

For instance the binary `&` is overloadable but the prefix `&` is not. Similarly, `*` has a different target depending on if it's prefix or infix.

Rust: Add abstraction over all kinds of calls

4786478

Rust: Use Call in type inference

7684e01

Rust: Fix canonical path for Deref trait

5642445

Rust: Use Call in data flow

fecd445

Rust: Adapt data flow consistency queries to ! being a call

3463ebd

The exact same problem occurs in Ruby, hence the `multipleArgumentCallExclude` implementation is adapted from Ruby's.

Rust: Simplify accessDeclarationPositionMatch

03c9a78

Rust: Only adjust access type for method call expressions

f138f77

Rust: Remove Access from adjustAccessType

8cde1ee

paldepind force-pushed the rust/df-operator-overloading branch from 436961f to 8cde1ee Compare June 12, 2025 09:16

hvitved reviewed Jun 12, 2025

View reviewed changes

Rust: Address review comments

2dd2f2e

hvitved approved these changes Jun 12, 2025

View reviewed changes

paldepind merged commit e30a9e9 into github:main Jun 12, 2025
17 checks passed

Rust: Data flow through overloaded operators #19685

Rust: Data flow through overloaded operators #19685

Uh oh!

Conversation

paldepind commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

paldepind commented Jun 12, 2025

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!

paldepind Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

paldepind commented Jun 12, 2025

Uh oh!

Uh oh!

Uh oh!

paldepind commented Jun 6, 2025 •

edited

Loading

paldepind Jun 12, 2025 •

edited

Loading