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

Can not impl trait from another crate for Vec<T> with T from this crate #24745

Closed
SimonSapin opened this issue Apr 23, 2015 · 16 comments
Closed
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-type-system Area: Type system C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Test case:

a.rs

pub trait Foo {}

b.rs

extern crate a;
struct Bar;
impl a::Foo for Vec<Bar> {}
fn main() {}

In rustc 1.1.0-nightly (90cc830 2015-04-22) (built 2015-04-23):

$ rustc --crate-type lib a.rs
$ rustc -L . a.rs
a.rs:3:1: 3:28 error: the impl does not reference any types defined in this crate; only traits defined in the current crate can be implemented for arbitrary types [E0117]
a.rs:3 impl b::Foo for Vec<Bar> {}
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

Note the impl does not reference any types defined in this crate. This is not true, Bar is defined is this crate.

This indicates a bug either in the coherence rules (this program should be valid and the error not be shown) or in the error message, which is misleading.

CC @nikomatsakis

@SimonSapin
Copy link
Contributor Author

In either case, the work-around / fix is to make a new type that wraps Vec<Bar> and implement the trait on that.

@ftxqxd
Copy link
Contributor

ftxqxd commented Apr 23, 2015

I think this is an intentional result of #23086 (RFC 1023).

@Diggsey
Copy link
Contributor

Diggsey commented Apr 24, 2015

I think the rationale is so that Vec<_> could be made to implement the trait later on without breaking backwards compatibility (it would conflict with your implementation of the trait otherwise).

@SimonSapin
Copy link
Contributor Author

@Diggsey by “later on”, do you mean “in a future version of the crate that defines Vec?” Ok, that’s understandable. In that case, this issue is about reformulating the error message.

@Ryman
Copy link
Contributor

Ryman commented Apr 25, 2016

This error now provides a link to a detailed explanation of E0117 so this issue might be considered fixed?

@GildedHonour
Copy link

what's the reason for this restriction in the 1st place?

@zbraniecki
Copy link

An example use case for this:

I have a library that implements an AST, Parser and Serializer for a given DSL. I'd like to keep it lean because that's what people will bundle.

I want to have a separate crate that adds serde and allows to serialize and deserialize the AST to/from json.

I think that this limitation prevents me from being able to do that.

@sinkuu
Copy link
Contributor

sinkuu commented Dec 30, 2016

If this is allowed, adding implss become a breaking-change, because any other crate could potentially have an conflicting impl. We need to bump minor version (or major version if >1.0.0) of a crate for every addition of trait impl. Moreover, as standard library is generally not allowed to make breaking-change, we can't implement existing std traits for existing std types anymore.

if your concern is the additional dependencies, you can use features.

@zbraniecki
Copy link

Sure, features are one option, but if I don't want to mess with the original package and keep it clean, I just end up with something like this:

https://github.com/zbraniecki/fluent-json-rs/blob/99a2b3e08306fe9b298769e0fbe3abf458e9c69e/src/lib.rs

which is a clone of https://github.com/projectfluent/fluent-rs/blob/70b5e8d387f3b836032b42c997c359263b21c8d0/src/syntax/ast.rs

but with From/Into. (to serialize FTL->AST->JSON and JSON->AST->FTL back).

I'm wondering if it would be possible to write code to automate the creation of a clone of a struct/enum from another crate. Then one could add Serialize/Deserialize on those local struct/enums.

Another option would be to mark structs/enums as extensible by other crates, which would allow the author to decide that he wants to let others impl features for them. That would work better in my case, because it would remove the step of converting from fluent::syntax::ast::* into fluent_json::ast::* back and forth.

@oberien
Copy link
Contributor

oberien commented Mar 24, 2017

I think the rationale is so that Vec<_> could be made to implement the trait later on without breaking backwards compatibility (it would conflict with your implementation of the trait otherwise).

With specialization this is not an issue anymore. If some library implements Foo for Vec<T> and I implement Foo for Vec<Bar> then with specialization both can co-exist and mine will be executed. Therefore I think that allowing implementations of traits for a type not defined in your crate but with a generic parameter from your trait could be allowed again. Do I miss another problem?

@nikomatsakis
Copy link
Contributor

This indicates a bug either in the coherence rules (this program should be valid and the error not be shown) or in the error message, which is misleading.

The bug is in the error message, which I agree is misleading. It's also true that it might be nice to rejigger the rules in this area, but that's a fairly complex undertaking.

@nikomatsakis nikomatsakis added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 24, 2017
@nikomatsakis
Copy link
Contributor

Here is a standalone test case for the same issue:

use std::fmt;
struct Bar;
impl fmt::Display for Vec<Bar> { } 
fn main() { }

Generates:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> <anon>:3:1
  |
3 | impl fmt::Display for Vec<Bar> { } 
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl doesn't use types inside crate
  |
  = note: the impl does not reference any types defined in this crate
  = note: define and implement a trait or new type instead

I'm not sure of the ideal wording, but one idea would be to have it highlight the "foreign types" (in this example, there is only one, the Vec type):

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> <anon>:3:1
  |
3 | impl fmt::Display for Vec<Bar> { } 
  |                                     ^^^^ not defined in the current crate
  |
  = note: define and implement a trait or new type instead

I am not sure if that last note is all that useful either. Maybe. It seems very jargon heavy.

Anyway, the trick would be getting the highlight to exclude the Bar. It may also be worth trying to comment on how Bar doesn't count because it is "under" Vec.

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 22, 2017
@nayato
Copy link

nayato commented Sep 12, 2017

Would it make sense to consider type parameters' boundaries to allow anchoring / scoping impls for non-local types? e.g.

use std::fmt;
struct Bar;
trait LocalMarker {}
impl LocalMarker for Bar {}
impl<T: LocalMarker> fmt::Display for Vec<T> { } 
fn main() { }

Centril added a commit to Centril/rust that referenced this issue Oct 29, 2019
Call out the types that are non local on E0117

CC rust-lang#24745.
Centril added a commit to Centril/rust that referenced this issue Oct 29, 2019
Call out the types that are non local on E0117

CC rust-lang#24745.
Centril added a commit to Centril/rust that referenced this issue Oct 29, 2019
Call out the types that are non local on E0117

CC rust-lang#24745.
@estebank
Copy link
Contributor

The current output for this case is:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:3:1
  |
3 | impl fmt::Display for Vec<Bar> { } 
  | ^^^^^^^^^^^^^^^^^^^^^^--------
  | |                     |
  | |                     `std::vec::Vec` is not defined in the current crate
  | impl doesn't use only types from inside the current crate
  |
  = note: define and implement a trait or new type instead

For the last comment:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> src/main.rs:5:1
  |
5 | impl<T: LocalMarker> fmt::Display for Vec<T> { } 
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^------
  | |                                     |
  | |                                     `std::vec::Vec` is not defined in the current crate
  | impl doesn't use only types from inside the current crate
  |
  = note: define and implement a trait or new type instead

For the original report:

error[E0117]: only traits defined in the current crate can be implemented for arbitrary types
 --> b.rs:3:1
  |
3 | impl a::Foo for Vec<Bar> {}
  | ^^^^^^^^^^^^^^^^--------
  | |               |
  | |               `std::vec::Vec` is not defined in the current crate
  | impl doesn't use only types from inside the current crate
  |
  = note: define and implement a trait or new type instead

@nikomatsakis is there anything else we should do here?

@Arnavion
Copy link

Arnavion commented Nov 18, 2019

I know what the issue is and what the fix is, but I don't understand what "define and implement a trait or new type instead" is trying to tell me.

  • "define and implement a trait or new type" should presumably be "define and implement a new trait, or implement the trait on a newtype". As it's written the suggestion doesn't make sense.

  • Even with that alternative wording, it implies either option is feasible, but realistically creating a brand new trait is unlikely to be what the user wants. It might be worth only mentioning the newtype option, or at least listing it first.

  • "note: define ..." should probably be "note: consider defining ...", because actually creating a newtype or new trait may not fit with the rest of the user's program. Just "define ... instead" implies the compiler's suggestion is a drop-in replacement.

So with all that, should it instead be "consider defining a newtype and implement the trait on it instead" ?

@estebank estebank added the D-papercut Diagnostics: An error or lint that needs small tweaks. label Nov 18, 2019
@SimonSapin
Copy link
Contributor Author

The original test case now compiles, I believe this was fixed with https://blog.rust-lang.org/2020/01/30/Rust-1.41.0.html#relaxed-restrictions-when-implementing-traits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-type-system Area: Type system C-enhancement Category: An issue proposing an enhancement or a PR with one. D-papercut Diagnostics: An error or lint that needs small tweaks. 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