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

Tweak suggestions when using incorrect type of enum literal #127891

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

estebank
Copy link
Contributor

More accurate suggestions when writing wrong style of enum variant literal:

error[E0533]: expected value, found struct variant `E::Empty3`
  --> $DIR/empty-struct-braces-expr.rs:18:14
   |
LL |     let e3 = E::Empty3;
   |              ^^^^^^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     let e3 = E::Empty3 {};
   |                        ++
error[E0533]: expected value, found struct variant `E::V`
  --> $DIR/struct-literal-variant-in-if.rs:10:13
   |
LL |     if x == E::V { field } {}
   |             ^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     if x == (E::V { field }) {}
   |             +              +
error[E0618]: expected function, found enum variant `Enum::Unit`
  --> $DIR/suggestion-highlights.rs:15:5
   |
LL |     Unit,
   |     ---- enum variant `Enum::Unit` defined here
...
LL |     Enum::Unit();
   |     ^^^^^^^^^^--
   |     |
   |     call expression requires function
   |
help: `Enum::Unit` is a unit enum variant, and does not take parentheses to be constructed
   |
LL -     Enum::Unit();
LL +     Enum::Unit;
   |
error[E0599]: no variant or associated item named `tuple` found for enum `Enum` in the current scope
  --> $DIR/suggestion-highlights.rs:36:11
   |
LL | enum Enum {
   | --------- variant or associated item `tuple` not found for this enum
...
LL |     Enum::tuple;
   |           ^^^^^ variant or associated item not found in `Enum`
   |
help: there is a variant with a similar name
   |
LL |     Enum::Tuple(/* i32 */);
   |           ~~~~~~~~~~~~~~~~;
   |

Tweak "field not found" suggestion when giving struct literal for tuple struct type:

error[E0560]: struct `S` has no field named `x`
  --> $DIR/nested-non-tuple-tuple-struct.rs:8:19
   |
LL | pub struct S(f32, f32);
   |            - `S` defined here
...
LL |     let _x = (S { x: 1.0, y: 2.0 }, S { x: 3.0, y: 4.0 });
   |                   ^ field does not exist
   |
help: `S` is a tuple struct, use the appropriate syntax
   |
LL |     let _x = (S(/* f32 */, /* f32 */), S { x: 3.0, y: 4.0 });
   |               ~~~~~~~~~~~~~~~~~~~~~~~

@rustbot
Copy link
Collaborator

rustbot commented Jul 18, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 18, 2024

HIR ty lowering was modified

cc @fmease

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 18, 2024
@rust-log-analyzer

This comment has been minimized.

…teral

```
error[E0533]: expected value, found struct variant `E::Empty3`
  --> $DIR/empty-struct-braces-expr.rs:18:14
   |
LL |     let e3 = E::Empty3;
   |              ^^^^^^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     let e3 = E::Empty3 {};
   |                        ++
```
```
error[E0533]: expected value, found struct variant `E::V`
  --> $DIR/struct-literal-variant-in-if.rs:10:13
   |
LL |     if x == E::V { field } {}
   |             ^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     if x == (E::V { field }) {}
   |             +              +
```
```
error[E0618]: expected function, found enum variant `Enum::Unit`
  --> $DIR/suggestion-highlights.rs:15:5
   |
LL |     Unit,
   |     ---- enum variant `Enum::Unit` defined here
...
LL |     Enum::Unit();
   |     ^^^^^^^^^^--
   |     |
   |     call expression requires function
   |
help: `Enum::Unit` is a unit enum variant, and does not take parentheses to be constructed
   |
LL -     Enum::Unit();
LL +     Enum::Unit;
   |
```
```
error[E0599]: no variant or associated item named `tuple` found for enum `Enum` in the current scope
  --> $DIR/suggestion-highlights.rs:36:11
   |
LL | enum Enum {
   | --------- variant or associated item `tuple` not found for this enum
...
LL |     Enum::tuple;
   |           ^^^^^ variant or associated item not found in `Enum`
   |
help: there is a variant with a similar name
   |
LL |     Enum::Tuple(/* i32 */);
   |           ~~~~~~~~~~~~~~~~;
   |
```
…le struct type

```
error[E0560]: struct `S` has no field named `x`
  --> $DIR/nested-non-tuple-tuple-struct.rs:8:19
   |
LL | pub struct S(f32, f32);
   |            - `S` defined here
...
LL |     let _x = (S { x: 1.0, y: 2.0 }, S { x: 3.0, y: 4.0 });
   |                   ^ field does not exist
   |
help: `S` is a tuple struct, use the appropriate syntax
   |
LL |     let _x = (S(/* f32 */, /* f32 */), S { x: 3.0, y: 4.0 });
   |               ~~~~~~~~~~~~~~~~~~~~~~~
```
help: there is a variant with a similar name
|
LL | S::A {} => {},
| ~
Copy link
Contributor

Choose a reason for hiding this comment

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

Tangent unrelated to this PR: I do wonder if the similarity detector should not kick in here, for a single-letter identifier. A and B are not that similar, they're 100% different!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that we do limit the levenshtein distance logic to N>3, but we still want to mention "hey, you might have meant to use another available variant". For that maybe we should just list all variants, maybe?

|
help: you might have meant to create a new value of the struct
|
LL | let f = Foo::Variant { x: /* value */ };
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggesting Foo::variant { x: 42 } would be nice here, if it's easy.

Copy link
Contributor Author

@estebank estebank Jul 19, 2024

Choose a reason for hiding this comment

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

I don't think it's easy per-se. It's doable, but I'd prefer doing it as part of its own PR to avoid landing a larger change than strictly necessary.

We would also want to take into consideration bindings in scope that have the appropriate type, and maybe the same or similar name.

@@ -9,8 +9,8 @@ LL | let z = NonCopyable{ p: () };
|
help: `NonCopyable` is a tuple struct, use the appropriate syntax
|
LL | let z = NonCopyable(/* fields */);
| ~~~~~~~~~~~~~~~~~~~~~~~~~
LL | let z = NonCopyable(/* () */);
Copy link
Contributor

Choose a reason for hiding this comment

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

This one's a little confusing because () is the name of the type and its one value. But it's probably not worth specializing it to removes the comment markers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a "Give us a default value for this type" logic. I do think that this and others are edge cases that we can ignore for now.

@nnethercote
Copy link
Contributor

TIL there are SVG tests. I assume that's to test the colour output? Reviewing changes to them is difficult :/

r=me, feel free to address the minor suggestions above, or not.

@estebank
Copy link
Contributor Author

TIL there are SVG tests. I assume that's to test the colour output?

Indeed. We added them recently.

Reviewing changes to them is difficult :/

This test in particular is really long, as it incorporates every case of incorrect variant literal. That makes it less than ideal for SVG output. I find that they work best when there are a handful of errors, and then the side-by-side, swipe and "onion skin" methods of comparison that GitHub provides work well enough (as long as the before and after aren't too different in height).


Based on my responses above, I'll go ahead and merge this, but take those comments as future action items for suggestions, both for literals and for ADT literals.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2024

📌 Commit 33bd4bd has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2024
Comment on lines 10 to +13
help: `S` is a tuple struct, use the appropriate syntax
|
LL | let _x = (S(/* fields */), S { x: 3.0, y: 4.0 });
| ~~~~~~~~~~~~~~~
LL | let _x = (S(/* f32 */, /* f32 */), S { x: 3.0, y: 4.0 });
| ~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nnethercote this is a case where the "reuse the literal values that were already there" gets harder: should it have been S(3.0, 4.0) or S(4.0, 3.0)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing these help suggestions aren't required to be 100% correct, right? Though of course we want them to be as close to 100% as reasonable.

So I would answer S(3.0, 4.0), because that matches the order the float literals appear in the code. Sure, it's possible that somebody wrote the literal with the fields in a different order than in the type declaration, but I would expect that is less likely than matching the type declaration order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if we ever handle this case, I'd only hazard a guess on moving existing code when there is no ambiguity, and leave the comment placeholder for cases like this one so that the user has to make a determination manually (with IDEs a user could accidentally accept a suggestion without noticing the subtlety of what happened).

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#121533 (Handle .init_array link_section specially on wasm)
 - rust-lang#127825 (Migrate `macos-fat-archive`, `manual-link` and `archive-duplicate-names` `run-make` tests to rmake)
 - rust-lang#127891 (Tweak suggestions when using incorrect type of enum literal)
 - rust-lang#127902 (`collect_tokens_trailing_token` cleanups)
 - rust-lang#127928 (Migrate `lto-smoke-c` and `link-path-order` `run-make` tests to rmake)
 - rust-lang#127935 (Change `binary_asm_labels` to only fire on x86 and x86_64)
 - rust-lang#127953 ([compiletest] Search *.a when getting dynamic libraries on AIX)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc6e34f into rust-lang:master Jul 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup merge of rust-lang#127891 - estebank:enum-type-sugg, r=estebank

Tweak suggestions when using incorrect type of enum literal

More accurate suggestions when writing wrong style of enum variant literal:

```
error[E0533]: expected value, found struct variant `E::Empty3`
  --> $DIR/empty-struct-braces-expr.rs:18:14
   |
LL |     let e3 = E::Empty3;
   |              ^^^^^^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     let e3 = E::Empty3 {};
   |                        ++
```
```
error[E0533]: expected value, found struct variant `E::V`
  --> $DIR/struct-literal-variant-in-if.rs:10:13
   |
LL |     if x == E::V { field } {}
   |             ^^^^ not a value
   |
help: you might have meant to create a new value of the struct
   |
LL |     if x == (E::V { field }) {}
   |             +              +
```
```
error[E0618]: expected function, found enum variant `Enum::Unit`
  --> $DIR/suggestion-highlights.rs:15:5
   |
LL |     Unit,
   |     ---- enum variant `Enum::Unit` defined here
...
LL |     Enum::Unit();
   |     ^^^^^^^^^^--
   |     |
   |     call expression requires function
   |
help: `Enum::Unit` is a unit enum variant, and does not take parentheses to be constructed
   |
LL -     Enum::Unit();
LL +     Enum::Unit;
   |
```
```
error[E0599]: no variant or associated item named `tuple` found for enum `Enum` in the current scope
  --> $DIR/suggestion-highlights.rs:36:11
   |
LL | enum Enum {
   | --------- variant or associated item `tuple` not found for this enum
...
LL |     Enum::tuple;
   |           ^^^^^ variant or associated item not found in `Enum`
   |
help: there is a variant with a similar name
   |
LL |     Enum::Tuple(/* i32 */);
   |           ~~~~~~~~~~~~~~~~;
   |
```

Tweak "field not found" suggestion when giving struct literal for tuple struct type:

```
error[E0560]: struct `S` has no field named `x`
  --> $DIR/nested-non-tuple-tuple-struct.rs:8:19
   |
LL | pub struct S(f32, f32);
   |            - `S` defined here
...
LL |     let _x = (S { x: 1.0, y: 2.0 }, S { x: 3.0, y: 4.0 });
   |                   ^ field does not exist
   |
help: `S` is a tuple struct, use the appropriate syntax
   |
LL |     let _x = (S(/* f32 */, /* f32 */), S { x: 3.0, y: 4.0 });
   |               ~~~~~~~~~~~~~~~~~~~~~~~
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants