-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Rust: Apply inherent method prioritization inside type inference loop #19903
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
Conversation
10a6897
to
84f3f8c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
84f3f8c
to
732bbb5
Compare
732bbb5
to
d379688
Compare
d379688
to
0723391
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only reviewed this in quite a shallow way as I don't know the type inference code all that well. I will review the DCA run carefully when that's complete. Are we expecting a performance improvement here???
@@ -214,7 +214,7 @@ fn test_io_stdin() -> std::io::Result<()> { | |||
{ | |||
let mut buffer = Vec::<u8>::new(); | |||
let _bytes = std::io::stdin().read_to_end(&mut buffer)?; // $ Alert[rust/summary/taint-sources] | |||
sink(&buffer); // $ MISSING: hasTaintFlow | |||
sink(&buffer); // $ hasTaintFlow -- @hvitved: works in CI, but not for me locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird. I've written an issue to look into this, my suspicion is that when we finish updating all the models and then generalize what we can to trait models, the platform specific behaviours are going to go away.
| main.rs:490:13:490:13 | x | &T.&T.&T.&T.&T.&T.&T.&T | main.rs:397:5:398:14 | S1 | | ||
| main.rs:490:13:490:13 | x | &T.&T.&T.&T.&T.&T.&T.&T.&T | file://:0:0:0:0 | & | | ||
| main.rs:490:13:490:13 | x | &T.&T.&T.&T.&T.&T.&T.&T.&T | main.rs:397:5:398:14 | S1 | | ||
| main.rs:490:17:490:18 | S1 | | file://:0:0:0:0 | & | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this certainly looks cleaner now.
DCA run has completed, and it looks fine. No performance improvements expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DCA run has completed, and it looks fine.
Yeah - there is one timeout, though I can't see that it's related to these changes. Some result changes that I now suspect are wobble. Path resolution inconsistencies may have increased slightly.
I think that is a spurious DCA issue. |
Inherent methods are prioritized over trait methods:
Previously, this prioritization was applied after the recursive type inference loop, because it involved a negative call to the method resolution (and hence type inference) relation.
However, as the added test case shows, this may lead to an explosion in inferred types, when the type signatures of the inherent/trait methods differ. This PR hence moves the restriction into the type inference loop, but in order to avoid non-monotonic recursion, we formulate the restriction as a monotonic property: A trait method only applies when all potential inherent targets have a receiver type that it not an instance of the
impl
type (the non-monotonic version is: there is no receiver type that is an instance of theimpl
type).