`#[expect(unused_imports)]` does not work correctly on grouped imports

[vxpm]

the following snippet:

#[expect(unused_imports)]
use std::{io, fs};

results in the following compiler output:

warning: this lint expectation is unfulfilled
 --> src/main.rs:2:10
  |
2 | #[expect(unused_imports)]
  |          ^^^^^^^^^^^^^^
  |
  = note: ​`#[warn(unfulfilled_lint_expectations)]​` on by default

but removing the #[expect] results in:

warning: unused imports: ​`fs​` and ​`io​`
 --> src/main.rs:2:11
  |
2 | use std::{io, fs};
  |           ^^  ^^
  |
  = note: ​`#[warn(unused_imports)]​` on by default

so it is, indeed, supressing the lint - which means the first warning is wrong. it is important to note that the same does not happen on imports of a single item.

rustc version:

rustc 1.81.0-nightly (24d2ac0b5 2024-07-15)
binary: rustc
commit-hash: 24d2ac0b56fcbde13d827745f66e73efb1e17156
commit-date: 2024-07-15
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7

[tgross35]

cc @xFrednet who has been handling expect

[xFrednet]

Thank you for the ping. So my educated guess is that this issue comes from how rustc handles group imports. The import from the issue is internally desugared into three separate imports, like this (Playground "Show HIR"):

#[expect(unused_imports)]
use ::{};
#[expect(unused_imports)]
use std::io;
#[expect(unused_imports)]
use std::fs;

From the text output of HIR it looks like the attributes are duplicated. This in turn creates additional expectations which are not fulfilled.

The question is now, hot to figure out, that these items and attributes all originate from the same item.

@cjgillot do you maybe have an idea on this one?

[cjgillot]

That's not the only place where we duplicate attributes. I had to partially work around it in https://github.com/rust-lang/rust/pull/127313, but that's not a good solution in general.

Right now, the diagnostic infra stores the stable LintExpectationId, we update unstable -> stable before emitting diagnostics, and we try to match the stable id.

What if we did the other way?

I mean:

• In the DiagCtxtInner, store keep fulfilled_expectations: Vec<LintExpectationId> but stop stashing diagnostics to convert unstable->stable version, just emit them. Emitting an unstable id should only happen from pre-lowering code, which is not incremental, so we won't try to decode such a diagnostic to re-emit it.
• In lint_expectations(()), stop computing the unstable -> stable mapping.
• In check_expectations(()), make a FxHashSet<(AttrId, u16)> from the global fulfilled expectations, and use it to check what lint_expectations(()) returns.

Step 3 should be ok for incr comp, as check_expectations is eval_always.

Does that make sense to you?

[xFrednet]

I'm not sure, I fully understand you. Does this rephrasing sound reasonably close?

1. The pre-lowering code, that can emit unstable LintExpectationIDs is always executed. Instead of transforming these ID's we just don't store between compilations as they will be emitted again during the next run.
2. lint_expectations() returns all lint expectations defined by the user.
3. check_expectations() will take the stable IDs and map them to their unstable versions. These unstable ID's are then used to fulfill the expectations from lint_expectations(). And this should fix this issue since the AST hasn't duplicated any attributes yet.

[cjgillot]

Yes.

[xFrednet]

On what level does lint_expectations() operate? Is it on the AST or HIR?

Because, if it's on the HIR, we still need to find a way to map from duplicated attributes back to their original single ones. Don't we?

[cjgillot]

lint_expectations() can operate on either, depending on what is easier to code. You could also have the early lint pass feed it.

check_expectations() can always look at the attribute in HIR to do this mapping. Given id: LintExpectationId, something like:

match id {
    Unstable { attr_id, lint_index: Some(lint_index) } => (attr_id, lint_index),
    Stable { hir_id, attr_index, lint_index: Some(lint_index) } => {
        // We are an `eval_always` query, so looking at the attribute's `AttrId` is ok.
        let attr_id = tcx.hir().attrs(hir_id)[attr_index].id;
        (attr_id, lint_index)
    }
    _ => bug!(),
}

When we duplicate the attributes, we don't change the AttrId, so relying on it is ok.

[xFrednet]

Interesting, yep, then this should work :D

Is this something that can/should be done with https://github.com/rust-lang/rust/pull/127313?

If we want to make it a followup PR, we should probably ping the reviewer or reroll :thinking: