Don't try to promote already promoted out temporaries#54816

oli-obk · 2018-10-04T14:24:04Z

fixes #53201

r? @eddyb

TimNN · 2018-10-16T17:56:22Z

Ping from triage @eddyb: This PR requires your review.

oli-obk · 2018-10-25T15:48:40Z

r? @alexreg

alexreg · 2018-10-25T16:08:13Z

@oli-obk I don't have r+ powers. Unless you just want me to review the code...

oli-obk · 2018-10-25T16:10:43Z

Oh I guess that's why I can't r? you :D

Yes a review would be great. You've been in that code a lot lately so I assumed you're the best one to review right now

oli-obk · 2018-10-25T16:11:09Z

@bors delegate=alexreg

let's see if that works

bors · 2018-10-25T16:11:10Z

✌️ @alexreg can now approve this pull request

alexreg · 2018-10-25T16:17:39Z

src/librustc_mir/transform/qualify_consts.rs

                        return
                    }
-                    if this.qualif.is_empty() {
+                    // if the argument requires a constant, we care about constness, not


This could do with elaboration. What do we mean by "requires a constant"? Where is this checked for?

alexreg · 2018-10-25T16:37:49Z

Generally looks fine otherwise though...

alexreg · 2018-10-25T16:56:02Z

src/librustc_mir/transform/promote_consts.rs

                            let operand = Operand::Copy(promoted_place(ty, span));
                            mem::replace(&mut args[index], operand)
                        }
+                        // already promoted out


Can we explain how we know the candidate is already promoted out here?

alexreg · 2018-10-25T17:02:11Z

src/librustc_mir/transform/qualify_consts.rs

+                    // which happens even without the user requesting it.
+                    // We can error out with a hard error if the argument is not
+                    // constant here.
+                    if (this.qualif - Qualif::NOT_PROMOTABLE).is_empty() {


Thanks for expanding on this. I still think the terminology is a bit confusing: it seems to suggest "return values are never promotable but we're going to promote one here anyway, because it's const."

Well... yes. The rules for automatic promotion don't include function calls. The rules for constants do include function calls. Since the current function requires a constant here, if we did something like 0usize - 1 we'd get an error. If you just do &(0usize - 1) in normal runtime code, you get a warning and a panic at runtime (in debug mode).

Yeah sure, I agree this makes sense, but could we just change the comment to "never promoted for runtime-evaluated/non-const/whatever functions" or such, just to be super-clear and make me happy? :-)

alexreg · 2018-10-25T17:02:15Z

src/librustc_mir/transform/qualify_consts.rs

                    // Be conservative about the returned value of a const fn.
                    let tcx = self.tcx;
                    let ty = dest.ty(self.mir, tcx).to_ty(tcx);
-                    self.qualif = Qualif::empty();


Why did you remove this line?

Otherwise we'd be erasing the NOT_PROMOTABLE qualification we added above.

Yep, but I think you want to reset all fields apart from that one, no? Alternatively, just make sure that's set after self.qualif is reset.

alexreg · 2018-10-25T19:10:35Z

src/librustc_mir/transform/promote_consts.rs

+                        // already promoted this call away entirely. This case occurs when calling
+                        // a function requiring a constant argument and as that constant value
+                        // providing a value whose computation contains another call to a function
+                        // requiring a constant argument.


Thanks for adding this. Sorry to nitpick, but this 2nd sentence doesn't parse grammatically.

alexreg · 2018-10-25T20:26:11Z

Looks good, @oli-obk! Thanks for bearing with me.

alexreg · 2018-10-25T20:26:16Z

@bors r+

bors · 2018-10-25T20:26:17Z

📌 Commit fd77500 has been approved by alexreg

@eddyb

Don't try to promote already promoted out temporaries fixes rust-lang#53201 r? @eddyb

@eddyb

Don't try to promote already promoted out temporaries fixes rust-lang#53201 r? @eddyb

Rollup of 21 pull requests Successful merges: - #54816 (Don't try to promote already promoted out temporaries) - #54824 (Cleanup rustdoc tests with `@!has` and `@!matches`) - #54921 (Add line numbers option to rustdoc) - #55167 (Add a "cheap" mode for `compute_missing_ctors`.) - #55258 (Fix Rustdoc ICE when checking blanket impls) - #55264 (Compile the libstd we distribute with -Ccodegen-unit=1) - #55271 (Unimplement ExactSizeIterator for MIR traversing iterators) - #55292 (Macro diagnostics tweaks) - #55298 (Point at macro definition when no rules expect token) - #55301 (List allowed tokens after macro fragments) - #55302 (Extend the impl_stable_hash_for! macro for miri.) - #55325 (Fix link to macros chapter) - #55343 (rustbuild: fix remap-debuginfo when building a release) - #55346 (Shrink `Statement`.) - #55358 (Remove redundant clone (2)) - #55370 (Update mailmap for estebank) - #55375 (Typo fixes in configure_cmake comments) - #55378 (rustbuild: use configured linker to build boostrap) - #55379 (validity: assert that unions are non-empty) - #55383 (Use `SmallVec` for the queue in `coerce_unsized`.) - #55391 (bootstrap: clean up a few clippy findings)

RalfJung · 2018-11-21T08:12:04Z

Cc @eddyb this touched const qualification.

eddyb · 2018-11-21T09:33:59Z

@RalfJung I should've said something, but I reviewed this and it appears to be a sound relaxation.

rust-highfive assigned eddyb Oct 4, 2018

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

This comment has been minimized.

Sign in to view

Don't try to promote already promoted out temporaries

8b097c4

oli-obk force-pushed the double_promotion branch from b62bc2b to 8b097c4 Compare October 4, 2018 15:07

alexreg suggested changes Oct 25, 2018

View reviewed changes

Explain a comment in more detail

f7629ef

alexreg suggested changes Oct 25, 2018

View reviewed changes

oli-obk added 2 commits October 25, 2018 19:37

Explain why we can encounter a Goto terminator that we want to promote

4dc0287

Clarify exclusion comment further

f4fe9b0

alexreg reviewed Oct 25, 2018

View reviewed changes

oli-obk added 2 commits October 25, 2018 21:14

Grammar nit

ee7f4a2

Clear up nonpromotable const fn call qualification

fd77500

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 Oct 25, 2018

kennytm added a commit to kennytm/rust that referenced this pull request Oct 26, 2018

Rollup merge of rust-lang#54816 - oli-obk:double_promotion, r=alexreg

1960de0

Don't try to promote already promoted out temporaries fixes rust-lang#53201 r? @eddyb

kennytm mentioned this pull request Oct 26, 2018

Rollup of 15 pull requests #55371

Closed

kennytm added a commit to kennytm/rust that referenced this pull request Oct 26, 2018

Rollup merge of rust-lang#54816 - oli-obk:double_promotion, r=alexreg

68d5fdd

Don't try to promote already promoted out temporaries fixes rust-lang#53201 r? @eddyb

kennytm mentioned this pull request Oct 26, 2018

Rollup of 21 pull requests #55382

Merged

bors merged commit fd77500 into rust-lang:master Oct 26, 2018

emilyalbini mentioned this pull request Oct 29, 2018

[beta]: Prepare the 1.31.0 beta release #55405

Merged

oli-obk deleted the double_promotion branch November 21, 2018 09:45

Uh oh!

Conversation

oli-obk commented Oct 4, 2018

Uh oh!

This comment has been minimized.

TimNN commented Oct 16, 2018

Uh oh!

oli-obk commented Oct 25, 2018

Uh oh!

alexreg commented Oct 25, 2018

Uh oh!

oli-obk commented Oct 25, 2018

Uh oh!

oli-obk commented Oct 25, 2018

Uh oh!

bors commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexreg commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexreg Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexreg commented Oct 25, 2018

Uh oh!

alexreg commented Oct 25, 2018

Uh oh!

bors commented Oct 25, 2018

Uh oh!

RalfJung commented Nov 21, 2018

Uh oh!

eddyb commented Nov 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alexreg Oct 25, 2018 •

edited

Loading