Skip to content

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

Merged
merged 1 commit into from
Jun 16, 2025

Conversation

xizheyin
Copy link
Contributor

@xizheyin xizheyin commented Jun 11, 2025

Fixes #142311

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 11, 2025
Copy link
Member

@compiler-errors compiler-errors left a 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.

@xizheyin
Copy link
Contributor Author

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.

@compiler-errors
Copy link
Member

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.

@xizheyin
Copy link
Contributor Author

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.

Copy link
Member

@fee1-dead fee1-dead left a 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
    ]
}

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 12, 2025
Copy link
Contributor Author

@xizheyin xizheyin left a comment

Choose a reason for hiding this comment

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

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2025
@xizheyin xizheyin changed the title Add ParseContext for diagnostic in Parser and remove false comments suggestion Suggest add missing , before doc comments in parsing item list Jun 12, 2025
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2025
@xizheyin
Copy link
Contributor Author

I modified the fixme and used the E0585 code in two new commits. :3

@xizheyin xizheyin force-pushed the 142311 branch 2 times, most recently from 03f4ee4 to 9a3b2ce Compare June 14, 2025 02:06
@xizheyin
Copy link
Contributor Author

The current version is much better.
@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 14, 2025
@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2025
@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jun 15, 2025
@xizheyin xizheyin force-pushed the 142311 branch 2 times, most recently from 1c619e4 to e9bbf23 Compare June 15, 2025 15:44
Copy link
Contributor Author

@xizheyin xizheyin left a 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

Comment on lines +30 to +40
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
Copy link
Contributor Author

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 //.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 15, 2025
@fee1-dead fee1-dead changed the title Suggest add missing , before doc comments in parsing item list Don't suggest converting /// to // when expecting , Jun 16, 2025
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2025
Copy link
Contributor Author

@xizheyin xizheyin left a comment

Choose a reason for hiding this comment

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

@rustbot ready

Comment on lines 689 to 698
// 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comments are added.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2025
Comment on lines 689 to 695
// 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,
// }`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// 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,
// }
// ```

Copy link
Member

@fee1-dead fee1-dead left a 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

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2025
@fee1-dead
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 16, 2025

📌 Commit c63665c has been approved by fee1-dead

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 16, 2025
bors added a commit that referenced this pull request Jun 16, 2025
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
@bors bors merged commit d68432a into rust-lang:master Jun 16, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 16, 2025
rust-timer added a commit that referenced this pull request Jun 16, 2025
Rollup merge of #142341 - xizheyin:142311, r=fee1-dead

Don't suggest converting `///` to `//` when expecting `,`

Fixes #142311
@xizheyin xizheyin deleted the 142311 branch June 17, 2025 04:25
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.

Missing comma in enum interacts badly with doc comment advice
6 participants