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

Do not ICE on trait aliases with missing obligations #66392

Merged
merged 3 commits into from
Nov 20, 2019
Merged

Do not ICE on trait aliases with missing obligations #66392

merged 3 commits into from
Nov 20, 2019

Conversation

estebank
Copy link
Contributor

Fix #65673.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(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 Nov 14, 2019
@estebank estebank added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 14, 2019
eddyb
eddyb reviewed Nov 14, 2019
View reviewed changes
src/librustc_typeck/astconv.rs Outdated
self.tcx().sess.delay_span_bug(DUMMY_SP, &format!(
"trait_ref_to_existential called on {:?} with non-dummy Self",
trait_ref,
));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the correct fix - the caller shouldn't pass in a bad TraitRef in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eddyb Isn't this a reasonable as a way to paper over the issue while the underlying cause is being researched?

Copy link
Member

Choose a reason for hiding this comment

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

Oh I thought this code was in librustc.

Looking closer at the code, I don't think trait_ref_to_existential should exist as a method.
It should be a closure defined just before its two callsites, and it could refer to the dummy_self variable in scope directly.

Also, this definitely needs a FIXME. The overall bug appears to be a missing filter on top of expand_trait_aliases, which picks up non-supertraits where clauses - but also, the object safety completely ignores trait aliases, which could be object safety hazards.

So trait aliases aren't fully implemented yet, I'm guessing?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @alexreg

If you want to paper over something, please file a new issue for that FIXME and label it with F-trait_alias so we won't miss it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #66420 and added FIXME to the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this seems like a reasonable temporary fix, but ideally we want something proper in the medium term.

@Centril Centril removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Nov 14, 2019
@Centril
Copy link
Contributor

Centril commented Nov 14, 2019

The feature requires nightly so I don't know why it was beta/stable-nominated.

@estebank
Copy link
Contributor Author

@Centril the feature requires nightly, but you can trigger the ice even if the feature is disabled.

@Centril
Copy link
Contributor

Centril commented Nov 14, 2019

Ah! I see; my bad :)

@Centril Centril added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Nov 14, 2019
@nikomatsakis
Copy link
Contributor

The fix seems reasonable as a temporary crutch. I'm not sure we need to backport it, but it's also not harmful to do so.

@estebank estebank mentioned this pull request Nov 14, 2019
Open
@estebank estebank force-pushed the trait-alias-ice branch 2 times, most recently from 3f25273 to b3aa427 Compare November 17, 2019 22:58
@Centril Centril added the F-trait_alias `#![feature(trait_alias)]` label Nov 17, 2019
@rust-highfive

This comment has been minimized.

Sign in to view
@eddyb
Copy link
Member

eddyb commented Nov 19, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Nov 19, 2019

📌 Commit 0ff7353 has been approved by eddyb

@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 Nov 19, 2019
@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 19, 2019
@estebank
Copy link
Contributor Author

@bors retry

@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 Nov 19, 2019
@bors
Copy link
Contributor

bors commented Nov 20, 2019

⌛ Testing commit 0ff7353 with merge ea540b0...

bors added a commit that referenced this pull request Nov 20, 2019
@bors
Auto merge of #66392 - estebank:trait-alias-ice, r=eddyb
ea540b0 
Do not ICE on trait aliases with missing obligations

Fix #65673.
@bors
Copy link
Contributor

bors commented Nov 20, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing ea540b0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 20, 2019
@bors bors merged commit 0ff7353 into rust-lang:master Nov 20, 2019
@pnkfelix
Copy link
Member

beta-accepted (last week, sorry for delay!)

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 21, 2019
@pnkfelix
Copy link
Member

stable-accepted

@pnkfelix pnkfelix added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Nov 21, 2019
@alexreg alexreg added the A-traits Area: Trait system label Nov 27, 2019
alexreg added a commit to alexreg/rust that referenced this pull request Nov 29, 2019
@alexreg
Undid temporary hack introduced by rust-lang#66392.
3573acc
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Dec 5, 2019
Merged
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 5, 2019
bors added a commit that referenced this pull request Dec 7, 2019
@bors
Auto merge of #67063 - Mark-Simulacrum:beta-backports, r=Mark-Simulacrum
3f7f341 
[beta] backports

This pull request backports the following pull requests, which have all been beta-accepted by the
compiler team.

 * Handle non_exhaustive in borrow checking #66722
 * Do not ICE on trait aliases with missing obligations #66392
 * Do not ICE in `if` without `else` in `async fn` #66391
 * Fix ICE when trying to suggest `Type<>` instead of `Type()` #66390
 * Do not ICE on recovery from unmet associated type bound obligation #66388
 * find_deprecation: deprecation attr may be ill-formed meta. #66381
 * parser: don't use `unreachable!()` in `fn unexpected`. #66361
 * Undo an assert causing an ICE until we fix the underlying problem #66250
 * Do not ICE with a precision flag in formatting str and no format arguments #66093
 * Fix two OOM issues related to `ConstProp` #66394
bors added a commit that referenced this pull request Dec 8, 2019
@bors
Auto merge of #67063 - Mark-Simulacrum:beta-backports, r=Mark-Simulacrum
b509d9b 
[beta] backports

This pull request backports the following pull requests, which have all been beta-accepted by the
compiler team.

 * Handle non_exhaustive in borrow checking #66722
 * Do not ICE on trait aliases with missing obligations #66392
 * Do not ICE in `if` without `else` in `async fn` #66391
 * Fix ICE when trying to suggest `Type<>` instead of `Type()` #66390
 * Do not ICE on recovery from unmet associated type bound obligation #66388
 * find_deprecation: deprecation attr may be ill-formed meta. #66381
 * parser: don't use `unreachable!()` in `fn unexpected`. #66361
 * Undo an assert causing an ICE until we fix the underlying problem #66250
 * Do not ICE with a precision flag in formatting str and no format arguments #66093
 * Fix two OOM issues related to `ConstProp` #66394
@Mark-Simulacrum Mark-Simulacrum removed stable-accepted Accepted for backporting to the compiler in the stable channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Dec 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexreg alexreg alexreg left review comments

@Centril Centril Centril left review comments

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
A-traits Area: Trait system beta-accepted Accepted for backporting to the compiler in the beta channel. F-trait_alias `#![feature(trait_alias)]` 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICE on trait aliases without a flag
9 participants
@estebank @rust-highfive @Centril @nikomatsakis @eddyb @bors @pnkfelix @alexreg @Mark-Simulacrum