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

Fix leak of skolemized types in HRTB #19438

Conversation

nikomatsakis
Copy link
Contributor

A subtle bug in the higher-ranked sub/lub/glb routines could cause incorrect results and/or leaked skolemized regions, something I've seen from time to time but never quite knew why. Finally tracked it down. Didn't open an issue but there are various tests that report wrong result and/or ICE in the PR.

r? @pnkfelix

@nikomatsakis
Copy link
Contributor Author

Hmm, running this test to completion I see that it breaks some tests. (I think that those tests should not have been working in the first place, but what they are doing is fundamentally useful.) The tests are all cases where Clone must be implemented for fn(&int); the existing impl works only for types not containing bound regions. I think the only real fix to this is to have some kind of blanket impl that applies to all fn types, perhaps a meta-trait identifying fn pointers.

@reem
Copy link
Contributor

reem commented Dec 1, 2014

I remember the blanket Clone impl being broken for fn(&int) a while ago, unless it was fixed and this is re-breaking it that seems unrelated.

@reem
Copy link
Contributor

reem commented Dec 1, 2014

Also, is this related to the lack of an impl of Fn(&_,) for fn(&_)?

have such a silly over-engineered interface.
impl for fn pointer types including bound regions, unfortunately.
issue-14039 test in a more natural way. Previously the "type we will
cast to" was hidden unless it was an integer.
* we're not careful, it will succeed.
*
* The reason is that when we walk through the subtyping
* algorith, we begin by replacing `'a` with a skolemized
Copy link
Member

Choose a reason for hiding this comment

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

nit: "algorith" => "algorithm"

@nikomatsakis
Copy link
Contributor Author

Going to do more work in this area. I'll close this branch for now and re-open with more commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants