Unnecessary parentheses warning is printed twice

[jDomantas]

This code:

pub fn repro() {
    if let Some(_a) = foo() {
        let _b = (foo());
    }
}

fn foo() -> Option<i32> { None }

Gives unnecessary parentheses warning twice:

warning: unnecessary parentheses around assigned value
 --> interpreter\src\lib.rs:3:18
  |
3 |         let _b = (foo());
  |                  ^^^^^^^ help: remove these parentheses
  |
  = note: `#[warn(unused_parens)]` on by default

warning: unnecessary parentheses around assigned value
 --> interpreter\src\lib.rs:3:18
  |
3 |         let _b = (foo());
  |                  ^^^^^^^ help: remove these parentheses

warning: 2 warnings emitted

Reproduced with stable 1.54.0 and nightly 2021-08-22 af14075 on playground.

Playground link

[ehuss]

Bisected the regression to #82854.

@estebank Do you happen to know why the deduplicator here is not deduplicating these? I ran into another strange duplicate diagnostic in another situation, too.

[chenyukang]

@ehuss I have some investigation on this.
This is caused by two different hash values calculated in here.

The two diagnostics are the same, except that the first diagnostic has children, and the latter one doesn't have children.
I'm trying to make a fix on it.

[chenyukang]

My draft commit fixed this issue, how do you think about it, @ehuss .
Need to add a test case for it.

[ehuss]

I'm not an expert on the compiler, so I'm not the right person to ask. However, I would say changing the hash might not be a good idea, since I think it would be bad to lose the children, or have it be non-deterministic which diagnostic gets emitted. It's also dangerous to have the Hash implementation out of sync with PartialEq since some code may be assuming those are in sync.

For this particular issue, I would investigate more why there are multiple lints getting triggered with different sets of children, and see if it is possible to avoid emitting multiple lints. Or if multiple are emitted to make sure they are the same.

[chenyukang]

I have more debugging on this.

There are two execute paths finally flow to analyze the expr,

fn visit_expr(&mut self, e: &'a ast::Expr) {
        self.with_lint_attrs(e.id, &e.attrs, |cx| {
            run_early_pass!(cx, check_expr, e);       // C1: create fir the diagnostic 
            ast_visit::walk_expr(cx, e);                     // C2: create the second diagnostic
        })
    }

The C1 will finally call to check_expr
The C2 will finally call to check_stmt

Seems it's common in Rustc lint, since I see some code is trying to remove duplicates.

Then, why there is only the first diagnostic that contains children? The child is a note for diagnostic:

warning: unnecessary parentheses around assigned value
  --> ./src/main.rs:35:18
   |
35 |         let _b = (foo_b());
   |                  ^^^^^^^^^ help: remove these parentheses
   |
   = note: `#[warn(unused_parens)]` on by default                // This is the children part.

This is because when lint try to add a diagnostic, diag_once is called to add note for it, but this line of code get the fresh as false for the second diagnostic, so it won't add a note:

 let id_span_message = (msg_id, span_maybe, message.to_owned());
 let fresh = self.one_time_diagnostics.borrow_mut().insert(id_span_message);

[chenyukang]

These two test cases have similar duplicated warnings reported out:
https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/outer-forbid.stderr
https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/issue-70819-dont-override-forbid-in-same-scope.stderr

[chenyukang]

How do you think @estebank , do you think custom Hash is the right way for this? I'm a beginner in hacking Rust compiler, maybe you have any more idea.