Suppress suggestions while span is in external library by xizheyin · Pull Request #139316 · rust-lang/rust (original) (raw)

Status: Awaiting review from the assignee but also interested parties.

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Apr 3, 2025

Signed-off-by: xizheyin xizheyin@smail.nju.edu.cn

Werid Point
In the first commit, I use . /x test tests/ui could not reproduce the issue in issue, however I was able to reproduce it using the compiled out version of stage1 in my computer.

I've changed the code. But the strange points are still not solved, does it matter? @fmease

r? @fmease

But the strange points are still not solved

The difference you're likely seeing is the availability vs. unavailability of std's source code. I don't quite remember the reasons as to why the sources of the stage1(?) sysroot aren't available for tests/ui/ tests but in any case whenever they aren't available, some kinds of subdiagnostics simply won't get shown by the diagnostic emitter IIRC.

Regarding the issues you're addressing with this PR as well as with #139399 and #139403, I'm currently leaning towards suppressing structured suggestions wholesale if they point into the (local) expansion of an external macro, rather than sprinkling these checks all over the compiler.

What do you think, @rust-lang/wg-diagnostics?

The downside of hiding those suggestions: The end user might actually have control over the external crate (multi-crate projects are quite common).

I'm also asking myself whether issues #139251, #139253, #139050 etc. are recent(?) regressions? And if so, whether they were intentional. We might want to bisect them (e.g., with cargo-bisect-rustc).

I agree with you, because there are many such cases, it seems that it is a good idea to comprehensively suppress this behavior in diagnostic suggestions.

For the scope of suppression, maybe we can limit it to the standard library, not all external libraries, which can not be causually changed (I don't know if there is information to judge whether it comes from the standard library). Or just suppress external macros that are not from the current workspace, but I don't know if there is such fine information.

I tried cargo-bisect-rustc, and found something interesting.

When I switched the toolchain to nightly or stable, which are both new versions i.e. rustc 1.88.0-nightly (092a284ba 2025-04-13) and rustc 1.86.0 (05f9846f8 2025-03-31)), suggestions in the standard library were showing up.

But I switched the toolchain to the specific version, i.e. nightly-2025-04-13, the suggestion was gone.

$ rustup default nightly-2025-04-14
info: using existing install for 'nightly-2025-04-14-x86_64-unknown-linux-gnu'
info: default toolchain set to 'nightly-2025-04-14-x86_64-unknown-linux-gnu'

  nightly-2025-04-14-x86_64-unknown-linux-gnu unchanged - rustc 1.88.0-nightly (092a284ba 2025-04-13)

The same toolchain, except that one installs via NIGHTLY and the other installs via a specified date, yielding different results.
I need to check the appropriate source code. Do you have any suggestions? @fmease

Signed-off-by: xizheyin xizheyin@smail.nju.edu.cn

I modified HumanEmitter::emit_suggestion_default to suppress suggestions when the span's filename contains an external library in .cargo/registry/src/ or standard library in .rustup/toolchains/, which reduces false positives a lot. @fmease

xizheyin changed the title~~Suppress suggestions on deref in macro when == is in the macro~~ Suppress suggestions while span is in external library

Apr 14, 2025

Choose a reason for hiding this comment

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

I'll try to bisect later to see if these issues represent regressions or not.

Signed-off-by: xizheyin xizheyin@smail.nju.edu.cn

Comment on lines +3550 to +3559

let cargo_home = match std::env::var("CARGO_HOME") {
Ok(dir) => std::path::PathBuf::from(dir),
Err(_) => {
if let Ok(home) = std::env::var("HOME") {
std::path::PathBuf::from(home).join(".cargo")
} else {
return false;
}
}
};

Choose a reason for hiding this comment

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

Discussion: this feels really iffy to me, that we're querying so many env vars. E.g. I think on Windows, HOME isn't even set. This also magically encodes directory structures that are not guaranteed to remain the same (rustup, cargo, sysroot directory layouts). See for instance #135278 and #135501. Note that this also can't account for remapped paths or devirtualizing of /rustc/$hash, I think.

Choose a reason for hiding this comment

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

Yeah, for sure. This isn't the approach we should be taking. When I mentioned RUSTUP_HOME and CARGO_HOME in a previous review comment, I intended to dissuade the author from implementing this kind of approach by demonstrating the fragile nature, the amount of hacks/assumptions and unnecessary coupling to downstream tools.

I haven't re-reviewed yet because I need to think about a non-hacky approach but I first wanted to investigate things.

Choose a reason for hiding this comment

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

Yeah, that's the feeling I had from reading the previous review comments. I mostly wrote my own comment to prevent myself from forgetting 😆

Choose a reason for hiding this comment

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

I probably understand. I'm not familiar with this section and may need to investigate further. Are there any other resources I can refer to?

std sources w.r.t. diagnostics aren't available in ui tests because we explicitly set -Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX + -Ztranslate-remapped-path-to-local-path=no

// Hide libstd sources from ui tests to make sure we generate the stderr
// output that users will see.
// Without this, we may be producing good diagnostics in-tree but users
// will not see half the information.
//
// This also has the benefit of more effectively normalizing output between different
// compilers, so that we don't have to know the `/rustc/$sha` output to normalize after the
// fact.
rustc.arg("-Zsimulate-remapped-rust-src-base=/rustc/FAKE_PREFIX");
rustc.arg("-Ztranslate-remapped-path-to-local-path=no");

See for instance tests/codegen/remap_path_prefix/issue-73167-remap-std.rs, and maybe #83813, #105907, #129687.