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.

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

typeck: remove leaky nested probe during trait object method resolution #57835

Merged
merged 2 commits into from
Jan 23, 2019
Merged

typeck: remove leaky nested probe during trait object method resolution #57835

merged 2 commits into from
Jan 23, 2019

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jan 22, 2019 •
edited
Loading

addresses #57673 (but not marking with f-x because thats now afflicting beta channel).

Fix #57216

@pnkfelix
Do not initiate nested probe within assemble_probe.
2dea8ec 
In particular, the table entries (associated with type-variables
created during the probe) must persist as long as the candidates
assembled during the probe. If you make a nested probe without
creating a nested `ProbeContext`, the table entries are popped at the
end of the nested probe, while the type-variables would leak out via
the assembled candidates attached to `self` (the outer
`ProbeContext`). This causes an ICE (*if you are lucky*)!
@pnkfelix
unit test for issue 57673.
33c2ceb
@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2019
@pnkfelix
Copy link
Member Author

cc @arielb1

r? @nikomatsakis

@pnkfelix
Copy link
Member Author

while working on debugging this problem, I mused that it might be good for ena to provide an alternative table implementation that did not reuse entries so eagerly, but instead would just mark them as deleted (or "popped") while just letting the table continue to grow. Using this during development might allow us to catch bugs like this more eagerly.

@pnkfelix pnkfelix mentioned this pull request Jan 22, 2019
Closed
@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 22, 2019
@pnkfelix
Copy link
Member Author

beta nominated because #57673 reproduces on beta channel now.

@pnkfelix pnkfelix changed the title Remove leaky nested probe typeck: remove leaky nested probe during trait object method resolution Jan 22, 2019
@arielb1
Copy link
Contributor

arielb1 commented Jan 22, 2019 •
edited
Loading

This doesn't quite feel like the right solution to me, because it forces the TyDynamic deref. If the deref was conditional (and I need to find a good example), it will pollute the other candidates.

@arielb1
Copy link
Contributor

arielb1 commented Jan 22, 2019

I need to think this a bit more through.

@arielb1
Copy link
Contributor

arielb1 commented Jan 22, 2019

So I think this is OK for now, and I'll come up with a better fix soon-ish

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Jan 22, 2019

📌 Commit 33c2ceb has been approved by arielb1

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 22, 2019
@bors
Copy link
Contributor

bors commented Jan 22, 2019

⌛ Testing commit 33c2ceb with merge 6bba352...

bors added a commit that referenced this pull request Jan 22, 2019
@bors
Auto merge of #57835 - pnkfelix:issue-57673-remove-leaky-nested-probe…
6bba352 
…, r=arielb1

typeck: remove leaky nested probe during trait object method resolution

addresses #57673  (but not marking with f-x because thats now afflicting beta channel).

Fix #57216
@bors
Copy link
Contributor

bors commented Jan 23, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: arielb1
Pushing 6bba352 to master...

@bors bors merged commit 33c2ceb into rust-lang:master Jan 23, 2019
@pnkfelix pnkfelix mentioned this pull request Jan 23, 2019
Closed
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 24, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Jan 24, 2019 •
edited
Loading

discussed at T-compiler meeting. We're going to postpone the decision about whether to backport to beta for (at least) a week, to both see what the impact of this change is on nightly, and to also give @arielb1 a chance to show the better fix they are proposing.

Also: @arielb1 : if you do not have time to develop the fix, it would be good in the meantime to post code demonstrating the problem(s) you foresee with the change as written.

@arielb1 arielb1 mentioned this pull request Jan 24, 2019
Merged
bors added a commit that referenced this pull request Feb 9, 2019
@bors
Auto merge of #57885 - arielb1:xform-probe, r=nikomatsakis
4c9233c 
Avoid committing to autoderef in object method probing

This fixes the "leak" introduced in #57835 (see test for details, also apparently #54252 had no tests for the "leaks" that were fixed in it, so go ahead and add one).

Maybe beta-nominating because regression, but I'm against landing things on beta we don't have to.

r? @nikomatsakis
@pnkfelix
Copy link
Member Author

visiting for triage. beta-accepting under assumption that this (PR #57835) is a pre-requisite for another beta-accepted PR, #57885

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 14, 2019
@pietroalbini pietroalbini mentioned this pull request Feb 17, 2019
Merged
@pietroalbini pietroalbini removed beta-nominated Nominated for backporting to the compiler in the beta channel. labels Feb 17, 2019
bors added a commit that referenced this pull request Feb 17, 2019
@bors
Auto merge of #58537 - pietroalbini:beta-backports, r=pietroalbini
1456495 
[beta] Rollup backports

Cherry-picked:

* #58207: Make `intern_lazy_const` actually intern its argument.
* #58161: Lower constant patterns with ascribed types.
* #57908: resolve: Fix span arithmetics in the import conflict error
* #57835: typeck: remove leaky nested probe during trait object method resolution
* #57885: Avoid committing to autoderef in object method probing
* #57646: Fixes text becoming invisible when element targetted

Rolled up:

* #58522: [BETA] Update cargo

r? @ghost
bors added a commit that referenced this pull request Feb 20, 2019
@bors
Auto merge of #58537 - pietroalbini:beta-backports, r=pietroalbini
1f84156 
[beta] Rollup backports

Cherry-picked:

* #58207: Make `intern_lazy_const` actually intern its argument.
* #58161: Lower constant patterns with ascribed types.
* #57908: resolve: Fix span arithmetics in the import conflict error
* #57835: typeck: remove leaky nested probe during trait object method resolution
* #57885: Avoid committing to autoderef in object method probing
* #57646: Fixes text becoming invisible when element targetted

Rolled up:

* #58522: [BETA] Update cargo

r? @ghost
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
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pnkfelix @rust-highfive @arielb1 @bors @nikomatsakis @oli-obk @pietroalbini