Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix debug ICE when previous arg span during extra comma removal comes from macro context #112447

Closed
wants to merge 1 commit into from

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jun 9, 2023

Manually calculate prev.shrink_to_hi().to(span) to avoid empty span when prev comes from a macro context, such as in the code below:

#![feature(never_type)]
fn f(a: !) {}

fn main() {
    f(panic!(), 1);
}

Fixes #112437.

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2023

r? @oli-obk

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

@rustbot 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 Jun 9, 2023
@jieyouxu jieyouxu changed the title Fix debug ICE when previous arg during extra comma removal comes from macro context Fix debug ICE when previous arg span during extra comma removal comes from macro context Jun 9, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

// NOTE(#112437): we *manually* calculate the previous comma span here
// because `Span::to` returns incomplete span when `prev` or `span`
// involves macro contexts.
span = Span::new(prev.hi(), span.hi(), span.ctxt(), span.parent());
Copy link
Member

@lukas-code lukas-code Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix looks incorrect if the argument comes from a macro variable, for example for the following the span will now start after 1 and go all the way to 2:

fn foo(_: i32) {}

macro_rules! m {
    ($e:expr) => {
        foo(1, $e)
    }
}

fn main() {
    m!(2)
}

I think you can swap the arguments to get a better span: span.to(prev.shrink_to_hi()).

Copy link
Member Author

@jieyouxu jieyouxu Jun 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that isn't correct either: Span::to will also return incomplete span when the "current" argument span (span) is from a macro context...

e.g.

f(1, panic!());

I will have to think about how to correctly fix this span if either comes from macro contexts...

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 9, 2023

Unfortunately I do not know how to properly fix this issue, closing for now.

@jieyouxu jieyouxu closed this Jun 9, 2023
@jieyouxu jieyouxu deleted the issue-112437 branch October 22, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug ice if we pass superfluous args after panic to fn: no errors reported for args
5 participants