Allow method resolution to work on reference receiver types with Self : Sized bounds with reference trait implementations

[b-naber]

Fixes #82825

In situations in which we have a trait method that takes a reference receiver type, a Self : Sized bound and a trait implementation for a reference type, the adjustments that the compiler applies are insufficient to fulfill the Sized bound on Self. To allow the compiler to resolve the correct method we autoref during method probing, after a sized bound violation was found, try to confirm the new method candidate and check if all resulting obligations can be fulfilled.

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[b-naber]

r? @eddyb maybe?

[eddyb]

I don't think special-casing "a Self: Sized bound" is the necessary thing here, sorry I didn't explain this better.

What Self: Sized does separately of type-checking is that it causes the method to be treated as not "object safe".

So in my mind, once you found that the method comes from a dyn Trait (something like "object candidate" IIRC), you can then easily discard that candidate based on whether it's "object safe" or not.

Oh... looking at the diff, there's already a hack for a different situation involving non-object-safe methods, that special-cases Self: Sized - I feel like that special-casing should maybe be cleaned up before new behavior should be added?

But maybe I'm misunderstanding the existing code, so maybe ignore me until you get a proper review.

r? @nikomatsakis

[b-naber]

So in my mind, once you found that the method comes from a dyn Trait (something like "object candidate" IIRC), you can then easily discard that candidate based on whether it's "object safe" or not.

@eddyb That's what the compiler currently does. The existing hack you refer to is for diagnostics purposes.

It's very much possible that I misunderstood the intent of the issue. The way I understood it is that @guswynn wanted the compiler to autoref here to make Self correspond to the &T of the trait implementation and the receiver to have type &&dyn Trait therefore, so that a <&dyn Trait as Trait> predicate is created and the Self bound holds, whereas currently the compiler doesn't include an autoref and uses <dyn Trait as Trait> and <dyn Trait as Sized>. Is that different from the way you understood the issue?

Maybe @guswynn can also weigh in on this?

[b-naber]

@eddyb I think I understand what you meant now. Instead of using the hack to test whether an illegal sized bound was encountered, we could test whether the sized bound holds (or object safety in general) during method probing. If that fails (during the pick_by_value_method call), the compiler will then try to autoref anyway. Is that what you meant?

[guswynn]

@b-naber I believe your most recently comment is how I think of this problem

[b-naber]

One problem I see with ignoring non object-safe methods during method probing is that the resulting diagnostics would be less informative. Instead of being given information on what bounds aren't satisfied we would only give out a 'method not found' error. But keeping track of potential candidates, which have unfulfilled obligations, during method probing for diagnostics purposes would probably affect performance negatively. Not sure what's the right trade-off here.

[guswynn]

@b-naber is it possible in the event of a diagnostic to backtrack and recollect the methods that were tried? As far as I can tell, the diagnostic path in rustc prefers accuracy/helpfulness over performance

[b-naber]

I think we would need to re-run method probing and add an option to collect method candidates. But you're right, once we're not on the hot path anymore because of an error, performance matters a lot less. I can try that approach, thanks.

[guswynn]

@b-naber can we add a &mut test here? that was the motivating usecase

also, is there a way we can test that this has actually regressed diagnostics? ci appears to pass just fine

[b-naber]

Added the test, but we have to declare x as mutable in order for the mutable autoref to work, which might be confusing given that the autoref is implicit.

[b-naber]

We don't have diagnostics problems in this version (using the existing illegal_sized_bound hack.

[guswynn]

@b-naber oh thats very confusing...is the diagnostic around that good? as in, does it explicitly tell you to add the mut before the parameter?

[b-naber]

Hm not really great, the borrow part is confusing, I don't think we should expect the user to know that an autoref occurs here:

error[E0596]: cannot borrow `x` as mutable, as it is not declared as mutable
  --> src/test/ui/resolve/issue-82825-mut.rs:17:5
   |
17 |     x.static_call();
   |     ^
   |     |
   |     cannot borrow as mutable
   |     try removing `&mut` here

error: aborting due to previous error

What do you think?

[guswynn]

oh hmm, yeah we would likely need to improve this diagnostic

[guswynn]

Oh also we should make sure we can still call it with (x).static_call() so that this is backwards compatible

[b-naber]

Trying to solve this without the existing illegal_sized_bound hack is problematic and I haven't got this to work yet. The problem is that rejecting an ObjectCandidate once we find a Self: Sized bound is not sufficient, since the compiler also creates a TraitCandidate. I tried to reject these as well if the receiver on those methods either is a trait object itself or is a reference to a trait object and the receiver type of the method is a reference, in both cases having tested for a Self: Sized bound, but unfortunately this results in method probing not working correctly anymore. I'm not sure whether I just used the wrong conditions for deciding whether to reject a TraitCandidate and whether there is a way to get this to work.

[bors]

☔ The latest upstream changes (presumably #83580) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

Ping from triage:
@b-naber can you please resolve the merge conflicts?

@rustbot label: -S-waiting-on-review +S-waiting-on-author

[nikomatsakis]

I owe a review here, I know

[JohnCSimon]

Ping again from triage:
@b-naber can you please resolve the merge conflicts?

[JohnCSimon]

Triage:
@b-naber I'm closing this as inactive as it hasn't seen any movement in months. Please feel free to reopen if you're going to continue.

[Rua]

Any progress on this fix?