-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Avoid committing to autoderef in object method probing #57885
Conversation
@arielb1 I've been wondering about trying to move all of method selection to a canonical query. Have you thought about that at all? |
I'll rather talk a bit about it on Zulip. |
So I think the simpler approach to things I have found is actually sound, and I added a test for a few of the edge-cases that should pass (it passes today, and should continue passing). I'll rewrite this PR when I have the time for that. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@arielb1 is the "simpler approach" reflected in this PR now, or is that a new thing that you have yet to push? |
These aren't fixed by this PR, but were broken in a few older attempts at it. Make sure they don't regress.
PR rewritten to use new and much-simpler approach. That's not what I had in mind at first, but it's even better. |
@arielb1 in your earlier version of this PR, you said you didn't want to beta-nominate the first version you posted. From skimming this (simpler) version, it seems like a reasonable thing to consider backporting this to beta. Do you still dislike the idea of backporting this? Or do you agree it would be worthwhile to backport this? |
I'm not quite sure. This looks simple enough, but it's subtle enough I'm not sure I want it to land on beta. |
I'm actually ok with both options - the bug this PR fixes is quite a corner case. |
beta-nominating to ensure we discuss at weekly meeting (either today or at some point after this patch lands.) |
So @nikomatsakis told me I should r+ this myself. @bors r=nikomatsakis |
📌 Commit 927d01f has been approved by |
Avoid committing to autoderef in object method probing This fixes the "leak" introduced in #57835 (see test for details, also apparently #54252 had no tests for the "leaks" that were fixed in it, so go ahead and add one). Maybe beta-nominating because regression, but I'm against landing things on beta we don't have to. r? @nikomatsakis
☀️ Test successful - checks-travis, status-appveyor |
[beta] Rollup backports Cherry-picked: * #58207: Make `intern_lazy_const` actually intern its argument. * #58161: Lower constant patterns with ascribed types. * #57908: resolve: Fix span arithmetics in the import conflict error * #57835: typeck: remove leaky nested probe during trait object method resolution * #57885: Avoid committing to autoderef in object method probing * #57646: Fixes text becoming invisible when element targetted Rolled up: * #58522: [BETA] Update cargo r? @ghost
[beta] Rollup backports Cherry-picked: * #58207: Make `intern_lazy_const` actually intern its argument. * #58161: Lower constant patterns with ascribed types. * #57908: resolve: Fix span arithmetics in the import conflict error * #57835: typeck: remove leaky nested probe during trait object method resolution * #57885: Avoid committing to autoderef in object method probing * #57646: Fixes text becoming invisible when element targetted Rolled up: * #58522: [BETA] Update cargo r? @ghost
This fixes the "leak" introduced in #57835 (see test for details, also apparently #54252 had no tests for the "leaks" that were fixed in it, so go ahead and add one).
Maybe beta-nominating because regression, but I'm against landing things on beta we don't have to.
r? @nikomatsakis