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

Permit impl methods whose bounds cannot be satisfied to have no body #20021

Open
nikomatsakis opened this issue Dec 19, 2014 · 14 comments
Open
Labels
A-trait-system Area: Trait system C-feature-request Category: A feature request, i.e: not implemented / a PR. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

There is a curious case with where clauses where sometimes we can show that a method in an impl could not possibly be called. This is because the impl has more precise information than the trait. Here is an example:

    trait MyTrait<T> {
        fn method(&self, t: &T) where T : Eq;
    }

    struct Foo;
    struct Bar; // note that `Bar` does not derive `Eq`

    impl MyTrait<Bar> for Foo {
        fn method(&self, t: &T) where Bar : Eq { // <-- `Bar : Eq` cannot be satisfied!
        }
    }

We should permit the method body to be omitted in such a case. As a workaround, once #20020 is fixed, I imagine it would be possible to write an impl like this:

impl MyTrait<Bar> for Foo {
    fn method(&self, t: &T) { // no where clause at all
        panic!("Bar : Eq could not be satisfied");
    }
}

However, it is unfortunate to require that of the user. For one thing, perhaps it happens later that an impl of Eq is added for Bar -- now we have this method hanging around that will panic. It'd be nice to detect that statically.

The plan then would be to permit:

impl MyTrait<Bar> for Foo {
    fn method(&self, t: &T); // <-- no body or where clauses needed
}

This serves as a declaration that you believe this method could never be called. At trans time, we will generate a body that simply does the equivalent of panic!("unsatisfiable methodmethodinvoked").

I plan to open an amendment to the where clause RFC describing this particular case.

@brson brson mentioned this issue Dec 19, 2014
7 tasks
@tomjakubowski
Copy link
Contributor

It would be helpful to mark (or even erase) these uncallable methods in the "Trait Implementations" section of the rustdocs for the type implementing the trait.

@jroesch
Copy link
Member

jroesch commented Dec 20, 2014

@tomjakubowski I think I am the one who will be working #20020 and most likely this issue unless @nikomatsakis beats me to it. I will try to keep that in mind :).

@bltavares
Copy link
Contributor

Triaging this issue.

#20020 has already been closed, but yet we can't use unimplemented methods without where.

This is the code I used to see if it was still an issue:

trait MyTrait<T> {
    fn method(&self, t: &T) where T : Eq;
}

struct Foo;
struct Bar; // note that `Bar` does not derive `Eq`

impl MyTrait<Bar> for Foo {
    fn method(&self, t: &Bar);
}
fn main() {}

Rust version

rustc 1.7.0-nightly (b4707ebca 2015-12-27)

@apasel422
Copy link
Contributor

In a similar situation, despite being object-safe, the following trait cannot be implemented for [T]:

trait Foo {
    fn foo(self);
}

impl<T> Foo for [T] {
    fn foo(self) { unimplemented!() }
    //~^ ERROR the trait `core::marker::Sized` is not implemented for the type `[T]`
}

The foo method has an implicit where Self: Sized as a result of taking self by value. Adding an explicit where Self: Sized bound would help if this issue is fixed, but the fix has to be more general than method-level where clauses.

As an unsatisfactory workaround, one can default foo as follows:

trait Foo {
    fn foo(self) where Self: Sized { unimplemented!() }
}

impl<T> Foo for [T] {}

This allows the impl to compile, but is nonsensical because it is exactly the Sized types that we want to provide a foo method.

@nikomatsakis
Copy link
Contributor Author

On Sun, Jan 10, 2016 at 09:10:54AM -0800, Andrew Paseltiner wrote:

In a similar situation, despite being object-safe, the following trait cannot be implemented for [T]:

FWIW, this is intentional, as we still aim to allow DST moves at some
point (or at least I do). So I would not want to make it possible to
implement that trait with [T] until that point is settled.

@apasel422
Copy link
Contributor

@nikomatsakis Ah, I hadn't considered that. Are you still planning to amend the where clause RFC for this issue?

@nikomatsakis
Copy link
Contributor Author

This is also arising in the context of rust-lang/rfcs#1216. Recent PR rust-lang/rfcs#1699 is also relevant.

The specific issue is that the ! type allows us to identify cases where methods could not ever really be called (e.g., because the type of one of their parameters is !, and there can never be an instance of such a type). It'd be nice to omit such methods.

@brson brson added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2016
@brson
Copy link
Contributor

brson commented Aug 25, 2016

Nominating for triage. Add some tags please. Last work item in #17657

@glaebhoerl
Copy link
Contributor

@nikomatsakis These two are qualitatively different things though. In the case that this issue is about, no call to the method could ever type-check. In the case that rust-lang/rfcs#1699 is about, a call would type-check just fine, it's just that the method body would never actually be entered at runtime.

@nrc nrc added T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 25, 2016
@nikomatsakis
Copy link
Contributor Author

Discussed in @rust-lang/lang meeting. A couple of notes:

  • We should be careful here -- this is a form of negative reasoning, so we have to apply the "coherence criteria" to avoid subtle breakage where people get to elide methods because a trait is unimplemented, but then a trait is added in the future.
  • Agreed, @glaebhoerl, there is a distinction between this and Allow uncallable method impls to be omitted rfcs#1699.

triage: P-medium

@rust-highfive rust-highfive added P-medium Medium priority and removed I-nominated labels Aug 25, 2016
@nikomatsakis nikomatsakis added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed P-medium Medium priority E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Aug 25, 2016
@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Aug 25, 2016

Ah, reviewing this though -- I'm not sure we ever settled on a syntax. It may be that we need an RFC here? Should review the existing RFC :)

EDIT: Just did, it offers no guidance here.

@Mark-Simulacrum Mark-Simulacrum added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Jul 22, 2017
@steveklabnik
Copy link
Member

Triage: not aware of any specific updates here.

@LukasKalbertodt
Copy link
Member

I recently asked about exactly this on StackOverflow where I was referred to this issue. This issue is very relevant for the design of a library I'm developing right now, so I'm very interested in resolving this.

Here is a short real life example:

I'm working on a polygon mesh processing library that abstracts over meshes. The important part for this example is that there are some mesh data structures that are restricted to triangular faces while some other data structures support all kinds of faces (in particular, mixed faces in one instance).

// Dummy
struct Vertex;


// Mesh kind marker types. Can be replaced with enum once const generic 
// lands. This trait is "sealed", i.e. it is only implemented by these two
// types.
trait FaceKind {}
enum TriFace {}
enum PolyFace {}
impl FaceKind for TriFace {}
impl FaceKind for PolyFace {}


trait Mesh {
    type FaceKind: FaceKind;
    
    // All meshes can add triangles, but only some meshes can add 
    // arbitrary faces.
    fn add_triangle(&mut self, vertices: &[Vertex; 3]);
    fn add_face(&mut self, vertices: &[Vertex])
    where
        Self: Mesh<FaceKind = PolyFace>;
}


struct MyTriMesh {}
impl Mesh for MyTriMesh {
    type FaceKind = TriFace;
    
    fn add_triangle(&mut self, vertices: &[Vertex; 3]) {
        // stuff
    }
    
    fn add_face(&mut self, vertices: &[Vertex]) {
        unreachable!() // <-- this is annoying and not robust
    }
}

This is only a small part of my API: there are many other places where this is relevant.

This thread has been rather quiet and it doesn't seem like a lot has happened in this regards in the last 5 years. So: what would be next steps to resolve this issue?

Probably writing a new RFC, right? Or is there some other work on the way that will have significant impact on this issue (e.g. the chalkification) and thus writing an RFC now doesn't make sense? If not and someone would be willing to start writing an RFC, what are important topics to discuss in the RFC? @nikomatsakis already mentioned something about negative reasoning and coherence. Something else?

@Jules-Bertholet
Copy link
Contributor

Jules-Bertholet commented Sep 26, 2022

(Copying what I said in #48214 (comment))

I would actually argue against this feature, for the following reason: I believe that Self bounds (or bounds on type parameters of the trait) on individual trait methods with no default implementation are an anti-pattern. Such bounds should be discouraged, in favor of separate traits. This code is questionable because it means that if type A starts implementing trait T, that can suddenly invalidate its implementation of trait U because U had a where Self: T trait method with no default impl. It's also a problem for traits wanting to take advantage of expansions to object safety in future versions of the language. I don't think complicated negative-reasoning rules are worth it just to accommodate this questionable pattern.

But maybe there is a use-case I am missing, where separate traits are not adequate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system C-feature-request Category: A feature request, i.e: not implemented / a PR. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests