Suggest only Span without source changes when source code is unavailable

[xizheyin]

View all comments

UI tests hide std library sources so diagnostics match what users may see when std sources are unavailable.
In those cases, suggestions whose spans point into std macros cannot be rendered as source patches.

Before this change, if all substitutions for a suggestion were filtered out because the source could not be loaded, the suggestion was dropped entirely. This meant useful help messages could disappear in UI test output, while it will appear in user's terminal.

This adds a -Zshow-suggestions-with-unavailable-source flag and enables it for UI tests. When the annotate-snippet emitter cannot render a suggestion as a patch, but the original suggestion points to unavailable source, it now emits a span-only help instead:

help: consider using `drop` function
  --> $SRC_DIR/core/src/macros/mod.rs:LL:COL

cc #139316 (comment)

[rustbot]

jieyouxu is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

Some changes occurred in src/tools/compiletest

cc @jieyouxu

[xizheyin]

Looks like jieyou is busy

r? compiler

[jieyouxu]

This feels very sus. How does this handle stable vs beta vs nightly vs in-tree std sources?

I feel like there's a reason why ui tests don't get to access std sources, which is to prevent different diagnostics depending on availability of std sources, and which std sources?

[xizheyin]

How does this handle stable vs beta vs nightly vs in-tree std sources?

Is it possible to normalize the content of the source code and just let us know that the source code is displayed in stderr?

I'm curious what kind of impact different versions (stable/nightly/beta/in-tree) would have, I don't know enough about CI. Any examples of this?

[xizheyin]

Path remapped is introduced in #105500 may cc @oli-obk
Indeed it's not that simple, some of the existing test surprisingly shows something like

note: required by a bound in `Option`
  --> $SRC_DIR/core/src/option.rs:LL:COL

, just without the source code...

[xizheyin]

I've written up this change in the PR description above, and it should have a similar mechanism to span_note for now.

[estebank]

I don't think we should be suggesting changes to code the user doesn't own, specially for stdlib. For a suggestion here it should be pointing at the parameters instead (which might be difficult to accomplish).

[xizheyin]

Yes, this is exactly the problem we need to solve.

This pr is to solve the problem that UI test can't be reproduced to make suggestions in std. Being able to reproduce compiler bugs will make review easier.

[jieyouxu]

This pr is to solve the problem that UI test can't be reproduced to make suggestions in std.

I'm not 100% convinced this is the right problem to solve -- do we actually need the standard library sources, or can we just s/std/external crate/?

[xizheyin]

Another way is to use two files to represent two crate in ui test to simulate this situation. (If full reproduction is not required)

That is what this comment points out.
#144266 (comment)

But I think it is reasonable to be able to reproduce any bug in the test.

@compiler-errors What do you think?

[xizheyin]

For further explanation, I moved the comments here.

When fixing the problems listed in #142403, we found that the bug could not be reproduced in ui test. After investigation, I found that span suggestion and span note are not realized in the same way, so I submitted this pr to make std visible in ui tests.

In daily use, the source of std should be visible, and execution will fall to the else branch, so these will not be displayed.

This new test is a bug recorded in #142403, only for verifying this code change, I will fix it in later pr.

[xizheyin]

r? @estebank

[wesleywiser]

Hi @estebank and @jieyouxu, do you feel like this PR is ready for review again or do we need more design work to be done?

[estebank]

I think the changes are ok, but I would like to restrict the output at least to nightly and ideally behind a -Z flag that the test suite enables, but otherwise keeps the current output. The reason is that these are unactionable suggestions that shouldn't be emitted almost ever. Surfacing them so that we can debug them better and detect mistakes in the test suite? That's excellent. Showing them to end users? Most often than not they will be annoying at best, and confusing at worst (I can totally imagine blindly someone modifying the sources of a crate in their cargo directory without realizing what they were doing).

CC @Muscraft for output changes.

[apiraino]

Sorry lost track of this. Seems the last comment implies that @xizheyin can do some changes. Switching to waiting on author to incorporate changes. Feel free to request a review with @rustbot ready, thanks!

@rustbot author

[rustbot]

rustc_errors::annotate_snippet_emitter_writer was changed

cc @Muscraft

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[xizheyin]

I set it as a new zflag, enabled only during UI tests: if the source code isn't available, then print "span".

The results were as expected; the blessed UI test exposed some existing diagnostic bugs in the compiler, printing the contents of external macros(in std library). However, I think this might need to be fixed in a future PR.

@rustbot ready

View changes since this review

[xizheyin]

I added a option show_suggestions_with_unavailable_source.

[xizheyin]

diagnostic bug?