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

FIXME: ObligationForest assumptions incorrect #32286

Closed
nikomatsakis opened this issue Mar 16, 2016 · 4 comments
Closed

FIXME: ObligationForest assumptions incorrect #32286

nikomatsakis opened this issue Mar 16, 2016 · 4 comments
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

There are two assumptions I made in the Obligation Forest PR that I think may have been incorrect.

One. OF assumes that if selection results in ambiguity the inference context did not change. But, in working on #31867, @soltanmm reminded me that this is not always true, due to the "unify despite ambiguity" code.

Two. The stalled_on code assumes that the type of a variable will change if any progress can be made. I found this to be true even for closures -- because closures embed the types of their upvars. But this is not always true. That is, for closures with no upvars, when we select a closure kind (fn vs fnonce etc), the type doesn't change at all. This became an issue in the projection cache. I was not able however to make a test case that causes a problem with for the OF, but it seems likely to be a problem eventually.

Ultimately I expect the right fix for problem two is to expose the Closure Kind as part of the closure type, so that indeed the fully resolved type always changes. It seems very convenient to be able to use the fully resolved type as a cache key and not fear silent background changes.

The fix for problem one is just to refactor a bit I think to expose that information better. The only effect it has is when the OF will decide it has reached a fix point and stop trying to call select.

@nikomatsakis nikomatsakis changed the title ObligationForest assumptions incorrect FIXME: ObligationForest assumptions incorrect Mar 16, 2016
bors added a commit that referenced this issue Apr 13, 2016
Replace consider_unification_despite_ambiguity with new obligation variant

Is work towards #32730. Addresses part one of #32286. Addresses #24210 and #26046 to some degree.

r? @nikomatsakis
@Mark-Simulacrum
Copy link
Member

@nikomatsakis Is this still a problem today?

@nikomatsakis
Copy link
Contributor Author

@Mark-Simulacrum so @soltanmm-google fixed the first part. The second part is.. in theory.. still an issue, but I propose we close this issue anyway, if we can't reproduce a problem. Doing so.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented May 2, 2017

Meh, just realized there are still related FIXMEs in the code. (Which I guess doesn't mean we can't close...)

@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 22, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 24, 2017
@arielb1
Copy link
Contributor

arielb1 commented Dec 24, 2017

The closure issue has been fixed in #43938.

@arielb1 arielb1 closed this as completed Dec 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants