Skip to content

Rewrite dropck to be more correct - #27261

Merged
bors merged 3 commits into
rust-lang:masterfrom
arielb1:drop-sanity-check
Jul 29, 2015
Merged

Rewrite dropck to be more correct#27261
bors merged 3 commits into
rust-lang:masterfrom
arielb1:drop-sanity-check

Conversation

@arielb1

@arielb1 arielb1 commented Jul 24, 2015

Copy link
Copy Markdown
Contributor

This fixes multiple bugs, and as several of these are soundness issue, is a [breaking-change].

r? @pnkfelix

@pnkfelix

pnkfelix commented Jul 24, 2015

Copy link
Copy Markdown
Contributor

I have no problem with the refactoring overall. E.g. I have never been wedded to the use of the ty::walk method, so I do not mind the use of an explicit match with each case handled explicitly.

I am concerned about the removal of the handling of ReStatic. Maybe I overlooked something in your approach, but that side-condition was definitely motivated by concrete cases I encountered -- I would be a little surprised if this PR is actually passing the make check test suite, but maybe we never put in regression tests for the issue in question. :(

Ariel Ben-Yehuda and others added 2 commits July 24, 2015 23:46
Ariel Ben-Yehuda @arielb1
handle all type variants correctly in dropck
bfbc7bc 
This fixes a few soundness bugs in dropck, so to anyone who relied on them,
this is a
[breaking-change]

Fixes rust-lang#24086.
Fixes rust-lang#25389.
Fixes rust-lang#25598.
Fixes rust-lang#25750.
Fixes rust-lang#26641.
Fixes rust-lang#26657.
Fixes rust-lang#27240.
Fixes rust-lang#27241.
@arielb1
refactor has_dtor_of_interest
7f9953b
@arielb1

arielb1 commented Jul 24, 2015

Copy link
Copy Markdown
Contributor Author

My main opposition to the ReStatic check is that I don't think it is needed - if you have Trait+'static, then it being a dtorck type would only mean that we check Trait+'static : 'destruction_scope, which is totally harmless - in fact, in the case of Trait+'tcx, we already do the check.

With the current state of type-outlives, the check should trivially hold. However, with the new type-outlives, this would also forbid types like Trait<B<'a>>+'static, which are an important component of #26656 - which I think is a plus.

@arielb1
arielb1 force-pushed the drop-sanity-check branch from 080bafa to 7f9953b Compare July 24, 2015 21:14
@arielb1

arielb1 commented Jul 24, 2015

Copy link
Copy Markdown
Contributor Author

[rebased]
[passes make check locally]

eddyb
eddyb reviewed Jul 26, 2015
View reviewed changes
Comment thread src/librustc_typeck/check/dropck.rs Outdated

@eddyb eddyb Jul 26, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not use FnvHashSet<Ty<'tcx>>?

@arielb1 arielb1 changed the title Introduce some sanity into dropck Rewrite dropck to be more correct Jul 26, 2015
@pnkfelix pnkfelix mentioned this pull request Jul 28, 2015
Closed
@pnkfelix

pnkfelix commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

(okay, I'm now curious why the heck I added ReStatic in the first place, but I cannot argue with the logic you have presented here.)

@pnkfelix

pnkfelix commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jul 28, 2015

Copy link
Copy Markdown
Collaborator

📌 Commit e99b53e has been approved by pnkfelix

@pnkfelix

pnkfelix commented Jul 28, 2015

Copy link
Copy Markdown
Contributor

(while this is a [breaking-change], I think the problems it fixes are of a nature that we can safely just land this without concerning ourselves with e.g. warning cycles et cetera)

@pnkfelix pnkfelix added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 28, 2015
@bors

bors commented Jul 29, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit e99b53e with merge ec7f2e5...

@bors

bors commented Jul 29, 2015

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-linux-32-opt

@alexcrichton

alexcrichton commented Jul 29, 2015

Copy link
Copy Markdown
Member

@bors: retry

On Tue, Jul 28, 2015 at 10:12 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/5904

—
Reply to this email directly or view it on GitHub
#27261 (comment).

@bors

bors commented Jul 29, 2015

Copy link
Copy Markdown
Collaborator

⌛ Testing commit e99b53e with merge d576ef3...

bors added a commit that referenced this pull request Jul 29, 2015
@bors
Auto merge of #27261 - arielb1:drop-sanity-check, r=pnkfelix
d576ef3 
This fixes multiple bugs, and as several of these are soundness issue, is a [breaking-change].

r? @pnkfelix
@bors

bors commented Jul 29, 2015

Copy link
Copy Markdown
Collaborator

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

@bors
bors merged commit e99b53e into rust-lang:master Jul 29, 2015
@brson brson added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Sep 15, 2015
@brson

brson commented Sep 15, 2015

Copy link
Copy Markdown
Contributor

Marking this as stable-regression so i can find it later, even though these are just soundness fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@pnkfelix pnkfelix

Labels

regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. relnotes Marks issues that should be documented in the release notes of the next release.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@arielb1 @pnkfelix @bors @alexcrichton @brson @eddyb