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

remove deduplicate-diagnostics=no in suspicious_operation_groupings.stderr #13266

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kyoto7250
Copy link
Contributor

@kyoto7250 kyoto7250 commented Aug 13, 2024

a part of #12379.

In this PR, we will update the lint to store already outputted Spans internally to prevent suspicious_operation_groupings from outputting the same error multiple times.

changelog:
Fixed duplicate errors in suspicious_operation_groupings.

@rustbot
Copy link
Collaborator

rustbot commented Aug 13, 2024

r? @Centri3

rustbot has assigned @Centri3.
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 Aug 13, 2024
| ^^^^^^^^^^^ help: did you mean: `s1.b * s2.b`

error: this sequence of operators looks suspiciously like a bug
--> tests/ui/suspicious_operation_groupings.rs:88:19
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error is identical to the one at line 23, so it can be safely removed.

| ^^^^^^^^^^^ help: did you mean: `s1.c * s2.c`

error: this sequence of operators looks suspiciously like a bug
--> tests/ui/suspicious_operation_groupings.rs:136:42
Copy link
Contributor Author

@kyoto7250 kyoto7250 Aug 13, 2024

Choose a reason for hiding this comment

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

This error is identical to the one at line 77 91, so it can be safely removed.

edit: I made a mistake with the line numbers in the comment

Copy link
Member

Choose a reason for hiding this comment

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

this would indicate there's a place that should be using span_lint_hir_and_then but is not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this lint, span_lint_and_sugg is used, but what is the difference between it and span_lint_hir_and_then?
It seems to me that specifying hir may not be necessary in this case.

Copy link
Member

Choose a reason for hiding this comment

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

It tells the compiler at what node the lint is being emitted for the purposes of #[allow]/etc, if not specified it will be the node that check_xxx was called on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me confirm my understanding. I think this duplicate error output is coming from both sides of binOp.
I still don't understand the relationship between solving this problem and the need to use span_lint_hir_and_then.

@kyoto7250 kyoto7250 marked this pull request as ready for review August 20, 2024 15:38
Copy link
Member

@Centri3 Centri3 left a comment

Choose a reason for hiding this comment

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

Maybe this wouldn't work, but usually this is done by early returning if the other check successfully lints?

On span_lint_hir_and_then, this will need to use it one day as it doesn't lint on the exact node in check_expr, however that'd require rewriting the whole lint. I'm curious how that'd fix it? It does hash the diagnostic it's about to emit in emit_diagnostic so maybe the node differing causes it to emit regardless of deduplicate-diagnostics, but I'd like confirmation on that

@Centri3
Copy link
Member

Centri3 commented Sep 13, 2024

r? rust-lang/clippy

@rustbot rustbot assigned Manishearth and unassigned Centri3 Sep 13, 2024
@Manishearth
Copy link
Member

Yeah, I don't think this is the right fix. We should use the right spans whilst linting.

Why is this lint emitting duplicate diagnostics?

@bors
Copy link
Contributor

bors commented Sep 22, 2024

☔ The latest upstream changes (presumably #13440) made this pull request unmergeable. Please resolve the merge conflicts.

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.

6 participants