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

Using variant on self type with path Self::Variant causes subpar diagnostic #52118

Closed
andradei opened this issue Jul 6, 2018 · 7 comments · Fixed by #61682
Closed

Using variant on self type with path Self::Variant causes subpar diagnostic #52118

andradei opened this issue Jul 6, 2018 · 7 comments · Fixed by #61682
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@andradei
Copy link

andradei commented Jul 6, 2018

The snippet below fails to compile with the following messages of each enum variant:

pub enum State {
    Init,
    NewProject,
    NewTask,
    NewEvent,
}

impl State {
    pub fn commands<'a>(&self) -> usize {
        match self {
            Self::Init => 1,
            Self::NewProject | Self::NewTask | Self::NewEvent => 2,
            _ => 0,
        }
    }
}

fn main() {}
error[E0599]: no variant named `Init` found for type `State` in the current scope
  --> src/main.rs:11:13
   |
1  | pub enum State {
   | -------------- variant `Init` not found here
...
11 |             Self::Init => 1,
   |             ^^^^^^^^^^ variant not found in `State`
   |
   = note: did you mean `variant::Init`?

The solution is to replace Self::Init with explicit type State::Init. But the issues are:

  • Error E0599 doesn't explain the actual problem (Use of Self instead of the explicit type)
  • The message no variant named "Init" found for type "State" in the current scope is misleading
  • note: did you mean "variant::Init"? is also a little misleading. A phrase like note: replace "Self::Init" with "State::Init" is much clearer

And finally:

  • Why isn't Self:: allowed inside impl MyEnum blocks?
@ExpHP
Copy link
Contributor

ExpHP commented Jul 6, 2018

  • Why isn't Self:: allowed inside impl MyEnum blocks?

It does, the issue here is rather that type aliases (which includes Self) don't currently expose enum variants.

type Alias = MyEnum;
fn foo() {
    let x = Alias::Init; // this also fails
}

This has been a longstanding papercut in the language, but there are plans to fix it! An RFC was accepted back in February, and from what I can tell on the tracking issue, the feature is currently waiting for somebody to pick it up and start working on it.

@ExpHP
Copy link
Contributor

ExpHP commented Jul 6, 2018

note: did you mean "variant::Init"?

O_o

...I will agree it's quite possible that, even in the interim, the error message could probably use work.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 6, 2018

I think the "variant" part is fixed on nightly

@estebank
Copy link
Contributor

estebank commented Jul 7, 2018

@oli-obk is right:

error[E0599]: no variant named `Init` found for type `State` in the current scope
  --> src/main.rs:11:13
   |
1  | pub enum State {
   | -------------- variant `Init` not found here
...
11 |             Self::Init => 1,
   |             ^^^^^^^^^^ variant not found in `State`
   |
   = note: did you mean `State::Init`?

The diagnostic still needs to identify that we're referring to a variant through Self and 1) not point at State as that is completely misleading 2) change the note to be a suggestion, as it is very likely correct in this case in particular and 3) change the primary label to read something along the lines of "can't access any variant for enum State using Self". For the last point, probably adding a note with more detail and keeping the label short ("can't access variants of enum Self points to") might be best.

error[E0599]: no variant named `Init` found for type `State` in the current scope
  --> src/main.rs:11:13
   |
11 |             Self::Init => 1,
   |             ^^^^^^^^^^ can't access variant `State::Init` through `Self`
   |
   = note: until <https://github.com/rust-lang/rust/issues/49683> is implemented, you can't refer to enum variants using the local alias `Self`
help: instead of using `Self`, use the canonical path for the variant instead:
   |
11 |             State::Init => 1,
   |             ^^^^^^^^^^^


Then again, one #49683 is completed, the suggestion will no longer be needed.

@estebank estebank added the A-diagnostics Area: Messages for errors, warnings, and lints label Jul 7, 2018
@jkordish jkordish added C-enhancement Category: An issue proposing an enhancement or a PR with one. A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 11, 2018
@estebank
Copy link
Contributor

The current output is correct, if verbose:

error: enum variants on type aliases are experimental
  --> src/main.rs:13:13
   |
13 |             Self::Init => 1,
   |             ^^^^^^^^^^
   |
   = help: add `#![feature(type_alias_enum_variants)]` to the crate attributes to enable

error: enum variants on type aliases are experimental
  --> src/main.rs:14:13
   |
14 |             Self::NewProject | Self::NewTask | Self::NewEvent => 2,
   |             ^^^^^^^^^^^^^^^^
   |
   = help: add `#![feature(type_alias_enum_variants)]` to the crate attributes to enable

error: enum variants on type aliases are experimental
  --> src/main.rs:14:32
   |
14 |             Self::NewProject | Self::NewTask | Self::NewEvent => 2,
   |                                ^^^^^^^^^^^^^
   |
   = help: add `#![feature(type_alias_enum_variants)]` to the crate attributes to enable

error: enum variants on type aliases are experimental
  --> src/main.rs:14:48
   |
14 |             Self::NewProject | Self::NewTask | Self::NewEvent => 2,
   |                                                ^^^^^^^^^^^^^^
   |
   = help: add `#![feature(type_alias_enum_variants)]` to the crate attributes to enable

It could maybe suggest using the correct type name, but that might not be completely worth it (anyone using a Self path would already know enough about the type system to figure out how to solve it).

@estebank estebank changed the title Compiler error message could be clearer: did you mean variant::Init? Using variant on self type with path Self::Variant causes subpar diagnostic Apr 29, 2019
@andradei
Copy link
Author

andradei commented Apr 30, 2019

(anyone using a Self path would already know enough about the type system to figure out how to solve it)

Where is the line that separates "good compiler messages" and "user must know the tool" drawn?
Since the day I opened this issue I've learned enough of the type system to know why Self isn't supported in the way I tried to use. The old compiler message, though not helpful to complete beginners like I was back then, today is clear enough to me.

Maybe this issue should be closed, unless there is an intention to either:

  • Support exposing enum variants in Self for enum impl blocks
  • Improve the compiler error message

The current error message is great:

help: add `#![feature(type_alias_enum_variants)]` to the crate attributes to enable

error: enum variants on type aliases are experimental

That, combined with proper knowledge of the type system seems like the best approach to me.

I don't know if I should close the issue, so I'll let someone with better understanding of the project decisions do it.

@estebank
Copy link
Contributor

Where is the line that separates "good compiler messages" and "user must know the tool" drawn?

Personally I push as far towards "cater for newcomers" as possible, but this indeed looks like a relatively advanced feature that now is actually supported only on nightly.

Support exposing enum variants in Self for enum impl blocks

That's what what the nightly feature enables, and should be in track to be stabilized within a few versions.

Improve the compiler error message

I'm always for improving the errors, and the originally reported one was bad for all the reasons you stated in the original report, but that error is no longer emitted and #49683 will make the whole point mood once it's stabilized :)

Grouping errors due to the same feature lint in the same scope is unlikely to happen soon, and not strictly necessary, just a nice to have improvement.

On the other hand adding a suggestion or help to use the enum's name instead of Self should be easy enough to add in the meantime (depending on how quickly #49683 gets implemented this would be worth it).

@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Apr 30, 2019
bors added a commit that referenced this issue Jul 1, 2019
…petrochenkov

Stabilize `type_alias_enum_variants` in Rust 1.37.0

Stabilize `#![feature(type_alias_enum_variants)]` which allows type-relative resolution with highest priority to `enum` variants in both expression and pattern contexts. For example, you may now write:

```rust
enum Option<T> {
    None,
    Some(T),
}

type OptAlias<T> = Option<T>;

fn work_on_alias(x: Option<u8>) -> u8 {
    match x {
        OptAlias::Some(y) => y + 1,
        OptAlias::None => 0,
    }
}
```

Closes rust-lang/rfcs#2218
Closes #52118

r? @petrochenkov
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 C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants