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

[fmease]

(Please my earlier comments for more context)

Please remove the two .from_expansion() calls and instead do the following in order to get rid of the false negative I've mentioned in #147416 (the generate example).

- if self.predicate_must_hold_modulo_regions(&new_obligation) {
+ if self.predicate_must_hold_modulo_regions(&new_obligation)
+     // [NOTE or FIXME re. expr & stmt having the same span if expr comes from an expansion]
+     && stmt.span.eq_ctxt(tail_expr.span)
+ {
      err.span_suggestion_short(
          stmt.span.with_lo(tail_expr.span.hi()),
          "remove this semicolon",
          "",
          Applicability::MachineApplicable,
      );

[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.