Skip to content

Rust: Data flow through overloaded operators #19685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

paldepind
Copy link
Contributor

@paldepind paldepind commented Jun 6, 2025

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.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jun 6, 2025
@paldepind paldepind force-pushed the rust/df-operator-overloading branch 2 times, most recently from 7947dbf to 0953436 Compare June 6, 2025 12:40
@paldepind paldepind marked this pull request as ready for review June 6, 2025 12:49
@paldepind paldepind requested a review from a team as a code owner June 6, 2025 12:49
@paldepind paldepind changed the title Rust: Data flow through operator overloading Rust: Data flow through overloaded operators Jun 6, 2025
@paldepind paldepind force-pushed the rust/df-operator-overloading branch from 0953436 to c8746ba Compare June 9, 2025 09:38
For instance the binary `&` is overloadable but the prefix `&` is not. Similarly, `*` has a different target depending on if it's prefix or infix.
Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@paldepind paldepind force-pushed the rust/df-operator-overloading branch from c8746ba to eea6f52 Compare June 11, 2025 11:44
@paldepind paldepind force-pushed the rust/df-operator-overloading branch from 4d73871 to 5bdf989 Compare June 11, 2025 12:42
@paldepind paldepind added the no-change-note-required This PR does not need a change note label Jun 11, 2025
@paldepind
Copy link
Contributor Author

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.

@paldepind paldepind requested a review from hvitved June 12, 2025 07:01
) {
if apos.isSelf()
if apos.isSelf() and a.receiverImplicitlyBorrowed()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants