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

Fix single_match lint being emitted when it should not #13765

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

GuillaumeGomez
Copy link
Member

We realized when running clippy --fix on rustdoc (PR comment here) that some comments were removed, which is problematic. This PR checks that comments outside of match arms are taken into account before emitting the lint.

changelog: Fix single_match lint being emitted when it should not

@rustbot
Copy link
Collaborator

rustbot commented Nov 30, 2024

r? @llogiq

rustbot has assigned @llogiq.
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 Nov 30, 2024
@@ -1059,7 +1061,28 @@ impl<'tcx> LateLintPass<'tcx> for Matches {
}

redundant_pattern_match::check_match(cx, expr, ex, arms);
single_match::check(cx, ex, arms, expr);
let source_map = cx.tcx.sess.source_map();
let mut match_comments = span_extract_comments(source_map, expr.span);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to extract the comment spans instead of the text to compare?

What if we have the same comment text twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a Vec, not a map. So if a comment is present more than once, not an issue since it'll still remain one at the end. Gonna add a test for this case though.

As for using spans, it's possible but would likely make the code much less readable as we manipulate InnerSpans here since we parse a snippet and not use AST comments.

@llogiq
Copy link
Contributor

llogiq commented Dec 7, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Dec 7, 2024
@GuillaumeGomez
Copy link
Member Author

@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 from the author. (Use `@rustbot ready` to update this status) labels Dec 13, 2024
@llogiq
Copy link
Contributor

llogiq commented Dec 13, 2024

Thank you!

@llogiq llogiq added this pull request to the merge queue Dec 13, 2024
Merged via the queue into rust-lang:master with commit c607408 Dec 13, 2024
9 checks passed
@GuillaumeGomez GuillaumeGomez deleted the fix-single_match branch December 13, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants