Fix various aspects around `let` bindings inside const functions #56160

oli-obk · 2018-11-22T13:44:28Z

forbid let bindings in const contexts that use short circuiting operators
harden analysis code against derefs of mutable references

Initially this PR was about stabilizing let bindings, but too many flaws were exposed that need some more testing on nightly

Centril

Additionally, here's a comprehensive run-pass test that would be nice to add: https://gist.github.com/Centril/a8df54dc8ee0b6bf0da46b5fba446acd

src/test/ui/consts/const_let_assign2.rs

src/test/ui/consts/const_let_assign.rs

src/test/ui/feature-gates/feature-gate-const_let.rs

src/libcore/lib.rs

src/test/ui/consts/const_let_eq.rs

eddyb · 2018-11-23T17:30:04Z

@oli-obk Can we poison the MIR (i.e. add a flag that we set to true) if && or || are found and decide whether to allow const_let based on that?

Centril · 2018-11-23T17:31:05Z

@oli-obk I remembered one thing now... floating points and assignment ops... we should make sure that they are not stabilized by including a test with all of the assignment ops for f32 & f64. :)

Centril · 2018-11-23T17:33:05Z

Having reviewed the main tests, I pass on the torch to...
r? @eddyb

also cc @RalfJung and @nikomatsakis

src/librustc_mir/transform/qualify_consts.rs

eddyb · 2018-11-23T18:10:41Z

@oli-obk Can you add tests for match with a single arm? I expect it to work.

oli-obk · 2018-11-23T18:31:42Z

Can we poison the MIR (i.e. add a flag that we set to true) if && or || are found and decide whether to allow const_let based on that?

I had the exact same idea like an hour ago. I guess that means it wasn't a stupid idea

src/librustc/mir/mod.rs

src/libsyntax/feature_gate.rs

src/test/ui/consts/issue-54224.rs

RalfJung · 2018-11-24T15:28:35Z

Initial implementation was merged in May: #49172

Based on #56070 (which relaxes some restrictions around assignments, before that PR we only allowed x = 42; not x.y = 42;

These two seem to contradiction each other. Isn't this stabilizing behavior that has not even landed yet?

oli-obk · 2018-11-26T08:54:54Z

I'm going to reintroduce the feature gate and let the final version bake a little more in nightly. I don't feel comfortable changing so many things and stabilizing at the same time.

oli-obk · 2018-11-26T09:25:51Z

Single match arms lead to constant contains unimplemented expression type currently. (which happens when you call the not_const method, which happens a lot in const qualif)

eddyb · 2018-11-26T09:46:59Z

@oli-obk Maybe because of the Discriminant "fake read"?

RalfJung · 2018-11-26T11:02:13Z

Nit all single-arm matches should work, IMO. Just things like match foo { x => ... }, where there is no pattern. Those should not have a Discriminant "fake read", there might not even be an enum involved.

nikomatsakis · 2018-12-11T20:59:02Z

src/librustc_mir/transform/qualify_consts.rs

@@ -320,6 +320,10 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
        }
    }

+    fn const_let_allowed(&self) -> bool {


Seems like a great place for a comment =) in particular, documenting the logic behind these rules.

Something like:

Currently, without a feature gate, let is only allowed in const fn. With a feature gate, it can appear anywhere.

We decided on not doing this dual-scheme. Instead we're poisoning constants that use short circuiting operators so they can't also have let bindings.

I'll remove the function and move back to checking the feature gate directly

nikomatsakis · 2018-12-11T21:07:34Z

src/librustc_mir/hair/cx/expr.rs

+                        cx.control_flow_destroyed.push((
+                            op.span,
+                            "`&&` operator".into(),
+                        ));
                        ExprKind::Binary {


Woah. I don't think I realized we do this sort of conversion.

nikomatsakis · 2018-12-11T21:11:06Z

src/librustc_mir/transform/qualify_consts.rs

+                            if context.is_mutating_use() {
+                                this.not_const()
+                            } else {
+                                this.qualif = Qualif::NOT_CONST;


Is this intentionally overriding all other Qualif flags? (vs invoking add)? Why is that? Maybe a comment?

That is an accidental leftover from the larger refactorings. I undid the change and added comments

nikomatsakis · 2018-12-11T21:12:41Z

First round of review -- I'd like to take one more look before r+ and in particular to know the answer to this question

nikomatsakis · 2018-12-17T21:47:56Z

src/test/ui/consts/const_short_circuit.stderr

@@ -0,0 +1,26 @@
+error: new features like let bindings are not permitted in constant which also use short circuiting operators


this reads oddly, it should be "in constants" -- i.e., plural

nikomatsakis · 2018-12-17T21:48:26Z

src/test/ui/consts/const_short_circuit.stderr

+error: new features like let bindings are not permitted in constant which also use short circuiting operators
+  --> $DIR/const_short_circuit.rs:6:9
+   |
+LL |     let mut x = true && false;


also, I presume we intend this to be temporary? this restriction is very odd and surprising from an end-user point-of-view, I wonder if we can direct people to some kind of roadmap / URL that explains what the heck is going on?

nikomatsakis · 2018-12-17T21:48:56Z

r=me modulo nits above

oli-obk · 2018-12-18T08:43:12Z

@bors r=nikomatsakis

bors · 2018-12-18T08:43:13Z

📌 Commit d815e2b has been approved by nikomatsakis

bors · 2018-12-18T11:46:00Z

⌛ Testing commit d815e2b with merge 36b0f367a017ba709a4539a6873344e7ee62e756...

bors · 2018-12-18T14:21:18Z

⌛ Testing commit d815e2b with merge cb84844...

Fix various aspects around `let` bindings inside const functions * forbid `let` bindings in const contexts that use short circuiting operators * harden analysis code against derefs of mutable references Initially this PR was about stabilizing `let` bindings, but too many flaws were exposed that need some more testing on nightly

bors · 2018-12-18T16:51:07Z

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

rust-highfive assigned Centril Nov 22, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2018

This comment has been minimized.

Sign in to view

Centril reviewed Nov 22, 2018

View reviewed changes

Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 22, 2018

oli-obk commented Nov 23, 2018

View reviewed changes

src/libcore/lib.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Centril reviewed Nov 23, 2018

View reviewed changes

src/test/ui/consts/const_let_eq.rs Outdated Show resolved Hide resolved

rust-highfive assigned eddyb and unassigned Centril Nov 23, 2018

eddyb suggested changes Nov 23, 2018

View reviewed changes

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

eddyb suggested changes Nov 24, 2018

View reviewed changes

src/librustc/mir/mod.rs Outdated Show resolved Hide resolved

Centril reviewed Nov 24, 2018

View reviewed changes

src/libsyntax/feature_gate.rs Outdated Show resolved Hide resolved

src/test/ui/consts/issue-54224.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the const_fn_let branch from e518293 to e07242f Compare November 26, 2018 12:12

This comment has been minimized.

Sign in to view

oli-obk force-pushed the const_fn_let branch from 9b8ecfb to afe9e6e Compare November 26, 2018 15:05

This comment has been minimized.

Sign in to view

oli-obk changed the title ~~Stabilize let bindings inside const functions~~ Fix various aspects around let bindings inside const functions Nov 27, 2018

nikomatsakis reviewed Dec 11, 2018

View reviewed changes

oli-obk added 3 commits December 12, 2018 09:34

Undo a change that got lost in the larger refactorings

07345f0

Manually inline trivial function

2cb5e3d

Correct documentation about FakeRead

6ca2ad5

nikomatsakis approved these changes Dec 17, 2018

View reviewed changes

oli-obk added 2 commits December 18, 2018 09:16

Properly worded diagnostic message

b678238

Explain that lack of short circuiting support in constants is temporary

d815e2b

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 Dec 18, 2018

This comment has been minimized.

Sign in to view

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 Dec 18, 2018

This comment has been minimized.

Sign in to view

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 Dec 18, 2018

bors merged commit d815e2b into rust-lang:master Dec 18, 2018

bors mentioned this pull request Dec 18, 2018

NLL: User type annotations refactor, associated constant patterns and ref bindings. #55937

Merged

cramertj mentioned this pull request Dec 28, 2018

Stabilize let bindings and destructuring in constants and const fn #57175

Merged

oli-obk deleted the const_fn_let branch June 15, 2020 15:30

		@@ -0,0 +1,26 @@
		error: new features like let bindings are not permitted in constant which also use short circuiting operators

Fix various aspects around let bindings inside const functions #56160

Fix various aspects around let bindings inside const functions #56160

Conversation

oli-obk commented Nov 22, 2018 • edited Loading

This comment has been minimized.

This comment has been minimized.

Centril left a comment

Choose a reason for hiding this comment

This comment has been minimized.

eddyb commented Nov 23, 2018 • edited Loading

Centril commented Nov 23, 2018

Centril commented Nov 23, 2018

eddyb commented Nov 23, 2018

oli-obk commented Nov 23, 2018

RalfJung commented Nov 24, 2018

oli-obk commented Nov 26, 2018

oli-obk commented Nov 26, 2018

eddyb commented Nov 26, 2018

RalfJung commented Nov 26, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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

nikomatsakis commented Dec 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Dec 17, 2018

oli-obk commented Dec 18, 2018

bors commented Dec 18, 2018

bors commented Dec 18, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors commented Dec 18, 2018

bors commented Dec 18, 2018

Fix various aspects around `let` bindings inside const functions #56160

Fix various aspects around `let` bindings inside const functions #56160

oli-obk commented Nov 22, 2018 •

edited

Loading

eddyb commented Nov 23, 2018 •

edited

Loading