Skip to content
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

Method resolution fails to find inherent method on custom Deref type #53843

Closed
withoutboats opened this issue Aug 31, 2018 · 9 comments
Closed
Assignees
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@withoutboats
Copy link
Contributor

This cannot find the poll method on Pin:

use std::ops::Deref;

pub struct Pin<P>(P);

impl<P, T> Deref for Pin<P> where
    P: Deref<Target = T>,
{
    type Target = T;
    fn deref(&self) -> &T {
        &*self.0
    }
}

impl<'a, F> Pin<&'a mut F> {
    fn poll(self) {}
}

fn test(pin: Pin<&mut ()>) {
    pin.poll()
}

https://play.rust-lang.org/?gist=6e3b3f6f3aa4e8a49cb45480f4dd7e7a&version=stable&mode=debug&edition=2015

This clearly should compile.

This bug may be a blocker on changing the Pin API to a composeable form.

cc @nikomatsakis @eddyb @arielb1 @cramertj

@withoutboats withoutboats added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Aug 31, 2018
@withoutboats
Copy link
Contributor Author

It's specifically the transitive dereferencing that causes the bug, if the deref impl is changed to this it compiles fine:

impl<P> Deref for Pin<P> where {
    type Target = P;
    fn deref(&self) -> &P {
        &self.0
    }
}

@mikeyhew
Copy link
Contributor

I managed to reduce it a bit more:

use std::{
    ops::Deref,
};

pub struct Pin<P>(P);

impl<P, T> Deref for Pin<P>
where
    P: Deref<Target=T>,
{
    type Target = T;

    fn deref(&self) -> &T {
        &*self.0
    }
}

impl<P> Pin<P> {
    fn poll(self) {}
}

fn main() {
    let mut unit = ();
    let pin = Pin(&mut unit);
    pin.poll();
}

The error goes away if you remove the Deref impl for Pin altogether. What happens is the final type produced by autoderef is an inference variable (https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/method/probe.rs#L349), causing an error to be raised when structurally_resolved_type is called, because that type could have another method named poll that might conflict. Without the Deref impl for Pin, the error goes away, because the final type for autoderef is just Pin<&mut ()>.

Of course, the final type of autoderef in this case should actually be <Pin<&mut ()> as Deref>::Target, which is (). The fact that it is an inference variable seems like a bug.

@mikeyhew
Copy link
Contributor

My last comment is only for the "type annotations needed" error. I thought the "no method named poll found for type Pin<&mut ()>" error was also being caused by that, but it looks like it might be separate. Investigating...

@mikeyhew
Copy link
Contributor

Never mind, looks like the ambiguity error is triggering the "method not found" error. The reason is the return None here (https://github.com/rust-lang/rust/blob/master/src/librustc_typeck/check/method/probe.rs#L351), which makes the caller think that no methods by that name were found. If we remove that early return it should avoid the double error.

@arielb1
Copy link
Contributor

arielb1 commented Aug 31, 2018

The problem is that autoderef is not eagerly executing nested obligations. The original reason for that is that we can't use the "outer" fulfillment context, while creating a nested fulfillment context felt like it would cause problems (or maybe it did cause a problem back then?).

I'm not sure there's a good reason not to create a fulfillment context and drain it there, only passing the remaining obligations downwards. cc @nikomatsakis

@arielb1
Copy link
Contributor

arielb1 commented Aug 31, 2018

Now that we have canonoicalization, I'm think a good idea would be to do the autoderef chain in a "canonicalization context", and only apply a deref when we are considering it.

@aturon
Copy link
Member

aturon commented Sep 14, 2018

I've assigned @nikomatsakis and @eddyb with the hopes that we can make some progress here :-)

@nikomatsakis
Copy link
Contributor

Note that this version does compile:

use std::ops::Deref;

pub struct Pin<P>(P);

impl<P> Deref for Pin<P> where
    P: Deref,
{
    type Target = P::Target;
    fn deref(&self) -> &P::Target {
        &*self.0
    }
}

impl<'a, F> Pin<&'a mut F> {
    fn poll(self) {}
}

fn test(pin: Pin<&mut ()>) {
    pin.poll()
}

The only difference is that I am using P::Target and not T

@aturon
Copy link
Member

aturon commented Sep 14, 2018

Sounds from @cramertj that this should be enough to unblock the pin API work -- thanks @nikomatsakis!

arielb1 added a commit to arielb1/rust that referenced this issue Sep 16, 2018
This is a hack-fix to rust-lang#53843, but I am worried it might break things
because it makes the "inference pollution" problem worse.

Fixes rust-lang#53843 (but introduces a bug that someone might notice).
bors added a commit that referenced this issue Sep 18, 2018
process nested obligations in autoderef

This is a hack-fix to #53843, but I am worried it might break things because it makes the "inference pollution" problem worse.

I need to do the "autoderef querification" thing somehow to solve t.

Fixes #53843 (but introduces a bug that someone might notice).

r? @nikomatsakis
bors added a commit that referenced this issue Sep 19, 2018
Update to a new pinning API.

~~Blocked on #53843 because of method resolution problems with new pin type.~~

@r? @cramertj

cc @RalfJung @pythonesque anyone interested in #49150
arielb1 added a commit to arielb1/rust that referenced this issue Sep 22, 2018
This is a hack-fix to rust-lang#53843, but I am worried it might break things
because it makes the "inference pollution" problem worse.

Fixes rust-lang#53843 (but introduces a bug that someone might notice).
bors added a commit that referenced this issue Sep 23, 2018
process nested obligations in autoderef

This is a hack-fix to #53843, but I am worried it might break things because it makes the "inference pollution" problem worse.

I need to do the "autoderef querification" thing somehow to solve t.

Fixes #53843 (but introduces a bug that someone might notice).

r? @nikomatsakis
arielb1 added a commit to arielb1/rust that referenced this issue Dec 14, 2018
This is a hack-fix to rust-lang#53843, but I am worried it might break things
because it makes the "inference pollution" problem worse.

Fixes rust-lang#53843 (but introduces a bug that someone might notice).
bors added a commit that referenced this issue Dec 19, 2018
process nested obligations in autoderef

Fixes #53843.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants