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

Extended method lookup #37

Closed
wants to merge 2 commits into from
Closed

Extended method lookup #37

wants to merge 2 commits into from

Conversation

emberian
Copy link
Member

@emberian emberian commented Apr 7, 2014

When doing method lookup, consider all implementations from the crate the type
is defined in, not just inherent methods and in-scope traits.

@emberian
Copy link
Member Author

emberian commented Apr 7, 2014

cc @pcwalton, this touches resolve in ways I'm not entirely comfortable with.

to do the refactor, all dependent crates must now start importing this new
trait.

This restriction causes pain for all non-extension-method uses of traits. A
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific about exactly what you mean by "extension methods" and "non-extension methods"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extension method being a crate implementing a trait it defines for another crate's type. A non-extension method is everything that isn't one of those. Will clarify in text.

@chris-morgan
Copy link
Member

I love the sound of this; it fixes up one of the biggest problems that people have when starting with the language. On the other hand, it does make lookup more difficult to reason about. For myself, I very strongly hope that this is accepted.

What you describe with libterm has also been my experience with rust-http—I've been pushed away from using traits in certain places because of how much harder it would make it for others to use my library. Thus, some of the expressive power of Rust is lost to me because of its ergonomics. This proposal fixes that.

@lilyball
Copy link
Contributor

lilyball commented Apr 7, 2014

I agree with @chris-morgan. This does sound like a good proposal. My one worry is that a crate can no longer implement two traits that share a method on the same type and rely on scope to determine which method gets called. However, that seems like a rare edge case and UFCS should allow that to be handled.

@emberian
Copy link
Member Author

emberian commented Apr 7, 2014

@kballard that is a worrying case. I'm not sure if we can do anything sane
except just disallow calling the method in that case, except via UFCS.

On Mon, Apr 7, 2014 at 12:58 AM, Kevin Ballard notifications@github.comwrote:

I agree with @chris-morgan https://github.com/chris-morgan. This does
sound like a good proposal. My one worry is that a crate can no longer
implement two traits that share a method on the same type and rely on scope
to determine which method gets called. However, that seems like a rare edge
case and UFCS should allow that to be handled.


Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-39696267
.

http://octayn.net/

@lilyball
Copy link
Contributor

lilyball commented Apr 7, 2014

@cmr Yeah, that's what I meant. UFCS would be the only way to call it. We could emit a warning when this case is encountered, at least until UFCS exists. Of course, even without your proposed change this is still a problem because UFCS is the only way to call the method if you need both traits in scope.

Incidentally, there is a workaround, which is to write a wrapper method that's defined in terms of that specific trait.

fn workaround<T: Trait1>(obj: &T) {
    obj.overloaded_method()
}

@emberian
Copy link
Member Author

emberian commented Apr 7, 2014

Ah, nice workaround. No need to wait for UFCS in that case.

On Mon, Apr 7, 2014 at 1:28 AM, Kevin Ballard notifications@github.comwrote:

@cmr https://github.com/cmr Yeah, that's what I meant. UFCS would be
the only way to call it. We could emit a warning when this case is
encountered, at least until UFCS exists. Of course, even without your
proposed change this is still a problem because UFCS is the only way to
call the method if you need both traits in scope.

Incidentally, there is a workaround, which is to write a wrapper method
that's defined in terms of that specific trait.

fn workaround<T: Trait1>(obj: &T) {
obj.overloaded_method()}


Reply to this email directly or view it on GitHubhttps://github.com//pull/37#issuecomment-39697197
.

http://octayn.net/


# Summary

When doing method lookup, consider all implementations from the crate the type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this refers only to public/reachable traits, correct? It'd be a shame if private methods started conflicting with public methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the detailed description covers this, by saying "visible"

@SiegeLord
Copy link

It's unclear to me what the backwards compatibility hazard described is.

Take types A and B and a traits T1 and T2, with A implementing T1 and B implementing T2. We can easily refactor the behavior of A and B into a common trait and then implement T1 and T2 in terms of the common trait. Is some other refactoring being described here?

@emberian
Copy link
Member Author

emberian commented Apr 7, 2014

@SiegeLord Take a type A. Now you want to add a type B which is usable just like A is. You want to add a trait that both A and B implement, but you can't, because that'd require importing the trait to call the methods you used to be able to call.

A more concrete example arises in `libterm`. Ideally `AnsiTerminal` would be a
trait with `Win32Terminal` and `TerminfoTerminal` implementing it. Doing so,
however, affects every use of the crate. Were `libterm` marked stable,
introducing such an abstraction would be impossible. I worry that this will be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not strictly true. You can define inherent methods that delegate to identical trait methods (and I've done so in order to introduce traits in a backwards compatible way before).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that could work, I did not think of it. Wouldn't that require the
trait methods to be named differently?

On Mon, Apr 7, 2014 at 10:29 AM, Niko Matsakis notifications@github.comwrote:

In active/0000-less-trait-imports.md:

+one wants to call a trait-provided method on a type. This can become
+burdensome for crates which have many abstractions. Second, it causes a
+backwards compatibility hazard when factoring out behavior into a common
+trait. In order to do the refactor, all dependent crates must now start
+importing this new trait.
+
+This restriction causes pain for all non-extension-implementation uses of
+traits. An extension implementation is when a crate defines its own trait,
+but implements that trait for types in other crates. The less restrictive
+system proposed here is to instead use methods from an extension
+implementation only if that trait is in scope.
+
+A more concrete example arises in libterm. Ideally AnsiTerminal would be a
+trait with Win32Terminal and TerminfoTerminal implementing it. Doing so,
+however, affects every use of the crate. Were libterm marked stable,
+introducing such an abstraction would be impossible. I worry that this will be

This is not strictly true. You can define inherent methods that delegate
to identical trait methods (and I've done so in order to introduce traits
in a backwards compatible way before).


Reply to this email directly or view it on GitHubhttps://github.com//pull/37/files#r11345562
.

http://octayn.net/

@SiegeLord
Copy link

So the real backwards compatibility hazard is using inherent methods to begin with, rather than anything to do with traits per se. Once you do start using traits, it seems easy to maintain backwards compatibility. That said, it's not clear to me why this is not a solution:

mod version_1_0
{
    pub struct A;

    impl A
    {
        pub fn method(&self) { println!("A"); }
    }
}

mod version_1_1
{
    pub use version_1_0::A;

    pub struct B;

    pub trait T
    {
        fn method(&self) {}
    }

    impl T for A
    {
        fn method(&self)
        {
            self.method();
        }
    }

    impl T for B
    {
        fn method(&self) { println!("B"); }
    }
}


fn main()
{
    {
        use version_1_0::A;
        let a = A;
        a.method();
    }
    {
        use version_1_1::A;
        let a = A;
        a.method();
    }
    {
        use version_1_1::{A, B, T};
        let a = A;
        a.method();

        let b = B;
        b.method();
    }
}

@emberian
Copy link
Member Author

emberian commented Apr 7, 2014

@SiegeLord Well isn't that painful! You also have to do that to begin with. If you don't start by doing that, you're up a creek. It's also ugly as sin :(

It also doesn't address the problem of "Trait imports, trait imports everywhere." It exacerbates it.

@SiegeLord
Copy link

Absolutely. I think the annoyance of trait imports is the primary motivation for this RFC, in my eyes, and the backwards compatibility hazard is not really an issue.

I should note that removing old inherent methods and replacing them with a trait will not always be backwards compatible, e.g. consider this contrived example:

mod version_1_0
{
    struct A;

    impl A
    {
        fn method(&self) {}
    }

    trait T1
    {
        fn method(&self) {}
    }

    impl T1 for A {}
}

mod version_1_1
{
    struct A;

    trait T1
    {
        fn method(&self) {}
    }

    impl T1 for A {}

    trait T2
    {
        fn method(&self) {} // This is the old inherent method
    }

    impl T2 for A {}
}


fn main()
{
    use version_1_0::A;
    let a = A;
    A::method(a); // this won't work in version 1_1, will have to be (A: T2)::method(a);
}

I.e. moving the inherent method to a trait will change what type of disambiguation you will have to write (you'll have to disambiguate to a trait, vs disambiguating to an inherent implementation). Note that I'm assuming that there exists a syntax to call A's inherent method as a function (I don't see it in that RFC, but it seems useful to have).

Ultimately, I think one just has to be very cognizant about using inherent methods in public API.

@bstrie bstrie mentioned this pull request May 6, 2014
@brson
Copy link
Contributor

brson commented Jun 11, 2014

Per https://github.com/mozilla/rust/wiki/Meeting-weekly-2014-06-10, this is a real pain point, but we don't want to pursue solutions to it right now.

@brson brson closed this Jun 11, 2014
withoutboats pushed a commit to withoutboats/rfcs that referenced this pull request Jan 15, 2017
Fix links to poll method docs incorrectly going to Poll type
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 this pull request may close these issues.

None yet

7 participants