unsafe_op_in_unsafe_fn suggestion overlaps when used in a macro

[ehuss]

The following example triggers the unsafe_op_in_unsafe_fn lint twice in the same position:

#![warn(unsafe_op_in_unsafe_fn)]

macro_rules! foo {
    ($x:ident) => {
        pub unsafe fn $x() {
            let _ = String::new().as_mut_vec();
        }
    };
}

foo!(a);
foo!(b);

This causes cargo fix to apply both suggestions which ends up with:

// ...snip
pub unsafe fn $x() { unsafe { unsafe {
    let _ = String::new().as_mut_vec();
}}}
// ...snip

which triggers the unused_unsafe lint.

Would it be possible to avoid duplicate suggestions when used in a macro?
cc @asquared31415

I suspect this may be encountered frequently with the 2024 edition migration (crater run is pending in #122960). Although it isn't too much of a problem to repair, a frequently used macro could have many unsafe blocks, which could be confusing.

rustc 1.79.0-nightly (8df7e723e 2024-03-30)
binary: rustc
commit-hash: 8df7e723ea729a7f917501cc2d91d640b7021373
commit-date: 2024-03-30
host: aarch64-apple-darwin
release: 1.79.0-nightly
LLVM version: 18.1.2

[ehuss]

Another option I am considering is to have rustfix ignore a top-level diagnostic if all of its suggestions are identical to another top-level diagnostic. That might take a bit of reworking of how rustfix collects suggestions, though. I'm also trying to think of what problems that might introduce, though I think it should be safe.

[weihanglo]

Just looked into the given reproduction. It seems that the duplications happens between different diagnostic messages. I am not sure if such a fix should be in rustfix. Rustfix only deal with one diagnostic at a time. Perhaps we need to get it done at a higher level like here in Cargo? Alternatively, we can have rustfix::CodeFix aware of duplications and skip applying those. Do you have any preference?

[weihanglo]

Is there any situation that we'll get identical machine applicable suggestions, and they are all necessary not duplicated?

[ehuss]

Is there any situation that we'll get identical machine applicable suggestions, and they are all necessary not duplicated?

I brought this up in https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/overlapping.20suggestions, and I and others couldn't think of any concerns with that.

[ehuss]

Rustfix only deal with one diagnostic at a time. Perhaps we need to get it done at a higher level like here in Cargo? Alternatively, we can have rustfix::CodeFix aware of duplications and skip applying those. Do you have any preference?

It looks like in rust-lang/cargo#13728 that you maybe figured this out? The rustfix library handles collecting multiple diagnostics all at once, and that is where it should be able to filter things.

[weihanglo]

The rustfix library handles collecting multiple diagnostics all at once

I didn't fully get it. Did you mean get_suggestions_from_json? Cargo doesn't use that API and instead calls collect_suggestions, which collects from one top-level diagnostics at a time.

rust-lang/cargo#13728 addresses it on Cargo side, not inside rustfix crate. I feel it more nature for rustfix being as dumb as possible, and let Cargo handle business logic. Let me know if you have other thought :)

[ehuss]

Oh, indeed, I was thinking of get_suggestions_from_json. I did not realize it was not used in cargo, though.

The issue with doing it on the cargo side is that we also need to update compiletest to do the same thing (and ui_test and anyone else using it). I think it would be nice to avoid duplicating the logic between cargo and all other users.

Is there some reason cargo doesn't use get_suggestions_from_json? It looks like cargo's collector is a little more resilient, perhaps get_suggestions_from_json could be updated to have the same behavior as the way cargo collects the messages?

[weihanglo]

rust-lang/cargo#13728 has been merged. rustfix@0.8.3 should contain a similar fix when it gets released.

[tmandry]

Sorry, I just caught up on the discussion here. It looks like this can be closed then, now that rust-lang/cargo#13728 is merged?

[tmandry]

I've verified that the reported issue no longer occurs, and the migration behaves correctly with cargo fix.