Better error message for inner attribute following doc comment #34676
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Thanks @aravind-pg! This looks good to me. My only reservation is the text "an inner attr is not permitted following a doc comment". Technically what's invalid is following an outer doc comment - there are both inner and outer doc comments. Although raising the distinction is potentially confusion, I think it's probably better to be explicit about it. Can you update the text to say "outer doc comment"? f? @mikedilger @Manishearth @jonathandturner trying to make a confusing parse error understandable. |
|
Makes sense, updated now. |
|
inner/outer attrs aren't a well-known term, could we also display notes explaining what each is? |
|
Sure. Any suggestions on what the notes should be so as to be friendly to users? Given that in this error message we're distinguishing general outer attributes from outer doc comments anyway, maybe something very explicit like the following?
Alternatively maybe we should just link to https://doc.rust-lang.org/book/attributes.html. I'd like that better actually. |
|
Sounds good. I prefer things like this to be inline. |
|
Added notes now, may need further wordsmithing, PTAL. |
| (`mod` bar in our example)") | ||
| .note("an outer attribute is either of the form `#[foo]` in \ | ||
| `#[foo] struct Baz` or is an outer doc comment, modifying \ | ||
| the item following it (`struct Baz` in our example)") |
There was a problem hiding this comment.
This is a lot of spew. Let's try to condense a bit. First, let's remove the 'help'. I think with the new explanation it is pretty useless.
For the 'note' it would be good to indicate that inner attributes can be comments and are usually encountered at the crate level.
What about this: "inner attributes and doc comments, like #![no_std] or //! My crate, annotate the enclosing item, and are usually found at the crate root. Outer attributes and doc comments, like #[test] and /// My function, annotate the following item."
There was a problem hiding this comment.
Yeah, I do agree that this is a lot of spew, unfortunately. But I also think the 'help' is pretty useful here since it is actually actionable, and especially so for a beginner.
Your suggestion for the note sounds pretty good to me, and I'm happy to change mine, but I have a vague worry that the sentence structure doesn't quite emphasize the "enclosing vs. following" distinction enough (when used as adjectives like that it somehow seems they don't stand out enough, if that makes any sense). Maybe it would be good to get the opinion of @steveklabnik on this? Also @Manishearth since he was the one who suggested the note.
Also, would you like it to be two separate notes or one long sentence/note?
There was a problem hiding this comment.
I'd prefer one note if possible.
There was a problem hiding this comment.
Fix it to be a single note and r=me
There was a problem hiding this comment.
Sounds good. And should the 'help' stay or go?
|
Updated with @brson's suggested note now, with a minor change (s/following item/item following them, because otherwise it reads like it might be referring to something following the note itself. Similarly s/enclosing item/item enclosing them). Should be good to go now? |
| .help("place inner attribute at the top of the module or \ | ||
| block") | ||
| .note("inner attributes and doc comments, like `#![no_std]` or \ | ||
| `//! My crate`, annotate the item enclosing them, and are \ | ||
| usually found at the crate root. Outer attributes and doc \ |
There was a problem hiding this comment.
Inner attributes are also often found at the start of files for any module at all, not just the crate root, no?
Maybe it would be simpler to just say "usually found at the start of Rust source files", which would cover both crate roots and non-root modules housed in their own files?
There was a problem hiding this comment.
Agreed. @aravind-pg can you make one more update?
I also strongly feel that the help is redundant now. Help says 'place inner attribute at X'; note says 'inner attributes annotate the item enclosing them'. The two statements are semantically similar. If there is important information missing from the note then the wording can be tweaked slightly. I'd like to remove the help.
There was a problem hiding this comment.
Hm, on second thought this is a rare error, so one extra note isn't going to matter much. I do think it would be cleaner to say what we're trying to say in once sentence than two.
There was a problem hiding this comment.
Not completely clear what your final call is here. I can definitely get rid of the help, not strongly attached to it by any means. Will also make the change that @pnkfelix pointed out.
|
Updated to remove the help and make the change that @pnkfelix suggested. |
|
@bors r+ Thanks @aravind-pg ! |
|
📌 Commit 8f6dba9 has been approved by |
|
Fixed trailing whitespace tidy error. @bors r=brson |
|
@bors-servo r=brson only reviewers can do that 😄 |
|
📌 Commit ff95ba3 has been approved by |
|
Er. @bors r=brson |
|
💡 This pull request was already approved, no need to approve it again.
|
|
📌 Commit ff95ba3 has been approved by |
|
Haha yeah I figured, was just about to pop by #rust-internals to ask someone. Also, kinda cool that @bors-servo can stand in like that for @bors, huh. |
|
Homu/bors has a very simple parser that just throws out things it doesn't understand. As long as the line starts with bors, in Rust's case, it is fine. On servo it has to start with bors-servo. I'm involved in both projects so I often ping the wrong bot. This works on Rust, but not servo. |
Better error message for inner attribute following doc comment Before it was always just "an inner attribute is not permitted in this context", whereas now we add a special case for when an inner attr follows an outer attr. If the outer attr is a doc comment, then the error is "an inner attr is not permitted following a doc comment", and otherwise it's "an inner attr is not permitted following an outer attribute". In all other cases it's still "an inner attribute is not permitted in this context". Note that the public API and behaviour of `parse_attribute` is unchanged. Also, all new names are very open to bikeshedding -- they're arguably clunky. Fixes #34516. cc @brson
Before it was always just "an inner attribute is not permitted in this context", whereas now we add a special case for when an inner attr follows an outer attr. If the outer attr is a doc comment, then the error is "an inner attr is not permitted following a doc comment", and otherwise it's "an inner attr is not permitted following an outer attribute". In all other cases it's still "an inner attribute is not permitted in this context".
Note that the public API and behaviour of
parse_attributeis unchanged. Also, all new names are very open to bikeshedding -- they're arguably clunky.Fixes #34516. cc @brson