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 ICE: check if snippet is ) #108298

Merged
merged 1 commit into from
Mar 4, 2023
Merged

Fix ICE: check if snippet is ) #108298

merged 1 commit into from
Mar 4, 2023

Conversation

TaKO8Ki
Copy link
Member

@TaKO8Ki TaKO8Ki commented Feb 21, 2023

Fixes #107705

@rustbot
Copy link
Collaborator

rustbot commented Feb 21, 2023

r? @jackh726

(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 Feb 21, 2023
@TaKO8Ki TaKO8Ki closed this Feb 21, 2023
@TaKO8Ki TaKO8Ki reopened this Feb 21, 2023
@TaKO8Ki TaKO8Ki changed the title Check if snippet is ) Fix ICE: check if snippet is ) Feb 21, 2023
@cjgillot
Copy link
Contributor

Is the parser logic incorrect, or is the suggestion logic incorrect? In the latter, we should just check that the span is adequate before emitting the suggestion, shouldn't we?

More generally, I wonder if the diagnostic infra should just drop suggestions that satisfy this ICE.

@TaKO8Ki
Copy link
Member Author

TaKO8Ki commented Feb 27, 2023

@cjgillot I think the parser logic causes this problem. In the following case, snapshot.token.span in 1194 line is an empty span after a comma. So, to check if the token is close paren, we need to confirm whether the snippet of the span is ) or not.

we should just check that the span is adequate before emitting the suggestion, shouldn't we?

However, we can check that just before emitting the suggestion.

fn f() {a(b:&,

@jackh726
Copy link
Member

r? @cjgillot

@rustbot rustbot assigned cjgillot and unassigned jackh726 Feb 28, 2023
@cjgillot
Copy link
Contributor

However, we can check that just before emitting the suggestion.

Yes please. I consider each use of span_to_snippet to be suspicious, as it tries to re-parse code, with a lot of limitations.

In that code path, which one is the "empty" span that the ICE designates?

@cjgillot
Copy link
Contributor

cjgillot commented Mar 4, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Mar 4, 2023

📌 Commit 871ee18 has been approved by cjgillot

It is now in the queue for this repository.

@bors 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 Mar 4, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#108298 (Fix ICE: check if snippet is `)`)
 - rust-lang#108405 (Lazily compute crate name for consider_optimizing)
 - rust-lang#108656 (Rustdoc search: Emit an error for unclosed generic)
 - rust-lang#108660 (Remove ne implementations from strings)
 - rust-lang#108669 (Allow checking whether a type allows being uninitialized)
 - rust-lang#108727 (rustc_expand: make proc-macro derive error translatable)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 035aa28 into rust-lang:master Mar 4, 2023
@rustbot rustbot added this to the 1.69.0 milestone Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

ICE: Span must not be empty and have no suggestion
5 participants