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 logic error in #44269's prune_cache_value_obligations #45065

Merged
merged 1 commit into from
Oct 6, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Oct 6, 2017

We want to retain obligations that contain inference variables, not
obligations that don't contain them, in order to fix #43132. Because
of surrounding changes to inference, the ICE doesn't occur in its
original case, but I believe it could still be made to occur on master.

Maybe I should try to write a new test case? Certainly not right now
(I'm mainly trying to get us a beta that we can ship) but maybe before
we land this PR on nightly?

This seems to cause a 10% performance regression in my imprecise
attempt to benchmark item-body checking for #43613, but it's better to
be slow and right than fast and wrong. If we want to recover that, I
think we can change the constrained-type-parameter code to actually
give a list of projections that are important for resolving inference
variables and filter everything else out.

We want to retain obligations that *contain* inference variables, not
obligations that *don't contain* them, in order to fix rust-lang#43132. Because
of surrounding changes to inference, the ICE doesn't occur in its
original case, but I believe it could still be made to occur on master.

Maybe I should try to write a new test case? Certainly not right now
(I'm mainly trying to get us a beta that we can ship) but maybe before
we land this PR on nightly?

This seems to cause a 10% performance regression in my imprecise
attempt to benchmark item-body checking for rust-lang#43613, but it's better to
be slow and right than fast and wrong. If we want to recover that, I
think we can change the constrained-type-parameter code to actually
give a list of projections that are important for resolving inference
variables and filter everything else out.
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@arielb1
Copy link
Contributor Author

arielb1 commented Oct 6, 2017

r? @nikomatsakis

@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 6, 2017
@nikomatsakis
Copy link
Contributor

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Oct 6, 2017

📌 Commit 91fdadb has been approved by nikomatsakis

@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Oct 6, 2017
@nikomatsakis nikomatsakis mentioned this pull request Oct 6, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Oct 6, 2017

This PR seems to work locally - #43613 still doesn't use too much memory while all the tests run.

@bors
Copy link
Contributor

bors commented Oct 6, 2017

⌛ Testing commit 91fdadb with merge b67f428...

bors added a commit that referenced this pull request Oct 6, 2017
fix logic error in #44269's `prune_cache_value_obligations`

We want to retain obligations that *contain* inference variables, not
obligations that *don't contain* them, in order to fix #43132. Because
of surrounding changes to inference, the ICE doesn't occur in its
original case, but I believe it could still be made to occur on master.

Maybe I should try to write a new test case? Certainly not right now
(I'm mainly trying to get us a beta that we can ship) but maybe before
we land this PR on nightly?

This seems to cause a 10% performance regression in my imprecise
attempt to benchmark item-body checking for #43613, but it's better to
be slow and right than fast and wrong. If we want to recover that, I
think we can change the constrained-type-parameter code to actually
give a list of projections that are important for resolving inference
variables and filter everything else out.
@bors
Copy link
Contributor

bors commented Oct 6, 2017

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

@bors bors merged commit 91fdadb into rust-lang:master Oct 6, 2017
bors added a commit that referenced this pull request Oct 7, 2017
Beta 20170928

Backports of:

- Allow unused extern crate again #44825
- macros: fix bug in collecting trait and impl items with derives. #44757
-  `--cap-lints allow` switches off `can_emit_warnings` #44627
-  Update the libc submodule #44116
- limit and clear cache obligations opportunistically #44269
- clear out projection subobligations after they are processed #43999
-  fix logic error in #44269's `prune_cache_value_obligations` #45065
- REVERTS #43543: Cleanup some remains of `hr_lifetime_in_assoc_type` compatibility lint:
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: failed to get layout
6 participants