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

avoid translating roots with predicates that do not hold #42797

Merged
merged 2 commits into from Jun 29, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jun 21, 2017

Finally I got around to doing this.

Fixes #37725.

r? @nikomatsakis

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 21, 2017

NOTE: I want to talk about this with @nikomatsakis to confirm this is the correct strategy - anyone else should not r+ this.

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2017
@carols10cents
Copy link
Member

friendly ping @nikomatsakis, pinging you in IRC too!

@@ -304,6 +304,7 @@ fn collect_roots<'a, 'tcx>(scx: &SharedCrateContext<'a, 'tcx>,
scx.tcx().hir.krate().visit_all_item_likes(&mut visitor);
}

roots.retain(|root| root.is_instantiable(scx.tcx()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. This seems perfectly reasonable, but perhaps not optimal? (Also, why do we check below for whether something is instantiable, but not other places?)

Put another way, it might be better to have some helper routine for pushing into self.output that checks for "is instantiable" at that point -- perhaps this would also avoid enumerating the things reachable from a non-instantiable thing?

However, I see no obvious reason that this code is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everywhere except for roots and vtables, instantiability is guaranteed by the program type-checking, so we want to produce a clean ICE if the item is not instantiable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't the roots just the content of the things pushed to self.output?

@nikomatsakis
Copy link
Contributor

As discussed on IRC, r=me w/ a comment.

@arielb1
Copy link
Contributor Author

arielb1 commented Jun 28, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 28, 2017

📌 Commit a6ca302 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jun 28, 2017

⌛ Testing commit a6ca302 with merge c28cbfb...

bors added a commit that referenced this pull request Jun 28, 2017
avoid translating roots with predicates that do not hold

Finally I got around to doing this.

Fixes #37725.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jun 29, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants