Forward-compatibly deny drops in constants if they *could* actually run. by eddyb · Pull Request #43932 · rust-lang/rust

eddyb · 2017-08-17T10:11:48Z

This is part of #40036, specifically the checks for user-defined destructor invocations on locals which may not have been moved away, the motivating example being:

constFOO:i32 = (HasDrop{...},0).1;

The evaluation of constant MIR will continue to create 'static slots for more locals than is necessary (if Storage{Live,Dead} statements are ignored), but it shouldn't be misusable.

r? @nikomatsakis

arielb1 · 2017-08-17T10:35:23Z

+
+ // Deny *any* live drops anywhere other than functions.
+ if self.mode != Mode::Fn {
+ // HACK(eddyb) Emulate a bit of dataflow analysis,


This is not really "drop elaboration" dataflow - drop elaboration does not know that None has no destructor - but rather "const qual" dataflow.

Right, it's a combination of the two.

eddyb · 2017-08-17T11:38:42Z

@bors try (for cargobomb)

bors · 2017-08-17T11:38:52Z

⌛ Trying commit cdfe8d7 with merge f6c35c6...

@nikomatsakis

Forward-compatibly deny drops in constants if they *could* actually run. This is part of #40036, specifically the checks for user-defined destructor invocations on locals which *may not* have been moved away, the motivating example being: ```rust const FOO: i32 = (HasDrop {...}, 0).1; ``` The evaluation of constant MIR will continue to create `'static` slots for more locals than is necessary (if `Storage{Live,Dead}` statements are ignored), but it shouldn't be misusable. r? @nikomatsakis

bors · 2017-08-17T13:05:46Z

☀️ Test successful - status-travis
State: approved= try=True

eddyb · 2017-08-17T13:08:35Z

cc @rust-lang/infra Cargobomb run requested.

aidanhs · 2017-08-18T18:10:26Z

Cargobomb run started

nikomatsakis · 2017-08-21T21:30:26Z

Any news from cargobomb run?

aidanhs · 2017-08-22T12:53:41Z

~27 hours remaining. I was away for the weekend so didn't manage to kick off a phase of the run until yesterday.

aidanhs · 2017-08-23T23:58:11Z

At long last - http://cargobomb-reports.s3.amazonaws.com/pr-43932/index.html

eddyb · 2017-08-24T07:27:38Z

Great, only regression is Rocket (which I've seen on crater before). However, I've changed by mind about it - it only uses associated const in inherentimpls and we should probably treat those the same as const items - since there is no trait-like type dispatch being involved.

alexcrichton · 2017-08-24T14:25:42Z

Switching to S-waiting-on-review as looks like cargobomb's been run, cc @nikomatsakis

eddyb · 2017-08-24T17:16:24Z

Also waiting on me to treat inherent associated consts the same as const items.

eddyb · 2017-08-27T15:02:19Z

Latest crater report confirms Rocket has been fixed. r? @nikomatsakis

arielb1 · 2017-08-27T15:13:14Z

@eddyb

Could you add a test for the rocket case, so we can make sure it stays fixed?

nikomatsakis · 2017-08-28T15:48:19Z

+ if self.mode != Mode::Fn {
+ // HACK(eddyb) Emulate a bit of dataflow analysis,
+ // conservatively, that drop elaboration will do.
+ let needs_drop = if let Lvalue::Local(local) = *lvalue {


I presume that we will reject (for constants) things that mutate "MIR locals" once they are assigned?

Yeah that is considered an assignment and disallowed as "statement-like".

nikomatsakis · 2017-08-28T15:50:39Z

+ fn drop(&mut self) {}
+}
+
+static FOO: Option<&'static WithDtor> = Some(&WithDtor);


maybe add a test with struct WithoutDtor and Some(&WithoutDtor)? Does that behave differently?

That has worked since 1.0, but if you have something more advanced in mind that would actually stress the new code, I'm up for it.

nikomatsakis

Seems reasonable. I left a few questions.

nikomatsakis · 2017-08-28T15:51:30Z

(To be clear, I want to revisit shortly once I see answers.)

nikomatsakis · 2017-08-28T20:11:46Z

@eddyb would you mind adding a few more tests, just to have more "exhaustive" coverage of the various cases? otherwise, r=me I think.

nikomatsakis · 2017-08-30T16:24:36Z

@bors r+ -- after some discussion with @eddyb on IRC, decided tests are "good enough".

bors · 2017-08-30T16:24:37Z

📌 Commit c76a024 has been approved by nikomatsakis

bors · 2017-08-30T20:12:31Z

⌛ Testing commit c76a024 with merge 7eeac1b...

@nikomatsakis

Forward-compatibly deny drops in constants if they *could* actually run. This is part of #40036, specifically the checks for user-defined destructor invocations on locals which *may not* have been moved away, the motivating example being: ```rust const FOO: i32 = (HasDrop {...}, 0).1; ``` The evaluation of constant MIR will continue to create `'static` slots for more locals than is necessary (if `Storage{Live,Dead}` statements are ignored), but it shouldn't be misusable. r? @nikomatsakis

bors · 2017-08-30T22:59:27Z

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

@nikomatsakis

Better StorageLive / StorageDead placement for constants. Fixes problems in miri (see rust-lang/miri#324 (comment)) caused by the new scope rules in #43932. What I've tried to do here is always have a `StorageLive` but no `StorageDead` for `'static` slots. It might not work perfectly in all cases, but it should unblock miri. r? @nikomatsakis cc @oli-obk

eddyb added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Aug 17, 2017

rust-highfive assigned nikomatsakis Aug 17, 2017

arielb1 reviewed Aug 17, 2017
View reviewed changes

alexcrichton added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Aug 24, 2017

eddyb added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 24, 2017

eddyb force-pushed the const-scoping branch 5 times, most recently from 52667d8 to 8697b80Compare August 27, 2017 12:11

eddyb removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 27, 2017

eddyb force-pushed the const-scoping branch from 8697b80 to f5a22eaCompare August 28, 2017 05:12

eddyb added 3 commits August 28, 2017 09:27

rustc: use rvalue scope semantics for constant initializers.
b8c05fe

rustc_mir: conservatively deny non-noop drops in constant contexts.
9b61771

rustc: treat impl associated consts like const items for constness.
c76a024

eddyb force-pushed the const-scoping branch from f5a22ea to c76a024Compare August 28, 2017 06:28

eddyb mentioned this pull request Aug 28, 2017
permit Drop types in constants (tracking issue for RFC #1440) #33156
Closed
2 tasks

nikomatsakis reviewed Aug 28, 2017
View reviewed changes

nikomatsakis 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 Aug 30, 2017

bors merged commit c76a024 into rust-lang:masterAug 30, 2017

bors mentioned this pull request Aug 30, 2017
Use hir::ItemLocalId instead of ast::NodeId in rustc::middle::region::CodeExtent. #44171
Merged

eddyb deleted the const-scoping branch August 31, 2017 05:10

eddyb mentioned this pull request Sep 1, 2017
Better StorageLive / StorageDead placement for constants. #44252
Closed

Uh oh!

Uh oh!

Conversation

eddyb commented Aug 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1Aug 17, 2017

Choose a reason for hiding this comment

Uh oh!

eddybAug 17, 2017

Choose a reason for hiding this comment

Uh oh!

eddyb commented Aug 17, 2017

Uh oh!

bors commented Aug 17, 2017

Uh oh!

bors commented Aug 17, 2017

Uh oh!

eddyb commented Aug 17, 2017

Uh oh!

aidanhs commented Aug 18, 2017

Uh oh!

nikomatsakis commented Aug 21, 2017

Uh oh!

aidanhs commented Aug 22, 2017

Uh oh!

aidanhs commented Aug 23, 2017

Uh oh!

eddyb commented Aug 24, 2017

Uh oh!

alexcrichton commented Aug 24, 2017

Uh oh!

eddyb commented Aug 24, 2017

Uh oh!

eddyb commented Aug 27, 2017

Uh oh!

arielb1 commented Aug 27, 2017

Uh oh!

nikomatsakisAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

eddybAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakisAug 28, 2017

Choose a reason for hiding this comment

Uh oh!

eddybAug 29, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Aug 28, 2017

Uh oh!

nikomatsakis commented Aug 28, 2017

Uh oh!

nikomatsakis commented Aug 30, 2017

Uh oh!

bors commented Aug 30, 2017

Uh oh!

bors commented Aug 30, 2017

Uh oh!

bors commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eddyb commented Aug 17, 2017•
edited
Loading