Check `const Drop` impls considering `~const` Bounds #93028

compiler-errors · 2022-01-18T09:52:45Z

This PR adds logic to trait selection to account for ~const bounds in custom impl const Drop for types, elaborates the const Drop check in rustc_const_eval to check those bounds, and steals some drop linting fixes from #92922, thanks @drmeepster.

r? @fee1-dead @oli-obk ^{(edit: guess I can't request review from two people, lol)}
since each of you wrote and reviewed #88558, respectively.

Since the logic here is more complicated than what existed, it's possible that this is a perf regression. But it works correctly with tests, and that makes me happy.

Fixes #92881

fee1-dead

On a first glance the changes LGTM.

Preferably we'd want to have some sort of wf check for impl const Drops that requires it to already satisfy the conditions for a structurally const Drop, I'm not saying that it should be in this PR though.

I'll take a deeper look when I have time.

oli-obk · 2022-01-18T21:14:19Z

@bors try @rust-timer queue

rust-timer · 2022-01-18T21:14:20Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-18T21:14:27Z

⌛ Trying commit 33e5efb with merge 393a3fc1041abc8da9d58b1bded5f9319123be5a...

bors · 2022-01-18T22:46:42Z

☀️ Try build successful - checks-actions
Build commit: 393a3fc1041abc8da9d58b1bded5f9319123be5a (393a3fc1041abc8da9d58b1bded5f9319123be5a)

rust-timer · 2022-01-18T22:46:44Z

Queued 393a3fc1041abc8da9d58b1bded5f9319123be5a with parent 9ad5d82, future comparison URL.

rust-timer · 2022-01-19T00:16:36Z

Finished benchmarking commit (393a3fc1041abc8da9d58b1bded5f9319123be5a): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.3% on full builds of keccak check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

fee1-dead

I think I understand what is causing this regression: For user-defined types, you want to select bounds on impl const Drops of the types and check if all bounds are satisfied. Instead of nesting a lot of obligations and running the trait selection process many times, why not extract the components to check bounds for and do just that? For types that require the environment (parameter types) and types that are user-defined, collect them into a list and test them. For other trivial components don't even nest obligations for them.

fee1-dead · 2022-01-19T04:41:29Z

compiler/rustc_middle/src/ty/util.rs

+        | ty::Bound(..)
+        | ty::Param(_)
+        | ty::Placeholder(_)
+        | ty::Never


This should be trivial to make Result<u8, !>: ~const Drop.

compiler-errors · 2022-01-19T04:59:32Z

Instead of nesting a lot of obligations and running the trait selection process many times, why not extract the components to check bounds for and do just that?

I'm hesitant to structurally recurse on the type during candidate assembly (like the code was doing previously) because of normalization, and because other traits (e.g. auto traits, and Copy/Clone) leave that complicated machinery until during confirmation. I could perhaps be a bit more efficient with the list of nested obligations we generate during confirmation by directly recursing in cases like [Ty] => Ty, but I want to leave the code simpler if possible.

Actually I think the regression might've gone away with the last commit I pushed, at least with local testing on the keccak example...
@fee1-dead: Do you mind retrying a perf run? I'll see what I can do to make confirmation a bit more efficient if perf still says this code still needs some work.

Also I'll put up a fix to make !: ~const Drop later, but if you can get that perf run started I can check on it later.

fee1-dead · 2022-01-19T05:05:51Z

@bors try @rust-timer queue

rust-timer · 2022-01-19T05:05:52Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-19T05:06:04Z

⌛ Trying commit ba87be0 with merge b27084a23c48a70efdfbe79d73610550c8326204...

bors · 2022-01-19T06:38:57Z

☀️ Try build successful - checks-actions
Build commit: b27084a23c48a70efdfbe79d73610550c8326204 (b27084a23c48a70efdfbe79d73610550c8326204)

rust-timer · 2022-01-19T06:38:59Z

Queued b27084a23c48a70efdfbe79d73610550c8326204 with parent e5e2b0b, future comparison URL.

rust-timer · 2022-01-19T08:09:52Z

Finished benchmarking commit (b27084a23c48a70efdfbe79d73610550c8326204): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

Large regression in instruction counts (up to 2.3% on full builds of keccak check)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

compiler-errors · 2022-01-19T08:11:17Z

I'll look into this, I guess.

@rustbot author

bors · 2022-01-23T06:25:12Z

⌛ Testing commit b7e4433 with merge a63e6829dedc0100a777ce7851701f935e0bc337...

bors · 2022-01-23T09:16:28Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-01-23T09:16:46Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

compiler-errors · 2022-01-23T17:46:06Z

Bors seems to have failed for exactly no reason... let's try this again? Wonder if that delegate gives me retry permissions..

@bors retry

bors · 2022-01-23T18:51:11Z

⌛ Testing commit b7e4433 with merge 9b1c49dfc9db1922001ed79cb726b59a96311103...

bors · 2022-01-23T23:06:11Z

💥 Test timed out

compiler-errors · 2022-01-23T23:09:16Z

@bors retry because installing awscli timed out (x86_64-msvc-1)

rust-log-analyzer · 2022-01-23T23:15:37Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

compiler-errors · 2022-01-23T23:27:19Z

I'll try this one more time before posting on zulip or something

@bors retry spurious failure with no reason provided

bors · 2022-01-24T08:05:40Z

⌛ Testing commit b7e4433 with merge ef119d7...

bors · 2022-01-24T11:19:56Z

☀️ Test successful - checks-actions
Approved by: fee1-dead
Pushing ef119d7 to master...

rust-timer · 2022-01-24T12:50:38Z

Finished benchmarking commit (ef119d7): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

beepster4096 and others added 3 commits January 17, 2022 21:37

ignore constness when checking dtor predicates

f9eb0b3

Check const Drop impls considering ConstIfConst bounds

6ed42a7

adjust tests

33e5efb

rust-highfive assigned fee1-dead Jan 18, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 18, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 18, 2022

fee1-dead reviewed Jan 18, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 18, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 19, 2022

Short-circuit some trivially const Drop types

ba87be0

fee1-dead reviewed Jan 19, 2022

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 19, 2022

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 20, 2022

beepster4096 mentioned this pull request Jan 20, 2022

Make Box drop through Drop trait #93105

Closed

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 Jan 23, 2022

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 Jan 23, 2022

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 Jan 23, 2022

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 Jan 23, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 24, 2022

bors merged commit ef119d7 into rust-lang:master Jan 24, 2022

rustbot added this to the 1.60.0 milestone Jan 24, 2022

bors mentioned this pull request Jan 24, 2022

Automatically implement AsRepr and allow deriving FromRepr for fieldless enums #81642

Closed

compiler-errors mentioned this pull request Jan 28, 2022

Correctly check auto traits on generator interiors #92449

Closed

compiler-errors deleted the const_drop_bounds branch April 7, 2022 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check `const Drop` impls considering `~const` Bounds #93028

Check `const Drop` impls considering `~const` Bounds #93028

compiler-errors commented Jan 18, 2022 •

edited

Loading

fee1-dead left a comment •

edited

Loading

oli-obk commented Jan 18, 2022

rust-timer commented Jan 18, 2022

bors commented Jan 18, 2022

bors commented Jan 18, 2022

rust-timer commented Jan 18, 2022

rust-timer commented Jan 19, 2022

fee1-dead left a comment

fee1-dead Jan 19, 2022

compiler-errors commented Jan 19, 2022 •

edited

Loading

fee1-dead commented Jan 19, 2022

rust-timer commented Jan 19, 2022

bors commented Jan 19, 2022

bors commented Jan 19, 2022

rust-timer commented Jan 19, 2022

rust-timer commented Jan 19, 2022

compiler-errors commented Jan 19, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

rust-log-analyzer commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

rust-log-analyzer commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

bors commented Jan 24, 2022

bors commented Jan 24, 2022

rust-timer commented Jan 24, 2022

Check const Drop impls considering ~const Bounds #93028

Check const Drop impls considering ~const Bounds #93028

Conversation

compiler-errors commented Jan 18, 2022 • edited Loading

fee1-dead left a comment • edited Loading

Choose a reason for hiding this comment

oli-obk commented Jan 18, 2022

rust-timer commented Jan 18, 2022

bors commented Jan 18, 2022

bors commented Jan 18, 2022

rust-timer commented Jan 18, 2022

rust-timer commented Jan 19, 2022

fee1-dead left a comment

Choose a reason for hiding this comment

fee1-dead Jan 19, 2022

Choose a reason for hiding this comment

compiler-errors commented Jan 19, 2022 • edited Loading

fee1-dead commented Jan 19, 2022

rust-timer commented Jan 19, 2022

bors commented Jan 19, 2022

bors commented Jan 19, 2022

rust-timer commented Jan 19, 2022

rust-timer commented Jan 19, 2022

compiler-errors commented Jan 19, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

rust-log-analyzer commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

bors commented Jan 23, 2022

bors commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

rust-log-analyzer commented Jan 23, 2022

compiler-errors commented Jan 23, 2022

bors commented Jan 24, 2022

bors commented Jan 24, 2022

rust-timer commented Jan 24, 2022

Check `const Drop` impls considering `~const` Bounds #93028

Check `const Drop` impls considering `~const` Bounds #93028

compiler-errors commented Jan 18, 2022 •

edited

Loading

fee1-dead left a comment •

edited

Loading

compiler-errors commented Jan 19, 2022 •

edited

Loading