Tweak diagnostic for use suggestion to blank text surrounding span.

[pnkfelix]

Our suggestion system is oriented around diff's to code, and in that
context, if your spans are perfect, everything works out. But our
spans are not always perfect, due to macros.

In the specific case of issue #87613, the #[tokio::main] attribute
macro works by rewriting an async fn main into an fn main, by just
removing the async token. The problem is that the async text
remains in the source code, and the span of the code rewritten by
suggestion system happily transcribes all the text on the line of that
fn main when generating a use suggestion, yielding the absurd
looking async use std::process::Command; suggestion.

This patch works around this whole problem by adding a way for
suggestions to indicate that their transcriptions should not include
the original source code; just its whitespace. This leads to a happy
place where the suggested line numbers are correct and the indentation
is usually also correct, while avoiding the async use output we
would see before.

Fix #87613

[rust-highfive]

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

[pnkfelix]

I'd like to get @estebank 's take on this, if they have time.

[pnkfelix]

r? @estebank

[estebank]

I've reread the PR a couple of times now, and I think I get all that it's doing, but will need to look at it again tomorrow morning.

The visual output looks obviously correct, but now I'm concern that applying the suggestion from the json output will still result in the original bug. Am I wrong to imagine that?

I'm not sure how much time it would take, but changing how we compute use_placement_span would also be a potential fix for this, right? I'm guessing you already explored that. What did you find?

[estebank]

Could we make it run-rustfix? That way we enforce that the emitted suggestion is actually valid.

[pnkfelix]

Adding // run-rustfix to this test does not work out of the box. It ends up rewriting the code to the bad thing that we are trying to prevent:

#[amputate_span::drop_first_token]
/* what the
hey */ async use std::process::Command;

fn main() {
    Command::new("git"); //~ ERROR [E0433]
}

and

#[amputate_span::drop_first_token]
/* what the
hey */ async use std::process::Command;

fn main() {
    Command::new("git"); //~ ERROR [E0433]
}

I'm guessing that's because rustfix does not know Transcription::Blanks?

[pnkfelix]

This may be somewhat related to this FIXME from PR #85427 that originally stems from PR #44215. Fixing that might serve as an argument for moving away from the "infer the span from the given item's span" strategy, and using the span of the containing mod/crate instead, as discussed in my other comment

rust/src/test/ui/resolve/use_suggestion_placement.fixed

Lines 17 to 24 in 934624f

	// FIXME: UsePlacementFinder is broken because active attributes are
	// removed, and thus the `derive` attribute here is not in the AST.
	// An inert attribute should work, though.
	// #[derive(Debug)]
	use std::path::Path;
	#[allow(warnings)]
	pub struct Foo;

[pnkfelix]

I'm not sure how much time it would take, but changing how we compute use_placement_span would also be a potential fix for this, right? I'm guessing you already explored that. What did you find?

Yeah, I did explore this. I posted my findings in zulip, but here's the executive summary: Any span we choose here is going to be a heuristic (there's no way for us to infer what the end user would actually prefer, what rustfmt customizations they have, etc). The span of the item is usually an excellent basis for that heuristic.

(More long winded explanation follows.)

In this case, the span of the item has yielded non-ideal results because the procedural macro has literally thrown away the token that holds the ideal starting point for the span. The only way we could try to correct for that within the compiler would be to use the span that was associated with the item prior to expansion, and I'm not sure that we can distinguish scenarios where is the right thing to do (versus ones where it would lead us astray).

If you don't use the item (pre- or post-expansion) as the basis for the span, then you have to figure out what span you are going to use. I explored using the span of the mod/crate itself. The options there are either 1. the span of the mod/crate contents (which is readily available), or 2. the span of the whole module/crate (i.e. include the mod IDENT { ... }). But either option is going to require correction.

E.g. I tried option 1 (the span of the mod/crate contents), but that will cause you to emit suggestions like mod foo {use path::to::Item;, with no newline nor identation for the newly added item. I spent a little while trying to figure out how to infer the indentation, but such inference requires you derive it from some other span (you cannot use the contents span, because the column number will vary depending on how long the module name is). And all the experiments I did in this vein ended up causing a lot of changes (arguably regressions) to other test output.

In the end, the approach I'm suggesting here just seemed best. It is relatively self-contained, and ends up having minimal perturbation to our test suite (and hopefully minimal change to end-users), because the only significant change to behavior is that with this change, the suggestion stops including things like trailing text from /* ... */ comments that precede the item on its first line. (That's what the hey */ stuff is about in the test case I put in here.)

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_errors/src/lib.rs at line 222:
     fn transcribe<'a>(&self, text: Option<Cow<'a, str>>) -> Option<Cow<'a, str>> {
         match self.transcription {
             Transcription::Copy => text,
-            Transcription::Blank => text.map(|s| {
-                s.chars().map(|c| if c.is_whitespace() { c } else { ' ' }).collect()
-            }),
+            Transcription::Blank => {
+                text.map(|s| s.chars().map(|c| if c.is_whitespace() { c } else { ' ' }).collect())
         }
     }
 
 
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_target/src/spec/mipsel_sony_psp.rs" "/checkout/compiler/rustc_target/src/spec/thumbv7neon_unknown_linux_musleabihf.rs" "/checkout/compiler/rustc_target/src/spec/i686_unknown_netbsd.rs" "/checkout/compiler/rustc_target/src/spec/l4re_base.rs" "/checkout/compiler/rustc_errors/src/styled_buffer.rs" "/checkout/compiler/rustc_target/src/spec/armv7_unknown_linux_gnueabi.rs" "/checkout/compiler/rustc_errors/src/lib.rs" "/checkout/compiler/rustc_target/src/spec/wasm32_unknown_unknown.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

[Mark-Simulacrum]

If the goal is to preserve indentation, I think removing any non-whitespace characters may be better? For example, in the async fn case, you probably want use aligned with async, not indented to the fn.

[pnkfelix]

Well, then trailing ... */ comments (which would likewise get removed under your suggestion) end up messing up the indentation?

And I guess if the item is on the same line as its containing module, we might likewise have a problem:

mod m { fn foo() { Command::new("git"); } }

But I freely admit, both of these cases sound much more rare than #[tokio::main]. :)

[pnkfelix]

(I think I'm gradually talking myself into going back and looking at deriving the span from the span of the containing mod/crate again, despite my earlier frustrations with that approach.)

[pnkfelix]

Discussed this with some other members of T-compiler. Collectively agreed that this is not the right approach; the better solution here is to have the parser identify one spot (or more) where use items can/should be injected.

Closing this PR since I'm not planning to continue with it.