Skip to content

dropck: do not add types with escaped-regions into the breadcrumbs set. #27236

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
Closed
pnkfelix wants to merge 1 commit into from
Closed

dropck: do not add types with escaped-regions into the breadcrumbs set. #27236

pnkfelix wants to merge 1 commit into from

Conversation

@pnkfelix
Copy link
Contributor

@pnkfelix pnkfelix commented Jul 23, 2015

dropck: do not add types with escaped-regions into the breadcrumbs set.

In particular, when we traverse a type like:

(for <'r> Fn(&'r u8), for <'r> Fn(&'r u8))

we should not add the first type Fn(&'r u8) to the bread-crumbs set,
because that type ends up being structurally equivalent to the
second Fn(&'r u8) type, even though they each should be considered
distinct since they occur under distinct for <'r> binders.

Fix #25750.

@pnkfelix
dropck: do not add types with escaped-regions into the breadcrumbs set.
b4038a9 
In particular, when we traverse a type like:

  `(for <'r> Fn(&'r u8), for <'r> Fn(&'r u8))`

we should not add the first type `Fn(&'r u8)` to the bread-crumbs set,
because that type ends up being structurally equivalent to the
*second* `Fn(&'r u8)` type, even though they each should be considered
distinct since they occur under distinct `for <'r>` binders.

Fix rust-lang#25750.
@rust-highfive
Copy link
Contributor

rust-highfive commented Jul 23, 2015

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Contributor Author

pnkfelix commented Jul 23, 2015

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned Aatch Jul 23, 2015
@pnkfelix pnkfelix mentioned this pull request Jul 23, 2015
Closed
@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2015

What's the issue here again (test?)? I think adding a skip_current_subtree before the continue; when we meet a breadcrumb would be a wiser choice (actually, it is required, see #27240) - if there is an issue with that, it would be nice to at least comment on it.

Anyway, I think ty_walk is a stupid choice here, given that we don't actually treat anything nominally here (except trivially for primitives, and semi-accidentally for tuples).

@pnkfelix
Copy link
Contributor Author

pnkfelix commented Jul 23, 2015

@arielb1 you're right I should have included a regression test.

@pnkfelix
Copy link
Contributor Author

pnkfelix commented Jul 23, 2015

@arielb1 also I agree that it would probably be good to add a skip_current_subtree before the continue; ... its possible that doing so would resolve #25750 as well, ... I will think about it.

@pnkfelix
Copy link
Contributor Author

pnkfelix commented Jul 28, 2015

(closing, i suspect @arielb1 's approach of #27261 is going to be a more realistic PR to land.)

@pnkfelix pnkfelix closed this Jul 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakis nikomatsakis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pnkfelix @rust-highfive @arielb1 @nikomatsakis @Aatch