Const qualification comments #61492

RalfJung · 2019-06-03T16:08:46Z

I extracted some const-qualif knowledge from @eddyb. This is my attempt to turn that into comments.

Cc @oli-obk @eddyb

rust-highfive · 2019-06-03T16:08:53Z

r? @zackmdavis

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

zackmdavis · 2019-06-03T16:13:21Z

r? @eddyb

eddyb · 2019-06-03T16:15:41Z

src/librustc_mir/transform/qualify_consts.rs

-// Constant containing interior mutability (UnsafeCell).
+/// Constant containing interior mutability (UnsafeCell).
+/// This must be ruled out to make sure that evaluating the constant at compile-time
+/// and run-time would produce the same result.


Nope, that's irrelevant. The problem is promoting e.g. &AtomicUsize and then mutating it.
That is, uses of the promoted references must not be able to distinguish between the promoted and unpromoted case (ignoring pointer addresses).

Well that's what I meant.^^

And also the part where any use of a const item must be equivalent to inlining its definition.

I have expanded the description a bit, is this better?

src/librustc_mir/transform/qualify_consts.rs

eddyb · 2019-06-03T16:58:23Z

src/librustc_mir/transform/qualify_consts.rs

+/// for value qualifications, and accumulates writes of
+/// rvalue/call results to locals, in `local_qualif`.
+/// For functions (constant or not), it also records
+/// candidates for promotion in `promotion_candidates`.


Not just functions though, I think for everything? Promotion still has to do something in const/static, it's just slightly different than at runtime.

No idea, I just moved that comment up, I did not write it.^^

There's a comment in the file though saying

// No need to do anything in constants and statics, as everything is "constant" anyway // so promotion would be useless. if self.mode != Mode::Static && self.mode != Mode::Const {

So it does seem to not do promotion the same way for statics and consts?

eddyb · 2019-06-03T17:14:17Z

src/librustc_mir/transform/qualify_consts.rs

@@ -757,6 +788,9 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
                        // `let _: &'static _ = &(Cell::new(1), 2).1;`
                        let mut local_qualifs = self.qualifs_in_local(local);
                        local_qualifs[HasMutInterior] = false;
+                        // Make sure there is no reason to prevent promotion.
+                        // This is, in particular, the "implicit promotion" version of
+                        // the check making sure that we don't run drop glue during const-eval.


This seems oddly specific - this is the original sole check, so comparing it to something newer seems off.
You could move the comment one line above and make it more about "any qualifications, except HasMutInterior (see above), disqualify from promotion".

I don't think which check came first historically has any bearing here. Just because it grew that way doesn't mean that's the best way to explain it.

The other check is IMO clearly "more principled" by looking at the place where the side-effect actually occurs.

I have adjusted the wording a bit.

src/librustc_mir/transform/qualify_consts.rs

Centril

Some proof-reading :)

src/librustc_mir/transform/qualify_consts.rs

bors · 2019-06-05T21:32:00Z

☔ The latest upstream changes (presumably #61548) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2019-06-09T05:12:32Z

☔ The latest upstream changes (presumably #61653) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2019-06-09T10:24:44Z

This is bitrotting. @eddyb could you have a look at my edits?

eddyb · 2019-06-10T08:29:44Z

@bors r+

bors · 2019-06-10T08:29:46Z

📌 Commit 38c7f3e has been approved by eddyb

@eddyb

…ddyb Const qualification comments I extracted some const-qualif knowledge from @eddyb. This is my attempt to turn that into comments. Cc @oli-obk @eddyb

@eddyb

…ddyb Const qualification comments I extracted some const-qualif knowledge from @eddyb. This is my attempt to turn that into comments. Cc @oli-obk @eddyb

bors · 2019-06-10T11:03:02Z

☔ The latest upstream changes (presumably #61506) made this pull request unmergeable. Please resolve the merge conflicts.

…some Mode, and more comments

RalfJung · 2019-06-10T11:52:09Z

Rebased.

@bors r=eddyb

bors · 2019-06-10T11:52:11Z

📌 Commit 0edf46f has been approved by eddyb

bors · 2019-06-10T14:08:29Z

⌛ Testing commit 0edf46f with merge 0efa0a6a57bfe2ed3e05e02850f3c18d8747b767...

bors · 2019-06-10T18:08:29Z

💥 Test timed out

RalfJung · 2019-06-10T18:24:37Z

@bors retry

bors · 2019-06-11T05:24:47Z

⌛ Testing commit 0edf46f with merge 9d9c7ad...

@eddyb

Const qualification comments I extracted some const-qualif knowledge from @eddyb. This is my attempt to turn that into comments. Cc @oli-obk @eddyb

bors · 2019-06-11T08:15:04Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: eddyb
Pushing 9d9c7ad to master...

rust-highfive assigned zackmdavis Jun 3, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2019

rust-highfive assigned eddyb and unassigned zackmdavis Jun 3, 2019

eddyb reviewed Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

eddyb reviewed Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

eddyb reviewed Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

oli-obk requested changes Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

eddyb reviewed Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

RalfJung commented Jun 3, 2019

View reviewed changes

src/librustc_mir/transform/qualify_consts.rs Outdated Show resolved Hide resolved

Centril reviewed Jun 3, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the const-qualif-comments branch from 6bb0760 to dfa40d3 Compare June 6, 2019 10:00

RalfJung force-pushed the const-qualif-comments branch from 96436e3 to 38c7f3e Compare June 9, 2019 10:24

eddyb approved these changes Jun 10, 2019

View reviewed changes

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 Jun 10, 2019

Centril mentioned this pull request Jun 10, 2019

Rollup of 6 pull requests #61713

Closed

Centril mentioned this pull request Jun 10, 2019

Rollup of 5 pull requests #61714

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 10, 2019

RalfJung added 5 commits June 10, 2019 13:06

some more comments for const_qualif

ea21a8a

replace some mode comparisons by more readable function call, rename …

ed02b15

…some Mode, and more comments

avoid 'const-context' terminology

f2b49d9

const-correctness might be confusing for C++ people

b823cc1

comments

0edf46f

RalfJung force-pushed the const-qualif-comments branch from 38c7f3e to 0edf46f Compare June 10, 2019 11:51

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2019

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

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 Jun 10, 2019

bors added a commit that referenced this pull request Jun 11, 2019

Auto merge of #61492 - RalfJung:const-qualif-comments, r=eddyb

9d9c7ad

Const qualification comments I extracted some const-qualif knowledge from @eddyb. This is my attempt to turn that into comments. Cc @oli-obk @eddyb

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 11, 2019

bors merged commit 0edf46f into rust-lang:master Jun 11, 2019

RalfJung deleted the const-qualif-comments branch June 21, 2019 07:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const qualification comments #61492

Const qualification comments #61492

RalfJung commented Jun 3, 2019

rust-highfive commented Jun 3, 2019

zackmdavis commented Jun 3, 2019

eddyb Jun 3, 2019

RalfJung Jun 3, 2019

RalfJung Jun 9, 2019

eddyb Jun 3, 2019

RalfJung Jun 3, 2019

RalfJung Jun 6, 2019

eddyb Jun 3, 2019

RalfJung Jun 3, 2019 •

edited

Loading

RalfJung Jun 9, 2019

Centril left a comment

This comment has been minimized.

bors commented Jun 5, 2019

bors commented Jun 9, 2019

RalfJung commented Jun 9, 2019

eddyb commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

RalfJung commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

RalfJung commented Jun 10, 2019

bors commented Jun 11, 2019

bors commented Jun 11, 2019

Const qualification comments #61492

Const qualification comments #61492

Conversation

RalfJung commented Jun 3, 2019

rust-highfive commented Jun 3, 2019

zackmdavis commented Jun 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jun 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Centril left a comment

Choose a reason for hiding this comment

This comment has been minimized.

bors commented Jun 5, 2019

bors commented Jun 9, 2019

RalfJung commented Jun 9, 2019

eddyb commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

RalfJung commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

bors commented Jun 10, 2019

RalfJung commented Jun 10, 2019

bors commented Jun 11, 2019

bors commented Jun 11, 2019

RalfJung Jun 3, 2019 •

edited

Loading