make `ConstEvaluatable` more strict #74595

lcnr · 2020-07-21T16:04:22Z

relevant zulip discussion: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/.60ConstEvaluatable.60.20generic.20functions/near/204125452

Let's see how much this impacts. Depending on how this goes this should probably be a future compat warning.

Short explanation: we currently forbid anonymous constants which depend on generic types, e.g. [0; std::mem::size_of::<T>] currently errors.

We previously checked this by evaluating the constant and returned an error if that failed. This however allows things like

const fn foo<T>() -> usize {
    if std::mem::size_of::<*mut T>() < 8 { // size of *mut T does not depend on T
        std::mem::size_of::<T>()
    } else {
        8
    }
}

fn test<T>() {
    let _ = [0; foo::<T>()];
}

which is a backwards compatibility hazard. This also has worrying interactions with mir optimizations (#74491 (comment)) and intrinsics (#74538).

r? @oli-obk @eddyb

lcnr · 2020-07-21T16:21:41Z

@bors try

bors · 2020-07-21T16:21:53Z

⌛ Trying commit 617f7ad0b02ca21994b4cf28d14634486163b166 with merge 536076e3df53cc305e0c3c5f38bdc73f6d9f08bb...

src/librustc_middle/mir/mod.rs

bors · 2020-07-21T18:52:06Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 536076e3df53cc305e0c3c5f38bdc73f6d9f08bb (536076e3df53cc305e0c3c5f38bdc73f6d9f08bb)

lcnr · 2020-07-21T19:04:09Z

It looks like the crater queue is fairly full. @Mark-Simulacrum do we currently have the capacity to do another check run?

I also did some small changes here, so once again @bors try

bors · 2020-07-21T19:04:24Z

⌛ Trying commit 65395c0710061f8745bdfab3520256a178e92c9d with merge a5913924b14838b364c42230cae014cc2534331d...

bors · 2020-07-21T21:46:11Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: a5913924b14838b364c42230cae014cc2534331d (a5913924b14838b364c42230cae014cc2534331d)

lcnr · 2020-07-22T20:00:14Z

Well, will go ahead and add this PR into the crate queue. If we don't have the capacity for this we can just remove it again.

@craterbot check

craterbot · 2020-07-22T20:00:31Z

👌 Experiment pr-74595 created and queued.
🤖 Automatically detected try build a5913924b14838b364c42230cae014cc2534331d
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-07-28T14:22:03Z

🚧 Experiment pr-74595 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-07-28T14:52:07Z

🚨 Experiment pr-74595 has encountered an error: command Command { std: "/workspace/cargo-home/bin/rustup-toolchain-install-master" "8ad7bc3f428300aee6764f6e23527e19eb235e81" "-c" "cargo", kill_on_drop: false } failed
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Mark-Simulacrum · 2020-07-28T17:23:08Z

@craterbot retry seems to have been spurious

craterbot · 2020-07-28T17:23:12Z

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2020-07-28T17:33:56Z

@craterbot retry

craterbot · 2020-07-28T17:33:57Z

🛠️ Experiment pr-74595 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2020-07-31T20:02:58Z

This behavior was introduced in #70452 which was merged on the 15.04.2020, meaning that this has been only stable for 2 stable versions. (since 1.43.0)

So hopefully there aren't many uses of this yet

craterbot · 2020-08-03T12:23:40Z

🚧 Experiment pr-74595 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-08-04T13:12:49Z

🚨 Experiment pr-74595 has encountered an error: command Command { std: "/workspace/cargo-home/bin/rustup-toolchain-install-master" "8ad7bc3f428300aee6764f6e23527e19eb235e81" "-c" "cargo", kill_on_drop: false } failed
🛠️ If the error is fixed use the retry command.

🆘 Can someone from the infra team check in on this? @rust-lang/infra
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Mark-Simulacrum · 2020-08-04T13:24:52Z

@craterbot retry

craterbot · 2020-08-04T13:24:53Z

🛠️ Experiment pr-74595 queued again.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-08-04T13:36:14Z

🚧 Experiment pr-74595 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

lcnr · 2020-09-08T14:50:05Z

I wasn't able to reproduce this locally, but added a #[warn(const_evaluatable_unchecked)] anyways.

oli-obk · 2020-09-08T15:12:47Z

r=me with CI passing

lcnr · 2020-09-08T16:05:45Z

@bors r=oli-obk

bors · 2020-09-08T16:05:47Z

📌 Commit 4226a17 has been approved by oli-obk

Aaron1011 · 2020-09-08T22:27:44Z

@bors retry

bors · 2020-09-08T22:28:00Z

⌛ Testing commit 4226a17 with merge 8d92815944e001c9703489b285f04a9eb6567699...

bors · 2020-09-08T23:05:58Z

💔 Test failed - checks-actions

lcnr · 2020-09-09T06:25:21Z

How can I reproduce this locally? I don't understand this error

oli-obk · 2020-09-09T09:23:20Z

The only thing I can come up with that could cause this difference is --target=i586-unknown-linux-gnu. You may be able to do a --pass=check test run of that specific test and check why it does this. I'm fine with just ignoring the test on that platform once we know why it's happening

lcnr · 2020-09-09T18:22:37Z

@bors r=oli-obk

bors · 2020-09-09T18:22:38Z

📌 Commit 74e0719 has been approved by oli-obk

bors · 2020-09-09T20:04:09Z

⌛ Testing commit 74e0719 with merge e2be5f5...

bors · 2020-09-09T21:59:04Z

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing e2be5f5 to master...

Mark-Simulacrum · 2020-09-15T14:13:43Z

This was an improvement of up to 2% on wf-projection-stress. Presumably not an expected result?

lcnr · 2020-09-16T07:35:42Z

We were afraid of causing a regression here and had similar results 🤔 Probably some inlining changes or something

but not really expected

edit: https://perf.rust-lang.org/compare.html?start=3cfc7fe78eccc754b16981704a098d7bd520e2fd&end=2ce316b2752e1543bc064357871d77fd4896ca27

rust-highfive assigned oli-obk Jul 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2020

lcnr commented Jul 21, 2020

View reviewed changes

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

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2020

withoutboats added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 23, 2020

lcnr mentioned this pull request Jul 28, 2020

Tracking Issue for min_const_generics #74878

Closed

8 tasks

add tracking issue, fix rebase

1dd00e6

lcnr force-pushed the ConstEvaluatable-fut-compat branch from 1a1c3dc to 1dd00e6 Compare September 8, 2020 14:44

fix test

4226a17

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 Sep 8, 2020

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 Sep 8, 2020

fix test on 32 bit systems

74e0719

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 Sep 9, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 9, 2020

bors merged commit e2be5f5 into rust-lang:master Sep 9, 2020

rustbot added this to the 1.48.0 milestone Sep 9, 2020

lcnr deleted the ConstEvaluatable-fut-compat branch September 10, 2020 05:46

make ConstEvaluatable more strict #74595

make ConstEvaluatable more strict #74595

Conversation

lcnr commented Jul 21, 2020 • edited Loading

lcnr commented Jul 21, 2020

bors commented Jul 21, 2020

bors commented Jul 21, 2020

lcnr commented Jul 21, 2020

bors commented Jul 21, 2020

bors commented Jul 21, 2020

lcnr commented Jul 22, 2020

craterbot commented Jul 22, 2020

craterbot commented Jul 28, 2020

craterbot commented Jul 28, 2020

Mark-Simulacrum commented Jul 28, 2020

craterbot commented Jul 28, 2020

lcnr commented Jul 28, 2020

craterbot commented Jul 28, 2020

lcnr commented Jul 31, 2020

craterbot commented Aug 3, 2020

craterbot commented Aug 4, 2020

Mark-Simulacrum commented Aug 4, 2020

craterbot commented Aug 4, 2020

craterbot commented Aug 4, 2020

lcnr commented Sep 8, 2020

oli-obk commented Sep 8, 2020

lcnr commented Sep 8, 2020

bors commented Sep 8, 2020

Aaron1011 commented Sep 8, 2020

bors commented Sep 8, 2020

bors commented Sep 8, 2020

lcnr commented Sep 9, 2020

oli-obk commented Sep 9, 2020

lcnr commented Sep 9, 2020

bors commented Sep 9, 2020

bors commented Sep 9, 2020

bors commented Sep 9, 2020

Mark-Simulacrum commented Sep 15, 2020

lcnr commented Sep 16, 2020 • edited Loading

make `ConstEvaluatable` more strict #74595

make `ConstEvaluatable` more strict #74595

lcnr commented Jul 21, 2020 •

edited

Loading

lcnr commented Sep 16, 2020 •

edited

Loading