-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Don't suggest converting ///
to //
when expecting ,
#142341
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach feels larger than the issue it's trying to fix, and it also isn't applied to other generalizations of this problem like struct fields. I'm not sure if it's warranted--I think we could simply suppress the doc comment ///
-> // /
suggestion when a trailing comma is in the expected tokens list, which would be a good enough approximation of this without needing to add a whole new "parse context" tracking to the parser.
That's what I thought at first, but I'm worried that there are many similar cases in the code. If only record the case, fixing every such error will make the code ugly like patching. But I haven't got a detailed investigation of the specific situations where context is needed. Maybe the current one can be used without context. But in the future, this context refactoring may be useful to enhance the maintainability of the code to solve multiple similar problem. |
Well right now it doesn't seem particularly worthwhile. I'm not totally sure if the original issue report is really worth fixing, since the suggestion is valid in some cases, and suppressing it seems like more work than is worth it. |
For that edge situation, it is really worth fixing, at least it should be set to maybeincorrect, not machineappreciate.At present, the testcase related to that doc comment suggestion does not cover this situation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't needed because the edible
parameter on expected_one_of_not_found
already contains information on what tokens we expect.
It would be much better to suggest adding a comma before the doc comment if a comma was expected, so this case works out too:
fn a() {
[
a /// Test
b
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rustbot ready
ParseContext
for diagnostic in Parser
and remove false comments suggestion,
before doc comments in parsing item list
I modified the fixme and used the E0585 code in two new commits. :3 |
03f4ee4
to
9a3b2ce
Compare
The current version is much better. |
tests/ui/parser/doc-comment-after-missing-comma-issue-142311.rs
Outdated
Show resolved
Hide resolved
1c619e4
to
e9bbf23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rustbot ready
Surprisingly, the plainest solution was used in the end. lol
error: expected one of `,`, `.`, `;`, `?`, `]`, or an operator, found doc comment `///xxxxxx` | ||
--> $DIR/doc-comment-after-missing-comma-issue-142311.rs:20:10 | ||
| | ||
LL | 1///xxxxxx | ||
| ^^^^^^^^^ expected one of `,`, `.`, `;`, `?`, `]`, or an operator | ||
|
||
error: expected one of `,`, `.`, `?`, `]`, or an operator, found doc comment `///xxxxxx` | ||
--> $DIR/doc-comment-after-missing-comma-issue-142311.rs:29:10 | ||
| | ||
LL | 2///xxxxxx | ||
| ^^^^^^^^^ expected one of `,`, `.`, `?`, `]`, or an operator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For array, it does not suggest missing because it can be the following cases,
let a = [
Some(1)?,
2
];
or
let a = [
1.0,
2
];
It can be many situations ,
, .
, ;
, ?
.
But we may have reached our goal: supress the suggestion converting ///
to //
.
,
before doc comments in parsing item list///
to //
when expecting ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rustbot ready
// This is to avoid suggesting converting a doc comment to a regular comment | ||
// when missing a comma before the doc comment in lists. (issue #142311) | ||
// For example: | ||
// `enum Foo{ | ||
// A ///xxxxxxx | ||
// B, | ||
// }` | ||
if !expected.contains(&TokenType::Comma) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are added.
// This is to avoid suggesting converting a doc comment to a regular comment | ||
// when missing a comma before the doc comment in lists. (issue #142311) | ||
// For example: | ||
// `enum Foo{ | ||
// A ///xxxxxxx | ||
// B, | ||
// }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// This is to avoid suggesting converting a doc comment to a regular comment | |
// when missing a comma before the doc comment in lists. (issue #142311) | |
// For example: | |
// `enum Foo{ | |
// A ///xxxxxxx | |
// B, | |
// }` | |
// This is to avoid suggesting converting a doc comment to a regular comment | |
// when missing a comma before the doc comment in lists (#142311): | |
// | |
// ``` | |
// enum Foo{ | |
// A /// xxxxxxx | |
// B, | |
// } | |
// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please squash the three commits into one
…r missing `,` in list Signed-off-by: xizheyin <[email protected]>
@bors r+ rollup |
Rollup of 8 pull requests Successful merges: - #139340 (Fix RISC-V C function ABI when passing/returning structs containing floats) - #142341 (Don't suggest converting `///` to `//` when expecting `,`) - #142414 (ignore `run-make` tests that need `std` on targets without `std`) - #142498 (Port `#[rustc_as_ptr]` to the new attribute system) - #142554 (Fix `PathSource` lifetimes.) - #142562 (Update the `backtrace` submodule) - #142565 (Test naked asm for wasm32-unknown-unknown) - #142573 (`fn candidate_is_applicable` to method) r? `@ghost` `@rustbot` modify labels: rollup
Fixes #142311