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

Regression around Trait::method -> Self #18209

Closed
wycats opened this issue Oct 21, 2014 · 10 comments · Fixed by #18235
Closed

Regression around Trait::method -> Self #18209

wycats opened this issue Oct 21, 2014 · 10 comments · Fixed by #18235
Assignees

Comments

@wycats
Copy link
Contributor

wycats commented Oct 21, 2014

I have this trait definition:

pub trait LoadableMessage {
    fn load<R: Reader>(reader: &mut R) -> IoResult<Self> {
        let mut stream = InputStream::new();
        LoadableMessage::load_from_stream(&mut stream)
    }
}

And this usage:

pub fn load<'a, M: LoadableMessage, R: Reader>(reader: &mut R) -> IoResult<M> {
    LoadableMessage::load(reader)
}

And I get this error upon compilation:

-gnu/release/deps --extern nix=/home/wycats/Code/skylight-rust/target/x86_64-unknown-linux-gnu/release/deps/libnix-13dc7f065ef11e3c.rlib`
/home/wycats/Code/skylight-rust/libs/buffoon/src/lib.rs:19:5: 19:34 error: mismatched types: expected `core::result::Result<M,std::io::IoError>`, found `core::result::Result<Self,std::io::IoError>` (expected type parameter, found Self)
/home/wycats/Code/skylight-rust/libs/buffoon/src/lib.rs:19     LoadableMessage::load(reader)
                                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This was compiling recently (I believe last week), but does not compile with today's nightly.

@nrc
Copy link
Member

nrc commented Oct 21, 2014

cc me

@nrc
Copy link
Member

nrc commented Oct 21, 2014

This is a weird bug: here is a minimal test case:

pub trait Foo {
    fn load_from() -> Box<Self>;
    fn load() -> Box<Self> {
        Foo::load_from()
    }
}

pub fn load<M: Foo>() -> Box<M> {
    Foo::load()
}

but, replace Foo::load_from() by fail!(); and this compiles just fine. And yet the error message is on the call to Foo::load() (and commenting that out, this compiles fine too).

@nrc
Copy link
Member

nrc commented Oct 21, 2014

And calling load_from directly works too - it seems to need the default method in there

@nrc
Copy link
Member

nrc commented Oct 22, 2014

The regressing patch seems to be ce342f5. Which is ... impossible? It only touches files in trans and has nothing to do with type inference. But, I have triple checked this - it really seems to be this commit which regresses the reduced test case.

@nrc
Copy link
Member

nrc commented Oct 22, 2014

And of course patching without that commit does not fix things, so I was right about it being impossible. Now I am just confused about my bisection.

@nrc nrc self-assigned this Oct 22, 2014
@nrc
Copy link
Member

nrc commented Oct 22, 2014

OK, now I actually have a sensible candidate: 2c0f876

@nrc
Copy link
Member

nrc commented Oct 22, 2014

This is probably a dup of #18187

@nrc
Copy link
Member

nrc commented Oct 22, 2014

So, this (https://github.com/nick29581/rust/compare/rust-lang:master...nick29581:infer-bug?expand=1) fixes the problem, but I suspect it is not the right fix. I imagine the candidates.ambiguous = true; (probably, perhaps also the shallow_resolve, if that has side effects) should be moved out of this method some how. But I'm not clear on exactly how this should work. @nikomatsakis help?

@nikomatsakis
Copy link
Contributor

Interesting, I'll look.

@nikomatsakis
Copy link
Contributor

OK, so, what's happening is this:

  1. When checking the default method definition, we see a call to $0::load_from(), where we don't know what $0 is. We look around and find only the where clause Self : Foo in scope, so we decide to use that.
  2. Because $0 : Foo does not contain any type parameters, we place it in the global result cache.
  3. Later, we find the result in the cache and hence do not search.

This is why the bug goes away when you remove the definition. It would also go away if there were at most one impl.

I'm debating what the best fix is. Clearly, when the result is due to a where clause, we should place the result in the local cache. That would fix the problem. I could also imagine that we might not want to employ a where clause when there are inference variables -- but then again, why not? If there are no impls, it's the only plausible way to resolve this trait obligation.

@nikomatsakis nikomatsakis assigned nikomatsakis and unassigned nrc Oct 22, 2014
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Oct 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants