Early return if span is from expansion so we dont get empty span and ice later on

[Kivooeo]

Fixes #147255

The problem original was from that stmt.span was from expansion and it span was bigger than right part which is block.span, so it causes empty span and panic later on, I decided to add checks for both of them to be on the safe side

r? @fmease (you were in discussion on this issue so I decided to assign you, feel free to reroll)

[rustbot]

fmease is not on the review rotation at the moment.
They may take a while to respond.

[chenyukang]

these errors are recurring, how can we implement a long-term fix.
i remember we had a try, but it was blocked by performance regression.

[Kivooeo]

This should not affect performance. As for a long-term fix, I’m not sure we can implement one, since it may occur in unexpected places. We can probably fix issues like this only gradually over time

[Kivooeo]

For example this ICE was since 1.86, which is pretty old, but I have no idea what has changed after 1.85 that could break this

[chenyukang]

This should not affect performance. As for a long-term fix, I’m not sure we can implement one, since it may occur in unexpected places. We can probably fix issues like this only gradually over time

i'm referring this one which have perf regression: #109082

i have a revisit on that PR, seems it was fixing a different issue (but general in similar kind).

[fmease]

Using .from_expansion() is always a "heavy hammer" (see also #147577 (comment)) since it leads to us flat out suppressing diagnostics once macros are in the vicinity even if they don't pose any problem.

In #147255 (comment), you wrote that tail_expr.span and stmt.span were identical. However, that seems a bit surprising to me because "usually" (i.e.., when no macros are involved), the stmt span should include the ; while the expr span should not. Consider:

fn main() {
    let s = {
        "hello"    ;
    };
    println!("{}", s);
}

Using a nightly/master rustc (output trimmed by me):

error[E0277]: `()` doesn't implement `std::fmt::Display`
 --> src/main.rs:5:20
  |
3 |         "hello"    ;
  |                ----- help: remove this semicolon
4 |     };

As you probably already know with stmt.span.with_lo(tail_expr.span.hi()) later on we try to "diff" the stmt span (which includes the semicolon) with the inner expr one (which can include whitespace as seen above for demo purposes).

I mean I have no reason to doubt that what you wrote in #147255 (comment) is accurate. If they indeed the same span if a macro is involved, then I would consider that a bug / unfortunate decision in macro expansion (it would ignore the SyntaxContext of the ; when marking or concatenating Spans maybe)?

If they were different, we could then simply tail_expr.span.find_ancestor_in_same_ctxt(stmt.span) and use that inside stmt.span.with_lo(_).

[fmease]

Using .from_expansion() is always a "heavy hammer" (see also #147577 (comment)) since it leads to us flat out suppressing diagnostics once macros are in the vicinity even if they don't pose any problem.

E.g., it means that under your PR we would suppress the semicolon suggestion in the following case I'm pretty sure:

fn main() {
    macro_rules! generate {
        () => {{
            "txt";
        }}
    }

    let s = generate!();
    println!("{}", s);
}

Even though the suggestion would be perfectly fine and correct as s seen on nightly/master:

error[E0277]: `()` doesn't implement `std::fmt::Display`
 --> src/main.rs:9:20
  |
4 |             "txt";
  |                  - help: remove this semicolon

[fmease]

If they indeed the same span if a macro is involved,

Yeah, it looks like you're absolutely right. For fn main() { ""; } the expr.span ("") and stmt.span ("";) are obv. different but for macro_rules! s { () = { "" } } fn main() { s!(); }, stmt.span (s!();) and expr.span (s!()) are the same! Unbelievable.

I wonder if that's intentional (simplicity of implementation) or a bug we can easily fix.

[fmease]

I wonder if that's intentional (simplicity of implementation) or a bug we can easily fix.

IMO, this is the real fix. Remember, at a high-level we're simply trying to compute the "span of the semicolon" by diffing two spans but that sadly doesn't work correctly because the stmt&expr span are the same if the expr is a macro call, as a result the span is empty (as you've already come to realize) and the suggestion is also empty (since it's a removal).

So, the debug assert from the linked issue uncovered a real bug which is its purpose.

[Kivooeo]

I wonder if we can just do this instead (or something similar for empty span) #147255 (comment)

Maybe even under some FIXME comment with link to this

Because as you pointed correctly, current approach does suppress a semicolon suggestion in macro which is incorrect

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[fmease]

Thanks! r=me after fmt fix (CI should fail soon believe) & minor nit and once CI is green
@bors rollup

[Kivooeo]

0_0,

I ran ./x test ui before pushing

[fmease]

0_0,

I ran ./x test ui before pushing

You might not have rust.debug = true set in your local bootstrap.toml

[Kivooeo]

The spans are identical, so that's always true

Maybe just !stmt.span.eq_ctxt(tail_expr.span) then? (added negation)

[fmease]

The spans are identical, so that's always true

Maybe just !stmt.span.eq_ctxt(tail_expr.span) then? (added negation)

eq_ctxt means "do these spans come from different macro expansions" (simplified), so !eq_ctxt would mean we'd suppress the note for as simple cases as "text"; where both "text" and "text"; obviously come from the same "expansion" (well, syntax context), namely ROOT.

[fmease]

Sorry for forcing you to jump through some hoops, I was kinda investigating everything in real time and my bad suggestion slipped through.

r=me once green once again :)

Edit: Two final nits ^^' (#147416 (comment), #147416 (comment))

[Kivooeo]

Yeah I already tried that without success, also tried check if span is empty before suggeestion also with no luck, so I just reverted and unified it into one if to put FIXME on it

[Kivooeo]

I was kinda investigating everything

That's totally fine, I was also interested to find better solution instead of suppressing it

[fmease]

When is block.span.from_expansion() actually necessary? The following doesn't crash under debug-asserts:

fn main() {
    macro_rules! generate {
        () => {{ "text"; }}
    }
    let x_str = generate!();

    println!("{}", x_str);
}

but I think due to the block.span.from_expansion() it's now suppressed.

stmt.span.from_expansion() or equivalently tail_expr.span.from_expansion() should suffice.

[Kivooeo]

Well I keep only stmt.span.from_expansion(), but it still suppressing this suggestion here

I don't remember originally why I added this check, was it just sanity check or on purpose

[fmease]

Suggested change

	//! Regression test <https://github.com/rust-lang/rust/issues/147255>
	//! Regression test for <https://github.com/rust-lang/rust/issues/147255>

[fmease]

@bors r+

[bors]

📌 Commit 62ccf14 has been approved by fmease

It is now in the queue for this repository.