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

Return early to fix ICE #94711

Merged
merged 2 commits into from
Mar 12, 2022
Merged

Return early to fix ICE #94711

merged 2 commits into from
Mar 12, 2022

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Mar 7, 2022

This fixes #94627, ICE happens because compiler tries to suggest constraining type parameter but the only constraint is implicit std::Sized one, so it gets removed and there is nothing to suggest resulting in ICE.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 7, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 7, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Mar 7, 2022

Perhaps we can fix this issue in a different way. Right above your changes is a call to suggest_removing_unsized_bound -- perhaps we can make that return a bool (signifying if adding the suggestion was successful), and bail in this function if that returns false.

@ouz-a
Copy link
Contributor Author

ouz-a commented Mar 7, 2022

Perhaps we can fix this issue in a different way. Right above your changes is a call to suggest_removing_unsized_bound -- perhaps we can make that return a bool (signifying if adding the suggestion was successful), and bail in this function if that returns false.

But there is no sized constraint left so we don't pass sized_constraints.next() check, even if I change the condition so we pass the check, suggest_removing_unsized_bound would be unrelated to what we do here.

@compiler-errors
Copy link
Member

Hm.. really? How do we get into that loop if we have no constraints? I would assume the only constraint we had in that loop that get drained in that drain_filter call.

@ouz-a
Copy link
Contributor Author

ouz-a commented Mar 7, 2022

Hm.. really? How do we get into that loop if we have no constraints? I would assume the only constraint we had in that loop that get drained in that drain_filter call.

But... drain is inside the loop, we drain the std::marker::Sized then we have nothing left this is why we get ICE in the first place

@oli-obk
Copy link
Contributor

oli-obk commented Mar 12, 2022

What do you think about not calling multipart_suggestion_verbose if the suggestion list is empty?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 12, 2022

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned nagisa Mar 12, 2022
@ouz-a
Copy link
Contributor Author

ouz-a commented Mar 12, 2022

What do you think about not calling multipart_suggestion_verbose if the suggestion list is empty?

It would not matter much. Just would improve readability 😸

if suggestions.len() == 1 {//span_suggestion_verbose}
else if sugesstions.len() > 1 {// multipart}

like this?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 12, 2022

@bors r+ rollup

Perfect! Thanks

@bors
Copy link
Contributor

bors commented Mar 12, 2022

📌 Commit 1853ffc has been approved by oli-obk

@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 12, 2022
@bors
Copy link
Contributor

bors commented Mar 12, 2022

⌛ Testing commit 1853ffc with merge 22a20e3...

@bors
Copy link
Contributor

bors commented Mar 12, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 22a20e3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2022
@bors bors merged commit 22a20e3 into rust-lang:master Mar 12, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 12, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (22a20e3): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@ouz-a ouz-a deleted the master3 branch July 26, 2023 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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: assertion failed: !suggestion.is_empty()
8 participants