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

Rewrite dropck to be more correct #27261

Merged
merged 3 commits into from
Jul 29, 2015
Merged

Rewrite dropck to be more correct #27261

merged 3 commits into from
Jul 29, 2015

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Jul 24, 2015

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

r? @pnkfelix

@pnkfelix
Copy link
Member

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
@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
Copy link
Contributor Author

arielb1 commented Jul 24, 2015

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
Copy link
Contributor Author

arielb1 commented Jul 24, 2015

[rebased]
[passes make check locally]

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

let destructor_for_type = rcx.tcx().destructor_for_type.borrow();
// FIXME(arielb1): don't be O(n^2)
if cx.breadcrumbs.contains(&ty) {
Copy link
Member

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
Copy link
Member

(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
Copy link
Member

@bors r

@bors
Copy link
Contributor

bors commented Jul 28, 2015

📌 Commit e99b53e has been approved by pnkfelix

@pnkfelix
Copy link
Member

(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
Copy link
Contributor

bors commented Jul 29, 2015

⌛ Testing commit e99b53e with merge ec7f2e5...

@bors
Copy link
Contributor

bors commented Jul 29, 2015

💔 Test failed - auto-linux-32-opt

@alexcrichton
Copy link
Member

@bors: retry

On Tue, Jul 28, 2015 at 10:12 PM, bors [email protected] 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
Copy link
Contributor

bors commented Jul 29, 2015

⌛ 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
Copy link
Contributor

bors commented Jul 29, 2015

☀️ 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
Copy link
Contributor

brson commented Sep 15, 2015

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