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

explain what an item is when invalid code is placed at the module-level #113110

Closed
dawnofmidnight opened this issue Jun 27, 2023 · 9 comments · Fixed by #115473
Closed

explain what an item is when invalid code is placed at the module-level #113110

dawnofmidnight opened this issue Jun 27, 2023 · 9 comments · Fixed by #115473
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dawnofmidnight
Copy link
Contributor

dawnofmidnight commented Jun 27, 2023

Right now, it can be somewhat confusing what an item is when one gets an error like the one below. Most people who see this error likely don't know enough about Rust to know what an item is, making it fairly unhelpful. The let example does have a better error (although notably not for let mut), but that's only one particular case.

Code

5

Current output

error: expected item, found `5`
 --> src/lib.rs:1:1
  |
1 | 5
  | ^ expected item

Desired output

error: expected item, found `5`
 --> src/main.rs:1:1
  |
1 | 5
  | ^ expected item
  | 
  | help: items are things that can appear in a module. for example:
  |     - global variables: `const`, `static`
  |     - types and traits: `struct`, `enum`, `trait`,
  |     - functions: `fn`, `async fn`
  |     - implementations: `impl`
  |     - module definitions and uses: `mod`, `use`
  | 
  | for a full list of possible items see https://doc.rust-lang.org/reference/items.html.

The above "desired output" is not an exhaustive list. In particular, it doesn't include macros which expand to items, attributes, doc comments, unsafe, extern, and a number of other potentially relevant things. I'm not sure just how exhaustive this list should be, as it's a long one and preferably shouldn't overwhelm the reader.

Another consideration is whether a list is useful at all. There are a lot of things, and especially if one is making a mistake like this, it is entirely possible that they would not know what many of the things on this list are. A link to the reference is included here, but again, the reference isn't exactly an introductory resource that one reads to learn the language.

Other cases

error: expected item, found keyword `let`
 --> src/lib.rs:1:1
  |
1 | let x = 5;
  | ^^^ consider using `const` or `static` instead of `let` for global variables

Anything else?

Other similar issues:

@dawnofmidnight dawnofmidnight 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 Jun 27, 2023
@konnorandrews
Copy link

error: expected item, found `5`
 --> src/main.rs:1:1
  |
1 | 5
  | ^ expected item
  | 
  | help: For example, the following can appear here, at the root of a module:
  |     - global variables: `const`, `static`
  |     - types and traits: `struct`, `enum`, `trait`
  |     - functions: `fn`, `async fn`
  |     - implementations: `impl`
  |     - module definitions and uses: `mod`, `use`
  | 
  | For a full list of possible items see https://doc.rust-lang.org/reference/items.html.

Here is one trying to rely less on terms but still providing the same information about the context.

@sam0x17
Copy link

sam0x17 commented Jun 28, 2023

This is great. Originally it was suggested that we just list example items, but this doesn't help users understand what the list means. My original comment was "lists don't help much if you don't know how they are generated". So it is of course better to also give a concise but formally correct definition of what an item is: an item is anything that can appear in the top level of a module.

Another benefit I see is that a lot of beginner rust users don't have a good concept of how modules work yet, and many don't even fully realize that each .rs file is a module, and this error message could be the first hint they get that "oh, I'm in a module right now!"

side note: as a related effort, we should probably also review what kind of error messages we emit for this whole class of situations, like if you type something invalid in trait item position, in impl body position, etc

@Noratrieb
Copy link
Member

your desired output is too verbose imo. a diagnostic like this needs to be concise enough to make sure that it doesn't take away space from other diagnostics for people who already know what am item is. maybe it could get a separate error code with a help explicitly linking to the error code page which could explain it in detail?

@sam0x17
Copy link

sam0x17 commented Jun 28, 2023

I think it could simply be "expected item, found 5. An item is anything that can appear in the top level of a module, including [list here]."

@jyn514 jyn514 added A-parser Area: The parsing of Rust source code to an AST D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Jun 30, 2023
@workingjubilee
Copy link
Member

These suggestions overcalibrate on "improving" the current diagnostic, but the current diagnostic is bad, as it merely states "what is wrong?", even with an improvement. I believe this would be improved much more by focusing on the goal instead.

Either the user needs to either correct a stray typo, OR they need to separate from a mental model carried over from languages which allow expressions at the top level. These lead to two very different suggestions. In one case, saying anything but "delete this?" is noise. In the other case, we don't necessarily care to explain what an "item" is, because we actually want to get the user to put their code inside a const, static, or fn.

@gurry
Copy link
Contributor

gurry commented Aug 3, 2023

How about providing just the link for brevity:

error: expected item, found `5`
 --> src/main.rs:1:1
  |
1 | 5
  | ^ expected item
  | 
  | help: items are things that can appear at the root of a module. 
  | For a full list of items see https://doc.rust-lang.org/reference/items.html.

@workingjubilee
Copy link
Member

A link seems much more reasonable, yes.

@gurry
Copy link
Contributor

gurry commented Sep 2, 2023

@rustbot claim

@gurry
Copy link
Contributor

gurry commented Sep 3, 2023

How about providing just the link for brevity:

error: expected item, found `5`
 --> src/main.rs:1:1
  |
1 | 5
  | ^ expected item
  | 
  | help: items are things that can appear at the root of a module. 
  | For a full list of items see https://doc.rust-lang.org/reference/items.html.

I've opened PR #115473 implementing this. Please take a look everyone and see if it makes sense.

fmease added a commit to fmease/rust that referenced this issue Sep 6, 2023
…iler-errors

Add explanatory note to 'expected item' error

Fixes rust-lang#113110

It changes the diagnostic from this:

```
error: expected item, found `5`
 --> ../test.rs:1:1
  |
1 | 5
  | ^ expected item
 ```
to this:
```
error: expected item, found `5`
 --> ../test.rs:1:1
  |
1 | 5
  | ^ expected item
  |
  = note: items are things that can appear at the root of a module
  = note: for a full list see https://doc.rust-lang.org/reference/items.html
```
@bors bors closed this as completed in 4a31cc8 Sep 6, 2023
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-parser Area: The parsing of Rust source code to an AST D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. 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.

7 participants