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

Don't inspect the generated existential type items #51773

Merged
merged 1 commit into from
Jun 27, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 25, 2018

r? @nikomatsakis

My debugging led me to the hir::ItemExistential(..) checks, which are entirely unnecessary because we never use the items directly. The issue was that items were iterated over in a random order (due to hashmaps), so if you checked the ItemExistential before the function that has the actual return impl Trait, you'd run into those ICEs you encountered.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 25, 2018
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This seems good, though I'm a bit torn: maybe it's good to try and be a bit more robust?

@@ -419,7 +419,7 @@ fn convert_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, item_id: ast::NodeId) {
convert_variant_ctor(tcx, struct_def.id());
}
},
hir::ItemExistential(..) |
hir::ItemExistential(..) => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always something we will want to skip...? or will we want to do something here when user-given predicates are permitted?

Actually I think in general that we've been wanting to get rid of this part of the code anyway, which maybe isn't strictly needed anymore? (Though it may be good for generating the errors in a predictable order)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will want this even before user defined existentials. It's just that right now these existential types can't really live on their own because their generics still refer to generics of their parent function. Once we get that fully separated, there won't be any more code running twice with different settings on this node

let def = self.map.defs.get(&lifetime.id).cloned();
if let Some(Region::LateBound(_, def_id, _)) = def {
let def = self.map.defs.get(&lifetime.id);
if let Some(&Region::LateBound(_, def_id, _)) = def {
Copy link
Contributor

Choose a reason for hiding this comment

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

It still seems like it might be good not to generate the late-bound entries we do, as they are pretty malformed, and can lead to ICEs and confusion down the line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll reinstate your removal code

@oli-obk oli-obk force-pushed the cleanup_impl_trait branch from 00b7c75 to a6bcb9d Compare June 26, 2018 11:28
@bors
Copy link
Contributor

bors commented Jun 26, 2018

☔ The latest upstream changes (presumably #51805) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk oli-obk force-pushed the cleanup_impl_trait branch from a6bcb9d to 28a76a9 Compare June 26, 2018 14:36
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 26, 2018

📌 Commit 28a76a9 has been approved by nikomatsakis

@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 Jun 26, 2018
@bors
Copy link
Contributor

bors commented Jun 27, 2018

⌛ Testing commit 28a76a9 with merge d6e2239...

bors added a commit that referenced this pull request Jun 27, 2018
Don't inspect the generated existential type items

r? @nikomatsakis

My debugging led me to the `hir::ItemExistential(..)` checks, which are entirely unnecessary because we never use the items directly. The issue was that items were iterated over in a random order (due to hashmaps), so if you checked the `ItemExistential` before the function that has the actual return `impl Trait`, you'd run into those ICEs you encountered.
@bors
Copy link
Contributor

bors commented Jun 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing d6e2239 to master...

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants