Skip to content

Do not ICE on trait aliases with missing obligations#66392

Merged
bors merged 3 commits intorust-lang:masterfrom
estebank:trait-alias-ice
Nov 20, 2019
Merged

Do not ICE on trait aliases with missing obligations#66392
bors merged 3 commits intorust-lang:masterfrom
estebank:trait-alias-ice

Conversation

@estebank
Copy link
Contributor

@estebank estebank commented Nov 14, 2019

Fix #65673.

@rust-highfive
Copy link
Contributor

rust-highfive commented Nov 14, 2019

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
Copy link
Member

@eddyb eddyb Nov 14, 2019

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

@estebank estebank Nov 14, 2019

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

@eddyb eddyb Nov 14, 2019

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

@Centril Centril Nov 14, 2019

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

@estebank estebank Nov 14, 2019

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

@alexreg alexreg Nov 15, 2019

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

estebank commented Nov 14, 2019

@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

nikomatsakis commented Nov 14, 2019

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
eddyb
eddyb reviewed Nov 14, 2019
View reviewed changes
@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
Collaborator

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
Copy link
Collaborator

bors commented Nov 19, 2019

⌛ Testing commit 0ff7353 with merge 4df6cb564e881c51abafdd610c1b4e5269ad8780...

@rust-highfive

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Nov 19, 2019

💔 Test failed - checks-azure

@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

estebank commented Nov 19, 2019

@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
Collaborator

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
Collaborator

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
Contributor

pnkfelix commented Nov 21, 2019

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
Contributor

pnkfelix commented Nov 21, 2019

stable-accepted

@pnkfelix pnkfelix added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Nov 21, 2019
@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

3 more reviewers

@alexreg alexreg alexreg left review comments

@Centril Centril Centril left review comments

@eddyb eddyb eddyb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@eddyb eddyb

Labels

A-trait-system 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