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

Improve the long explanation of E0207. #33692

Merged
merged 1 commit into from
May 25, 2016
Merged

Conversation

nham
Copy link
Contributor

@nham nham commented May 17, 2016

The previous explanation does not seem to explain what it means for an
implementation parameter to be used or unused. The new explanation lists
the three ways specific ways by which an impl parameter becomes constrained
(taken from RFC 447).

This also adds a link to RFC 447.

The explanation has two different examples. The first is adapted from RFC 447,
and shows an instance of E0207 on a impl for a type. The second one is a trait
impl example adapted from issue #22834.

Closes #33650

cc #32777

r? @GuillaumeGomez

fn get(&self) -> usize;
struct Foo;

impl<T:Default> Foo {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after colon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, thanks.

@GuillaumeGomez
Copy link
Member

What generally bugs me in here is that it doesn't follow the waiting for getting merged RFC about error explanations.

@nham
Copy link
Contributor Author

nham commented May 18, 2016

@GuillaumeGomez can you be more specific about how this explanation doesn't match? I just added a little bit more description to my first example, but other than that I'm not sure I understand what you would like to see changed.

@GuillaumeGomez
Copy link
Member

Your changes are great, the point is not here. But currently, we're trying to uniformize the error explanations "template" by forcing people to follow the RFC I linked. This error explanation doesn't, and since you updated it, could you make fit to it please? 😺

@nham
Copy link
Contributor Author

nham commented May 18, 2016

The problem is I've read the RFC multiple times and I don't understand how my explanation fails to adhere to the template. The only thing I can see is that my explanation does not literally contain the phrase "Example of erroneous code:", but I don't see how inserting this exact phrase improves the explanation.


Here's an attempt at trying to match my explanation to the template:

Error description

Any type parameter or lifetime parameter of an impl must meet at least one of
the following criteria:

  • it appears in the self type of the impl
  • for a trait impl, it appears in the trait reference
  • it is bound as an associated type

Minimal example 1

For example, when implementing a method for a struct Foo, the following code
leads to an error:

struct Foo;

impl<T: Default> Foo {
    fn get(&self) -> T {
        <T as Default>::default()
    }
}

Error explanation 1

The problem is that the parameter T does not appear in the self type (Foo)
of the impl. In this case, we can fix the error by moving the type parameter
from the impl to the method get:

How to fix the problem

struct Foo;

impl Foo {
    fn get<T: Default>(&self) -> T {
        <T as Default>::default()
    }
}

Minimal example 2

As another example, suppose we have a Maker trait and want to establish a
type FooMaker that makes Foos:

trait Maker {
    type Item;
    fn make(&mut self) -> Self::Item;
}

struct Foo<T> {
    foo: T
}

struct FooMaker;

impl<T: Default> Maker for FooMaker {
    type Item = Foo<T>;

    fn make(&mut self) -> Foo<T> {
        Foo { foo: <T as Default>::default() }
    }
}

Error explanation 2

This fails to compile because T does not appear in the trait or in the
implementing type.

How to fix the problem, option 1

One way to work around this is to introduce a phantom type parameter into
FooMaker, like so:

use std::marker::PhantomData;

trait Maker {
    type Item;
    fn make(&mut self) -> Self::Item;
}

struct Foo<T> {
    foo: T
}

struct FooMaker<T> {
    phantom: PhantomData<T>,
}

impl<T: Default> Maker for FooMaker<T> {
    type Item = Foo<T>;

    fn make(&mut self) -> Foo<T> {
        Foo {
            foo: <T as Default>::default(),
        }
    }
}

How to fix the problem, option 2

Another way is to do away with the associated type in Maker and use an input
type parameter instead:

trait Maker<Item> {
    fn make(&mut self) -> Item;
}

struct Foo<T> {
    foo: T
}

struct FooMaker;

impl<T: Default> Maker<Foo<T>> for FooMaker {
    fn make(&mut self) -> Foo<T> {
        Foo { foo: <T as Default>::default() }
    }
}

Additional information

For more information, please see RFC 447.


Could you explain specifically what changes you think need to be made?

@arielb1
Copy link
Contributor

arielb1 commented May 18, 2016

Having the error message in a comment would be nice.

@nham
Copy link
Contributor Author

nham commented May 19, 2016

@arielb1 Thanks, added.

@GuillaumeGomez
Copy link
Member

Could you explain specifically what changes you think need to be made?

Sure, you just forgot a few little things. For example, before the very first you need to add "Example of erroneous code:".

In erroneous codes, you need to add the compiler error in a comment.

In other code examples, you need to add little comments to explain what you did.

@nham
Copy link
Contributor Author

nham commented May 20, 2016

I've added Markdown headers so that the explanation is structured like this:

[Error overview]

### Error example 1

[Error example code along with a discussion of the problem and how to fix it]

### Error example 2

[Error example code along with a discussion of the problem and how to fix it]

### Additional information

[See RFC 447 blah blah]

I think the headers nicely organize this explanation. You can see the HTML version here Without the headers, the explanation is 120 lines long, which is lengthy enough to make headers work here, IMO.

@nham
Copy link
Contributor Author

nham commented May 20, 2016

@GuillaumeGomez

Sure, you just forgot a few little things. For example, before the very first you need to add "Example of erroneous code:".

I am not sold on doing this yet. I do see your point that more structure is needed in the explanation. I have tried to explore an alternative above by using headings.

In erroneous codes, you need to add the compiler error in a comment.

Good point! Sorry I missed this in the RFC. This was already addressed above.

In other code examples, you need to add little comments to explain what you did.

Thanks, another good suggestion. I've added these in my last commit. Let me know if you think that's enough explanation.

@nham nham force-pushed the fix_E0207 branch 2 times, most recently from 4b94526 to 44c1de6 Compare May 21, 2016 00:36
@GuillaumeGomez
Copy link
Member

It's really good! The "erroneous code example" sentence is still missing but considering the quality of the rest of the error explanation, I think we can exceptionally get over it. Thanks a lot for your work @nham!

Now it just remains one last thing: can you squash your commits please? 😃

The previous explanation does not seem to explain what it means for an
implementation parameter to be used or unused. The new explanation lists
the three ways specific ways by which an impl parameter becomes constrained
(taken from RFC 447).

This also adds a link to RFC 447.

The explanation has two different examples. The first is adapted from RFC 447,
and shows an instance of E0207 on a impl for a type. The second one is a trait
impl example adapted from issue rust-lang#22834.

Closes rust-lang#33650
@nham
Copy link
Contributor Author

nham commented May 23, 2016

Sure! Squashed

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 23, 2016

📌 Commit 7d78436 has been approved by GuillaumeGomez

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 24, 2016
Improve the long explanation of E0207.

The previous explanation does not seem to explain what it means for an
implementation parameter to be used or unused. The new explanation lists
the three ways specific ways by which an impl parameter becomes constrained
(taken from RFC 447).

This also adds a link to RFC 447.

The explanation has two different examples. The first is adapted from RFC 447,
and shows an instance of E0207 on a impl for a type. The second one is a trait
impl example adapted from issue rust-lang#22834.

Closes rust-lang#33650

cc rust-lang#32777

r? @GuillaumeGomez
bors added a commit that referenced this pull request May 24, 2016
Rollup of 7 pull requests

- Successful merges: #33692, #33759, #33779, #33781, #33797, #33810, #33832
- Failed merges:
@bors bors merged commit 7d78436 into rust-lang:master May 25, 2016
Centril added a commit to Centril/rust that referenced this pull request Apr 29, 2019
Clarify the short explanation of E0207

After being greatly improved in rust-lang#33692, https://doc.rust-lang.org/error-index.html#E0207 uses terminology from the related RFC, which is different from the [reference](https://doc.rust-lang.org/reference/items/implementations.html), e.g. "self type" instead of "implementing type" and "trait reference" instead of "implemented trait".

It probably makes no difference to an experienced reader, but for a newbie like me it added to the confusion as you can't look up the definition of the terms being used and can't be sure you've guessed correctly...

I would also move the link to RFC to the top, as it seems to be the only doc that attempts to explain what the third criteria entails), but it seems to go against the accepted style.
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.

5 participants