Fix suggestion spans for expr from macro expansions #112043

jieyouxu · 2023-05-28T15:49:51Z

Issue #112007: rustc shows expanded `writeln!` macro in code suggestion

Before This PR

help: consider using a semicolon here
  |
6 |     };
  |      +
help: you might have meant to return this value
 --> C:\Users\hayle\.rustup\toolchains\nightly-x86_64-pc-windows-msvc\lib/rustlib/src/rust\library\core\src\macros\mod.rs:557:9
  |
55|         return $dst.write_fmt($crate::format_args_nl!($($arg)*));
  |         ++++++                                                  +

After This PR

help: consider using a semicolon here
   |
LL |     };
   |      +
help: you might have meant to return this value
   |
LL |         return writeln!(w, "but not here");
   |         ++++++                            +

Issue #110017: `format!` `.into()` suggestion deletes the `format` macro

Before This PR

help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
 --> /Users/eric/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/macros.rs:121:12
  |
12|         res.into()
  |            +++++++

After This PR

help: call `Into::into` on this expression to convert `String` into `Box<dyn std::error::Error>`
   |
LL |     Err(format!("error: {x}").into())
   |                              +++++++

Fixes #112007.
Fixes #110017.

rustbot · 2023-05-28T15:49:59Z

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

cjgillot · 2023-05-29T07:54:30Z

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

+        let parent_span = self.tcx.hir().span(parent);
+        let span = expr.span.find_ancestor_inside(parent_span).unwrap_or(expr.span);
+        span
+    }


What happens if the function itself is generated by a macro?
What happens if a local macro generates non-trivial code? Should we suggest inside that macro?
Should we stop the parent walk when we stop seeing expressions?

To be clear: I think this heuristic is too strong, and should be refined a bit.

What happens if the function itself is generated by a macro?

~~Hmm this doesn't work correctly, I need to fix it.~~ Do you mean something like this?

macro_rules! entire_fn_outer { () => { entire_fn!(); } } macro_rules! entire_fn { () => { pub fn baz(x: &str) -> Result<(), Box<dyn std::error::Error>> { Err(format!("error: {x}")) //~^ ERROR mismatched types } } } entire_fn_outer!();

What happens if a local macro generates non-trivial code? Should we suggest inside that macro?

I'm having a hard time trying to come up with a test example for this, do you have an example in mind?

Should we stop the parent walk when we stop seeing expressions?

I'm not quite sure about this... I don't think it would be right if we stopped the parent walk when we stop seeing expressions? The parent could very well be items and not expressions?

I'm having a hard time trying to come up with a test example for this, do you have an example in mind?

In your test, having the macro expand to Err(format!(...)), instead of just the format!.

Yeah unfortunately I don't think this can work...

Given something like

macro_rules! nontrivial { ($x: expr) => { Err(format!("error: {}", $x)) //~^ ERROR mismatched types } } pub fn qux(x: &str) -> Result<(), Box<dyn std::error::Error>> { nontrivial!(x) }

my current code will suggest nontrivial!(x).into() which isn't correct. Should we just suppress Into::into suggestion when expr's span comes from a macro expansion...? Seems unfortunate but I can't think of anything that'd be correct when nontrivial macros are involved.

I think it's okay to continue making a suggestion for the macro code, perhaps only if it's a locally-defined macro (if that's easy to check here). I don't see much value in making that suggestion if the macro is defined in an external crate.

jieyouxu · 2023-06-28T09:56:44Z

Changes Since Last Force-Push

Whereas previously I used incorrect parent node searching logic (I meant to have used parent callsites instead), the new changes differentiates treatment of finding a suitable span for suggesting return expr; vs expr.into() when local macros are involved.

For suggesting .into(), we now recursively try to look for the ancestor span which shares the same context as the initial expr.span. This will recursively look into local macros until the span inside the most ancestor local macro is found. It will stop recursing as soon as the context of a potential parent callsite changes, such as if the potential parent callsite is in a foreign macro.

For suggesting return expr;, we use Span::find_ancestor_inside to find an ancestor for expr.span which is inside the owner node of the function. This will be less aggressive than the .into() suggestion, and avoids looking into local macros, but instead tries to stay as close to the function as possible.

Also made sure to have run-rustfix on the tests.

Note that I'm not 100% confident on the correctness of these two approaches. @rustbot ready

davidtwco

This seems good to me now, but I'm not 100% confident there aren't cases which wouldn't work. Does this resolve your previous concerns, @cjgillot?

cjgillot · 2023-08-03T08:45:30Z

Thanks.
Let's go with this.
@bors r+

bors · 2023-08-03T08:45:32Z

📌 Commit 25d065b has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-08-03T10:01:20Z

⌛ Testing commit 25d065b with merge c115ec1...

bors · 2023-08-03T11:48:25Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing c115ec1 to master...

rust-timer · 2023-08-03T21:33:02Z

Finished benchmarking commit (c115ec1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 649.583s -> 649.925s (0.05%)

ehuss · 2024-04-08T20:15:14Z

@jieyouxu The test added here is causing some issues with updating rustfix. Newer versions of rustfix are better at applying certain kinds of suggestions, but unfortunately the ui/typeck/issue-112007-leaked-writeln-macro-internals.rs test still has a suggestion that tries to modify an external crate (libcore).

Do you think you would be able to help look at trying to fix that?

You can reproduce by changing src/tools/compiletest/Cargo.toml to have rustfix = "0.8.1".

The problem is that the diagnostic has a suggestion to add a ?:

use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller

and it points to inserting the ? into library/core/src/macros/mod.rs. I'm guessing that particular suggestion also needs to do a similar dance as what this PR does of peeling back the span to point to the user's code?

jieyouxu · 2024-04-08T20:19:43Z

The test added here is causing some issues with updating rustfix. Newer versions of rustfix are better at applying certain kinds of suggestions, but unfortunately the ui/typeck/issue-112007-leaked-writeln-macro-internals.rs test still has a suggestion that tries to modify an external crate (libcore).

That's... unfortunate. I'll take a look.

jieyouxu · 2024-04-08T20:26:21Z

Am I understanding correctly that because rustfix got better at applying certain suggestions, it exposed another case of pointing into library code?

ehuss · 2024-04-08T20:26:55Z

Am I understanding correctly that because rustfix got better at applying certain suggestions, it exposed another case of pointing into library code?

Yea.

ehuss · 2024-04-08T20:27:02Z

Hm, thinking about it some more, just fixing the span of the ? suggestion won't work, since the test won't be able to apply both the return…; and ? suggestions at the same time (they conflict with one another).

I'm not sure how to fix that.

jieyouxu · 2024-04-08T20:28:35Z

That is extremely unfortunate. How would rustfix/rustc even handle conflict resolution here? It's not immediately obvious to me which suggestion is better... I would also think that it's not really a rustfix problem, but a rustc problem, since rustc is emitting conflicting machine applicable suggestions?

ehuss · 2024-04-08T20:34:34Z

It's not uncommon for rustc to suggest conflicting suggestions, which is fine as long as they are not marked MachineApplicable. Sometimes there really are several legitimate options, and it isn't feasible for rustc to know which one is the correct one. The user has to decide.

On the rustfix side, it currently only handles MachineApplicable suggestions in cargo fix, so we haven't worried about this too much. In the future we want to add an interactive mode to cargo fix to allow the user to choose from multiple options when there is a choice (of MaybeIncorrect choices), but it would have to be a manual choice.

UI tests are a little unusual in that they apply all applicabilities by default, even if they are "maybe incorrect" or "has placeholders". For example, issue-112007-leaked-writeln-macro-internals.rs has both MaybeIncorrect and HasPlaceholders suggestions, which are ignored by cargo fix.

jieyouxu · 2024-04-08T20:38:49Z

Wait, I forgot this wasn't even machine applicable suggestions, both the return ... ; and ( ... ).into() in this PR are MaybeIncorrect

jieyouxu · 2024-04-08T20:44:55Z

Welp, one step at a time. I'll try to fix the ? pointing into library issue first. Still not sure how to fix overlapping MaybeIncorrect suggestions. I wonder if we have to deal with this on the compiletest side to somehow determine that if we have overlapping MaybeIncorrect suggestions, that we try to synthesize multiple .fixed files and verify each of them individually compiles...?

ehuss · 2024-04-08T21:01:18Z

Detecting conflicting suggestions seems like an interesting idea. Perhaps it could assume that all non-MachineApplicable sub-diagnostics are conflicting, and apply them separately. That would only work assuming there aren't any diagnostics where separate messages depend on one another, but I'm not aware of any offhand that are like that. Most should work like the return...? in this PR where they are separate spans within the same message, which should be applied together (which in rustfix terminology is a single Solution).

ehuss · 2024-04-08T21:04:00Z

BTW, thanks for responding so fast and taking a look! 😃

jieyouxu · 2024-04-08T21:07:06Z

I could try to patch compiletest to make the assumption that each MaybeIncorrect (actually, it might need to assume all diagnostics) may conflict such that:

for each suggestion:
- we apply a single solution to synthesize a fixed source file
- try to compile the fixed source file and see if it errors

This might regress ui test suite time by quite a bit, definitely will have to do a perf run.

I also need to patch up compiletest's error reporting when the fixed code doesn't compile so it actually reportings something that helps to debug...

jieyouxu · 2024-04-08T21:29:20Z

just fixing the span of the ? suggestion won't work

well the fix for ? suggestion span seems rather easy, it's exactly the same logic as the (...).into() span in this PR...

diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
index f09af999957..d349da95266 100644
--- a/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
+++ b/compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
@@ -1927,8 +1927,14 @@ pub(crate) fn suggest_missing_unwrap_expect(
             None => sugg.to_string(),
         };

+        let mut span = expr.span;
+        while expr.span.eq_ctxt(span) && let Some(parent_callsite) = span.parent_callsite()
+        {
+            span = parent_callsite;
+        }
+
         err.span_suggestion_verbose(
-            expr.span.shrink_to_hi(),
+            span.shrink_to_hi(),
             msg,
             sugg,
             Applicability::HasPlaceholders,

jieyouxu · 2024-04-08T21:31:43Z

and we're just left with problem of incompatible MaybeIncorrect suggestions:

error[E0308]: `?` operator has incompatible types
  --> /home/gh-jieyouxu/rust/tests/ui/typeck/question-mark-operator-suggestion-span.fixed:8:16
   |
LL |         return writeln!(w, "but not here")?;
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `Result<(), Error>`, found `()`
   |
   = note: `?` operator cannot convert from `()` to `Result<(), std::fmt::Error>`
   = note:   expected enum `Result<(), std::fmt::Error>`
           found unit type `()`
help: try removing this `?`
   |
LL -         return writeln!(w, "but not here")?;
LL +         return writeln!(w, "but not here");
   |
help: try wrapping the expression in `Ok`
   |
LL |         return Ok(writeln!(w, "but not here")?);
   |                +++                            +

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0308`.

jieyouxu · 2024-04-08T21:53:06Z

Ah actually this is more complicated than I expected: compiletest emits .fixed files, so we actually track the fixed files in version control. If I try to split solutions and apply each solution individually, i.e. one .fixed each, I am met with the problem of how do I guarantee that the .fixed files can be reproduced (as in, how do I encode their filename)?

It can't be derived from hash of a Suggestion, because if rustfix changes anything about a Suggestion then all the run-rustfix tests will need to be reblessed.
It can't be ordered, because if we change diagnostic order then the .fixed file ordering completely breaks.

jieyouxu · 2024-04-08T22:12:28Z

Actually, we should just do the simplest and most correct thing here:

delete //@ run-rustfix and .fixed file for that test

This will definitely fix the issue. And since the suggestions are MaybeIncorrect anyway, it's okay if they conflict and don't cleanly apply together, we can verify that they don't leak stdlib implementation details already via stderr, we don't need .fixed!

compiler-errors · 2024-04-08T22:56:25Z

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

+            let mut span = expr.span;
+            while expr.span.eq_ctxt(span) && let Some(parent_callsite) = span.parent_callsite()
+            {
+                span = parent_callsite;
+            }


Why is this different than Span::find_ancestor_in_same_ctxt?

…rieril typeck: fix `?` suggestion span Noticed in <rust-lang#112043 (comment)>, if the ``` use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller ``` suggestion is applied to a macro that comes from a non-local crate (e.g. the stdlib), the suggestion span can become non-local, which will cause newer rustfix versions to fail. This PR tries to remedy the problem by recursively probing ancestors of the expression span, trying to identify the most ancestor span that is (1) still local, and (2) still shares the same syntax context as the expression. This is the same strategy used in rust-lang#112043. The test unfortunately cannot `//@ run-rustfix` because there are two conflicting MaybeIncorrect suggestions that when collectively applied, cause the fixed source file to become non-compilable. Also avoid running `//@ run-rustfix` for `tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs` because that also contains conflicting suggestions. cc `@ehuss` who noticed this. This question mark span fix + not running rustfix on the tests containing conflicting MaybeIncorrect suggestions should hopefully unblock rustfix from updating.

Rollup merge of rust-lang#123654 - jieyouxu:question-mark-span, r=Nadrieril typeck: fix `?` suggestion span Noticed in <rust-lang#112043 (comment)>, if the ``` use the `?` operator to extract the `Result<(), std::fmt::Error>` value, propagating a `Result::Err` value to the caller ``` suggestion is applied to a macro that comes from a non-local crate (e.g. the stdlib), the suggestion span can become non-local, which will cause newer rustfix versions to fail. This PR tries to remedy the problem by recursively probing ancestors of the expression span, trying to identify the most ancestor span that is (1) still local, and (2) still shares the same syntax context as the expression. This is the same strategy used in rust-lang#112043. The test unfortunately cannot `//@ run-rustfix` because there are two conflicting MaybeIncorrect suggestions that when collectively applied, cause the fixed source file to become non-compilable. Also avoid running `//@ run-rustfix` for `tests/ui/typeck/issue-112007-leaked-writeln-macro-internals.rs` because that also contains conflicting suggestions. cc `@ehuss` who noticed this. This question mark span fix + not running rustfix on the tests containing conflicting MaybeIncorrect suggestions should hopefully unblock rustfix from updating.

rustbot assigned davidtwco May 28, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 28, 2023

cjgillot reviewed May 28, 2023

View reviewed changes

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs Outdated Show resolved Hide resolved

jieyouxu force-pushed the suggestion_macro_expansion_source_callsites branch from 1b2b9ba to ad127e4 Compare May 29, 2023 01:49

cjgillot reviewed May 29, 2023

View reviewed changes

jieyouxu force-pushed the suggestion_macro_expansion_source_callsites branch 2 times, most recently from bc91f84 to 711f6b6 Compare May 29, 2023 09:48

jieyouxu force-pushed the suggestion_macro_expansion_source_callsites branch 2 times, most recently from 544f080 to bcdffdb Compare June 10, 2023 09:38

davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2023

Fix suggestions span for when expr is from macro expansions

25d065b

jieyouxu force-pushed the suggestion_macro_expansion_source_callsites branch from bcdffdb to 25d065b Compare June 28, 2023 09:55

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 28, 2023

davidtwco approved these changes Jun 28, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 3, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2023

bors merged commit c115ec1 into rust-lang:master Aug 3, 2023

rustbot added this to the 1.73.0 milestone Aug 3, 2023

GrigorenkoPV mentioned this pull request Mar 28, 2024

rustc error report suggests uncompilable #[derive(PartialEq.into())] #123132

Open

jieyouxu deleted the suggestion_macro_expansion_source_callsites branch April 8, 2024 20:19

jieyouxu mentioned this pull request Apr 8, 2024

typeck: fix ? suggestion span #123654

Merged

compiler-errors reviewed Apr 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix suggestion spans for expr from macro expansions #112043

Fix suggestion spans for expr from macro expansions #112043

jieyouxu commented May 28, 2023 •

edited

Loading

rustbot commented May 28, 2023

cjgillot May 29, 2023

jieyouxu May 29, 2023 •

edited

Loading

cjgillot Jun 10, 2023

jieyouxu Jun 10, 2023 •

edited

Loading

davidtwco Jun 27, 2023

jieyouxu commented Jun 28, 2023 •

edited

Loading

davidtwco left a comment

cjgillot commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 3, 2023

rust-timer commented Aug 3, 2023

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

ehuss commented Apr 8, 2024

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 •

edited

Loading

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

ehuss commented Apr 8, 2024

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

compiler-errors Apr 8, 2024

Fix suggestion spans for expr from macro expansions #112043

Fix suggestion spans for expr from macro expansions #112043

Conversation

jieyouxu commented May 28, 2023 • edited Loading

Issue #112007: rustc shows expanded writeln! macro in code suggestion

Before This PR

After This PR

Issue #110017: format! .into() suggestion deletes the format macro

Before This PR

After This PR

rustbot commented May 28, 2023

Choose a reason for hiding this comment

jieyouxu May 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu Jun 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Jun 28, 2023 • edited Loading

Changes Since Last Force-Push

davidtwco left a comment

Choose a reason for hiding this comment

cjgillot commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 3, 2023

bors commented Aug 3, 2023

rust-timer commented Aug 3, 2023

Overall result: no relevant changes - no action needed

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

ehuss commented Apr 8, 2024

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 • edited Loading

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 • edited Loading

jieyouxu commented Apr 8, 2024 • edited Loading

ehuss commented Apr 8, 2024

ehuss commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024

jieyouxu commented Apr 8, 2024 • edited Loading

jieyouxu commented Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu commented May 28, 2023 •

edited

Loading

Issue #112007: rustc shows expanded `writeln!` macro in code suggestion

Issue #110017: `format!` `.into()` suggestion deletes the `format` macro

jieyouxu May 29, 2023 •

edited

Loading

jieyouxu Jun 10, 2023 •

edited

Loading

jieyouxu commented Jun 28, 2023 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading

jieyouxu commented Apr 8, 2024 •

edited

Loading