Function arguments should never get promoted #59724

oli-obk · 2019-04-05T12:38:39Z

fixes #59469

rust-highfive · 2019-04-05T12:38:43Z

r? @matthewjasper

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

oli-obk · 2019-04-05T12:39:02Z

r? @eddyb

eddyb · 2019-04-05T12:43:13Z

src/librustc_mir/transform/qualify_consts.rs

@@ -639,7 +639,7 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
                            per_local.insert(local);
                        }
                    }
-                    cx.per_local[IsNotPromotable].insert(local);
+                    cx.per_local[IsNotConst].insert(local);


I think we should do this for all locals where if !temps[local].is_promotable().

What I meant is that we should do this for all locals, not just temps.
That is, don't do it on a case-by-case basis in this match, but rather have this after it:

if !temps[local].is_promotable() { cx.per_local[IsNotConst].insert(local); }

That's a change that I don't want to backport. are you alright with r+ing this PR and I'll address this immediately in a master-only PR?

There's a risk of there being more issues we haven't hit due to this not being general enough.
This is not just code cleanup, I think right now this is kind of broken.

pnkfelix · 2019-04-05T15:04:33Z

As an experiment, I tried backporting this change to the beta branch, to double-check that it actually addresses the regression.

One potential issue is that the regression tests may not themselves be immediately backportable as written, because I do not think #59044 is in the beta branch. Just a warning.

pnkfelix · 2019-04-05T15:17:25Z

(commit ccc0cd8 itself does seem to address the key problem in the beta branch.)

@oli-obk @eddyb Regarding the beta-nomination, I do not think we can afford to wait until the next rustc meeting. Do you think we should go ahead and beta-accept this now? Or wait until @nikomatsakis is back (on monday I think)?

(To be absolutely clear, I have no personal problem with backporting ccc0cd8 to beta. Though it might be good to explore whether the generalization suggested by @eddyb in their review is what really needs to be backported.)

oli-obk · 2019-04-05T16:22:26Z

I think we should beta accept this without a full compiler team consensus as it's small and important to get fixed. There seems no alternative to me.

I can create a beta backport that regenerates the ui output the way beta expects it.

Centril · 2019-04-06T01:43:16Z

There seems no alternative to me.

Yes; absolutely. I would really like to avoid a C-future-compatibility warning due to a failure to backport.

eddyb · 2019-04-06T08:38:03Z

src/librustc_mir/transform/qualify_consts.rs

@@ -639,15 +639,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
                            per_local.insert(local);
                        }
                    }
-                    cx.per_local[IsNotPromotable].insert(local);
+                    cx.per_local[IsNotConst].insert(local);
                }

                LocalKind::Var if mode == Mode::Fn => {
                    cx.per_local[IsNotConst].insert(local);


Like, does this mean we'll try to promote a Var in a const fn?!

eddyb · 2019-04-06T08:39:22Z

src/librustc_mir/transform/qualify_consts.rs

@@ -817,15 +817,15 @@ impl<'a, 'tcx> Checker<'a, 'tcx> {
            }
        }

-        // Ensure the `IsNotPromotable` qualification is preserved.
+        // Ensure the `IsNotConst` qualification is preserved.
        // NOTE(eddyb) this is actually unnecessary right now, as
        // we never replace the local's qualif, but we might in
        // the future, and so it serves to catch changes that unset
        // important bits (in which case, asserting `contains` could
        // be replaced with calling `insert` to re-set the bit).
        if kind == LocalKind::Temp {


Again, here, this seems risky, why don't we do this for all locals?
I think relying on the Var / Temp distinction was a bit of a mistake... I mean, here, as opposed to in the analysis in promote_consts.

eddyb · 2019-04-06T08:44:03Z

r=me with or without #59724 (comment) / #59724 (comment) resolved

For runtime code, this PR is probably fine (it marks all args and vars as "not const" - not the return place, but that tends not to ever get borrowed or passed to a function), but for const fn some of this pass might still be bogus.

pnkfelix · 2019-04-06T12:50:23Z

I think we should beta accept this without a full compiler team consensus as it's small and important to get fixed. There seems no alternative to me.

I agree with this and am going to mark the PR as beta-accepted accordingly

eddyb · 2019-04-06T14:05:47Z

I guess I shouldn't delay this needlessly:
@bors r+ p=10 (needs backport soon)

bors · 2019-04-06T14:05:48Z

📌 Commit cc66b17 has been approved by eddyb

bors · 2019-04-06T16:40:26Z

⌛ Testing commit cc66b17 with merge f46819ad4835ed97d28d4f2b276314d63551fff7...

bors · 2019-04-06T17:26:08Z

💔 Test failed - checks-travis

bors · 2019-04-07T19:47:35Z

📌 Commit 01e8394 has been approved by eddyb

pietroalbini · 2019-04-07T20:31:35Z

@bors p=1 (needs to be backported)

bors · 2019-04-07T21:00:32Z

⌛ Testing commit 01e8394 with merge 4976e83b11d48330e77f7aef8c79666fbfffdb4f...

pietroalbini · 2019-04-07T21:12:09Z

@bors retry

Yielding priority to the beta backports.

bors · 2019-04-07T21:30:18Z

⌛ Testing commit 01e8394 with merge b6978e58ef44ca7c3b33ccaff8185111416e30ad...

pietroalbini · 2019-04-07T21:31:32Z

@bors retry

Yielding priority again to the beta backports.

@ghost

[beta] Rollup backports Cherry-picked: * #58021: Fix fallout from #57667 * #59599: Updated RELEASES.md for 1.34.0 * #59587: Remove #[doc(hidden)] from Error::type_id * #58994: Hide deprecation warnings inside derive expansions * #58015: Expand docs for `TryFrom` and `TryInto`. * #59770: ci: pin android emulator to 28.0.23 * #59704: ci: Update FreeBSD tarball downloads * #59257: Update CI configuration for building Redox libraries * #59724: Function arguments should never get promoted r? @ghost

Centril · 2019-04-07T23:06:10Z

Moving before the clippy PR, @bors p=15

bors · 2019-04-07T23:28:21Z

⌛ Testing commit 01e8394 with merge 330db3bec5731d012cdcf05ddfe577bd0b828a65...

Centril · 2019-04-07T23:30:00Z

@bors retry

rust-highfive · 2019-04-07T23:32:08Z

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

Checking out files: 100% (18109/18109), done.
travis_time:end:18f31c75:start=1554679821140769000,finish=1554679830524436000,duration=9383667000
$ cd rust-lang/rust
$ git checkout -qf 330db3bec5731d012cdcf05ddfe577bd0b828a65
fatal: reference is not a tree: 330db3bec5731d012cdcf05ddfe577bd0b828a65
The command "git checkout -qf 330db3bec5731d012cdcf05ddfe577bd0b828a65" failed and exited with 128 during .
Your build has been stopped.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2019-04-08T01:18:20Z

⌛ Testing commit 01e8394 with merge 3750348...

Function arguments should never get promoted fixes #59469

bors · 2019-04-08T04:01:28Z

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

@ghost

[beta] Rollup backports Cherry-picked: * #58021: Fix fallout from #57667 * #59599: Updated RELEASES.md for 1.34.0 * #59587: Remove #[doc(hidden)] from Error::type_id * #58994: Hide deprecation warnings inside derive expansions * #58015: Expand docs for `TryFrom` and `TryInto`. * #59770: ci: pin android emulator to 28.0.23 * #59704: ci: Update FreeBSD tarball downloads * #59257: Update CI configuration for building Redox libraries * #59724: Function arguments should never get promoted * #59499: Fix broken download link in the armhf-gnu image * #58330: Add rustdoc JS non-std tests * #58848: Prevent cache issues on version updates r? @ghost

rust-highfive assigned matthewjasper Apr 5, 2019

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

rust-highfive assigned eddyb and unassigned matthewjasper Apr 5, 2019

oli-obk mentioned this pull request Apr 5, 2019

Wrapping simd call results in compiler panic (nightly) #59469

Closed

eddyb reviewed Apr 5, 2019

View reviewed changes

jonas-schievink added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 5, 2019

oli-obk force-pushed the const_arg_ice branch from d04825d to 49025f3 Compare April 5, 2019 13:46

eddyb reviewed Apr 6, 2019

View reviewed changes

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 6, 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 Apr 6, 2019

bors mentioned this pull request Apr 6, 2019

Use for_each to extend collections #59740

Merged

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

pietroalbini mentioned this pull request Apr 7, 2019

[beta] Rollup backports #59737

Merged

pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 7, 2019

bors added a commit that referenced this pull request Apr 8, 2019

Auto merge of #59724 - oli-obk:const_arg_ice, r=eddyb

3750348

Function arguments should never get promoted fixes #59469

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 8, 2019

bors merged commit 01e8394 into rust-lang:master Apr 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Function arguments should never get promoted #59724

Function arguments should never get promoted #59724

oli-obk commented Apr 5, 2019

rust-highfive commented Apr 5, 2019

oli-obk commented Apr 5, 2019

eddyb Apr 5, 2019

eddyb Apr 5, 2019

oli-obk Apr 5, 2019

eddyb Apr 6, 2019

pnkfelix commented Apr 5, 2019

pnkfelix commented Apr 5, 2019 •

edited

Loading

oli-obk commented Apr 5, 2019

Centril commented Apr 6, 2019

eddyb Apr 6, 2019

eddyb Apr 6, 2019

eddyb commented Apr 6, 2019

pnkfelix commented Apr 6, 2019

eddyb commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

Centril commented Apr 7, 2019

bors commented Apr 7, 2019

Centril commented Apr 7, 2019

rust-highfive commented Apr 7, 2019

bors commented Apr 8, 2019

bors commented Apr 8, 2019

Function arguments should never get promoted #59724

Function arguments should never get promoted #59724

Conversation

oli-obk commented Apr 5, 2019

rust-highfive commented Apr 5, 2019

oli-obk commented Apr 5, 2019

eddyb Apr 5, 2019

Choose a reason for hiding this comment

eddyb Apr 5, 2019

Choose a reason for hiding this comment

oli-obk Apr 5, 2019

Choose a reason for hiding this comment

eddyb Apr 6, 2019

Choose a reason for hiding this comment

pnkfelix commented Apr 5, 2019

pnkfelix commented Apr 5, 2019 • edited Loading

oli-obk commented Apr 5, 2019

Centril commented Apr 6, 2019

eddyb Apr 6, 2019

Choose a reason for hiding this comment

eddyb Apr 6, 2019

Choose a reason for hiding this comment

eddyb commented Apr 6, 2019

pnkfelix commented Apr 6, 2019

eddyb commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 6, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

bors commented Apr 7, 2019

pietroalbini commented Apr 7, 2019

Centril commented Apr 7, 2019

bors commented Apr 7, 2019

Centril commented Apr 7, 2019

rust-highfive commented Apr 7, 2019

bors commented Apr 8, 2019

bors commented Apr 8, 2019

pnkfelix commented Apr 5, 2019 •

edited

Loading