More robust fallback for use suggestion

[pnkfelix]

Our old way to suggest where to add uses would first look for pre-existing uses in the relevant crate/module, and if there are no uses, it would fallback on trying to use another item as the basis for the suggestion.

But this was fragile, as illustrated in issue #87613

This PR instead identifies span of the first token after any inner attributes, and uses that as the fallback for the use suggestion.

Fix #87613

[rust-highfive]

Some changes occurred in src/tools/rustfmt.

cc @calebcartwright

[rust-highfive]

r? @estebank

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

[pnkfelix]

There is still one case that this arguably doesn't handle perfectly, namely this:

/* hey
whazzup */ fn main() {
    Command::new("git outta here");
}

where this still suggests:

2 | whazzup */ use std::process::Command;
  |            ++++++++++++++++++++++++++

(but that suggestion isn't wrong, and I expect that case to be absurdly rare, so I think its reasonable to just go ahead with this approach.)

[calebcartwright]

re: The rustfmt test failure, I'd think a fmt/tidy run would sort it.

That particular test is a bootstrapping sort for rustfmt which runs rustfmt against itself. Technically that means in this repository that test runs with a slightly different version of rustfmt than what x.py has pinned, so let me know if there's anything strange/conflicting with the formatting there.

Edit: Disregard the above. Believe manual formatting on the subtree is required when editing in tree, in part because of those differences between the two repos in how formatting is executed

[ehuss]

Nice!

There is a duplicate use placement finder at find_use_placement. Is it feasible to keep the implementations in sync?

Can you also update use_suggestion_placement.rs to remove the FIXME? Essentially something like:

--- a/src/test/ui/resolve/use_suggestion_placement.rs
+++ b/src/test/ui/resolve/use_suggestion_placement.rs
@@ -10,11 +10,7 @@ mod m {
 }

 mod foo {
-    // 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)]
-    #[allow(warnings)]
+    #[derive(Debug)]
     pub struct Foo;

     // test whether the use suggestion isn't

[pnkfelix]

There is a duplicate use placement finder at find_use_placement. Is it feasible to keep the implementations in sync?

Oh, I don't know! I didn't know about that, but it seems obvious that I should try to do that.

Can you also update use_suggestion_placement.rs to remove the FIXME? Essentially something like:

Will do!

[pnkfelix]

Nice!

There is a duplicate use placement finder at find_use_placement. Is it feasible to keep the implementations in sync?

Okay, I looked into doing this.

Unfortunately, the code I'm adding here is using the AST, but by the time typeck runs, the AST is long gone.

I don't want to block this PR on figuring out the best way to bring these two parts into sync. (E.g. one could thread down the same span information that I attached to the AST.) I will add a fixme at least, though.

[pnkfelix]

@calebcartwright can you explain this part of the rustfmt formatting error to me? (see below)

Mismatch at src/parse/parser.rs:113:
         let result = catch_unwind(AssertUnwindSafe(|| {
             let mut parser = new_parser_from_file(sess.inner(), path, Some(span));
             match parser.parse_mod(&TokenKind::Eof) {
-                Ok((a,
+                    a,

                     ast::ModSpans {
                         inner_span,
                         inner_span,

namely, why on earth is the diff saying that its removing Ok(( from the text?

Was there a line that got removed in the report? I would have perhaps expected to see:

             match parser.parse_mod(&TokenKind::Eof) {
-                Ok((a,
+                Ok((
+                    a,
                     i,

[calebcartwright]

It does look a bit odd @pnkfelix, I haven't noticed it before. Worth noting though that these tests are using an internal, rudimentary diff emitter because of where/how the test is constructed, and it's not the same emitter and output for standard end user invocations of rustfmt.

May take a look at this if we get time, but not too concerned given it's only seen by the small audience of those modifying rustfmt source. Here's what an end user would see and what I would expect:

             match parser.parse_mod(&TokenKind::Eof) {
-                Ok((a, i, ast::ModSpans { inner_span, inject_use_span: _ })) => Some((a, i, inner_span)),
+                Ok((
+                    a,
+                    i,
+                    ast::ModSpans {
+                        inner_span,
+                        inject_use_span: _,
+                    },
+                )) => Some((a, i, inner_span)),

[estebank]

r=me with or without the change to the test.

The follow up work to keep the suggestion span in the hir might be a good first-timer issue.

[camelid]

This bug has driven me nuts for a really long time, so thanks for fixing it ❤️

[pnkfelix]

@bors r=ekuber rollup+

[bors]

📌 Commit 8f4c6b0 has been approved by ekuber

[pnkfelix]

The follow up work to keep the suggestion span in the hir might be a good first-timer issue.

Filed as #94941

[bors]

⌛ Testing commit 8f4c6b0 with merge 95561b3...

[bors]

☀️ Test successful - checks-actions
Approved by: ekuber
Pushing 95561b3 to master...

[rust-timer]

Finished benchmarking commit (95561b3): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression