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.

Sign up for GitHub

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

Jump to bottom

Fix ICE from zero-length span when suggesting to remove trailing semi-colon from final statement in block #118953

Closed
wants to merge 1 commit into from
Closed

Fix ICE from zero-length span when suggesting to remove trailing semi-colon from final statement in block #118953

wants to merge 1 commit into from

Conversation

EliseZeroTwo
Copy link
Contributor

This fixes #114251

The span for the suggestion of removing the semi-colon from the final statement in a block was constructed with zero-length, resulting in an ICE when debug-assersions were enabled.

This PR fixes that by ensuring that the span covers the semi-colon and isn't just zero-length.

It includes a regression test.

🖤

@rustbot
Copy link
Collaborator

rustbot commented Dec 14, 2023

r? @b-naber

(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 Dec 14, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@EliseZeroTwo
Copy link
Contributor Author

@rustbot label -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot 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 Dec 19, 2023
@EliseZeroTwo
Copy link
Contributor Author

@rustbot review

@rustbot 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 Dec 24, 2023
@cjgillot
Copy link
Contributor

On the original issue, you wrote:

This appears to stem from macros not having their trailing semi-colon included in the span for the hir::Stmt. I am working on a fix for this still.

Should this be fixed by changing the spans that appears in the hir::Stmt?

@cjgillot
Copy link
Contributor

I solved this, and also fixed the issue with macro spans not including their trailing semi-colon, but as I was able to fix this without the need for the macro span fixes I did not include them in this pull request. I will ask in the zulip if the fixes for macro spans are wanted, and if so open a separate PR for them.

Seeing this just now.
I lean to consider that HIR not containing the semicolon is the bug, and that this PR papers over it.
So I'd definitely like to see a PR that fixes HIR.

@EliseZeroTwo
Copy link
Contributor Author

EliseZeroTwo commented Dec 24, 2023 •
edited
Loading

I solved this, and also fixed the issue with macro spans not including their trailing semi-colon, but as I was able to fix this without the need for the macro span fixes I did not include them in this pull request. I will ask in the zulip if the fixes for macro spans are wanted, and if so open a separate PR for them.

Seeing this just now. I lean to consider that HIR not containing the semicolon is the bug, and that this PR papers over it. So I'd definitely like to see a PR that fixes HIR.

Ok, I wasn't sure if it was out of scope for this PR and/or if it intentional for some reason, and as I could solve this by reusing a pattern already used in other places for diagnostics in HIR I thought reusing that pattern instead of making major changes was a safer idea. Would you prefer if I submitted the fixes for this inside this PR or opened a new one?

@apiraino
Copy link
Contributor

ping @cjgillot for a pending question in this comment. Thanks.

@cjgillot
Copy link
Contributor

cjgillot commented Mar 3, 2024

Would you prefer if I submitted the fixes for this inside this PR or opened a new one?

This PR if the fix is very close to the current code.
Another PR if the fix is in a completely different place with a different logic.

@apiraino
Copy link
Contributor

apiraino commented Mar 5, 2024

I guess the above comment should qualify this PR back to the author

@rustbot author

@rustbot rustbot 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 Mar 5, 2024
@oskgo
Copy link
Contributor

oskgo commented Jul 26, 2024

@EliseZeroTwo

Ping from triage: I'm closing this due to inactivity, Please reopen when you are ready to continue with this.
Note: if you are going to continue please open the PR BEFORE you push to it, else you won't be able to reopen - this is a quirk of github.
Thanks for your contribution.

@rustbot label: +S-inactive

@oskgo oskgo closed this Jul 26, 2024
@rustbot rustbot added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjgillot cjgillot cjgillot left review comments

Assignees

@b-naber b-naber

Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICE: Span must not be empty and have no suggestion (E0308)
7 participants
@EliseZeroTwo @rustbot @rust-log-analyzer @cjgillot @apiraino @oskgo @b-naber