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 in method resolution through Deref #17594

Closed
sfackler opened this issue Sep 27, 2014 · 13 comments
Closed

Regression in method resolution through Deref #17594

sfackler opened this issue Sep 27, 2014 · 13 comments
Labels
A-resolve Area: Name resolution E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@sfackler
Copy link
Member

This should compile but no longer does:

use std::cell::RefCell;

trait Foo {
    fn foo(&self) {}
}

impl<T> Foo for T where T: Clone {}

struct Bar;

impl Bar {
    fn foo(&self) {}
}

fn main() {
    let b = RefCell::new(Bar);
    b.borrow().foo()
}
test.rs:17:5: 17:21 error: the trait `core::clone::Clone` is not implemented for the type `core::cell::Ref<'_,Bar>`
test.rs:17     b.borrow().foo()
               ^~~~~~~~~~~~~~~~

Note that calling Bar.foo() directly works.

@sfackler sfackler added I-wrong A-resolve Area: Name resolution labels Sep 27, 2014
@sfackler
Copy link
Member Author

cc @pcwalton I'm assuming this was introduced in #17464.

sfackler added a commit to sfackler/rust-postgres that referenced this issue Sep 27, 2014
@bkoropoff
Copy link
Contributor

This works fine if you change the call to:

    (*b.borrow()).foo()

#17464 seems to indicate that this is intentional.

@sfackler
Copy link
Member Author

I don't think that applies. If Foo was actually implemented for RefMut<Bar>, then I'd expect that version of foo to be called, but it isn't.

@bkoropoff
Copy link
Contributor

So it should back off and continue dereferencing instead? That would certainly be more intuitive and convenient behavior.

@sfackler
Copy link
Member Author

That's what I would expect.

@bkoropoff
Copy link
Contributor

This example seems to have worked before due to the special treatment of inherent methods. It's possible to construct an example that does not use them that fails in a similar way even prior to #17464:

trait Foo {
    fn foo(&self) { fail!() }
}

impl<'a, T> Foo for &'a T where T: Clone {}

trait UsableFoo {
    fn foo(&self) {}
}

struct Bar;

impl UsableFoo for Bar {}

fn main() {
    let b = &&Bar;
    b.foo();
}
test.rs:17:5: 17:12 error: the trait `core::clone::Clone` is not implemented for the type `Bar`
test.rs:17     b.foo();
               ^~~~~~~
error: aborting due to previous error

@sfackler
Copy link
Member Author

Interesting! I wonder if this is a duplicate, then.

@aturon
Copy link
Member

aturon commented Sep 30, 2014

@sfackler I think the problem here is partly that we still don't have full trait reform -- in other words, the trait matching code sees the blanket impl and assumes it applies, without first checking the Clone bound. I believe that once @nikomatsakis lands the rest of trait reform, this problem will go away.

@nikomatsakis
Copy link
Contributor

This is fallout (intensional) from the change to out inherent and trait methods on equal footing. However @aturon is correct that once I integrated conditional/multi dispatch into method resolution (next patch...) this should work again.

@sfackler
Copy link
Member Author

sfackler commented Oct 1, 2014

Should I close this as a dup of the trait reform issue?

@nikomatsakis
Copy link
Contributor

Closing as dup of #5527

@nikomatsakis nikomatsakis mentioned this issue Oct 2, 2014
5 tasks
nikomatsakis added a commit to nikomatsakis/rust that referenced this issue Oct 22, 2014
@nikomatsakis nikomatsakis reopened this Oct 22, 2014
@nikomatsakis
Copy link
Contributor

I'm opening this and flagging as needs test. It works now.

@nikomatsakis nikomatsakis added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 22, 2014
@nikomatsakis
Copy link
Contributor

(Also, PR #18224 adds a test)

@bors bors closed this as completed in 96991e9 Oct 23, 2014
lnicola pushed a commit to lnicola/rust that referenced this issue Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Name resolution E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants