Check `const Drop` impls considering `~const` Bounds by compiler-errors · Pull R... - JOYK Joy of Geek, Geek News, Link all geek

Conversation

Copy link

Contributor

compiler-errors commented 12 days ago •

edited

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

Copy link

Member

fee1-dead left a comment •

edited

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.

compiler-errors reacted with thumbs up emoji

Copy link

Contributor

oli-obk commented 11 days ago

Copy link

Collaborator

rust-timer commented 11 days ago

Awaiting bors try build completion.

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

Copy link

Contributor

bors commented 11 days ago

Trying commit 33e5efb with merge 393a3fc...

Copy link

Contributor

bors commented 11 days ago

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

Copy link

Collaborator

rust-timer commented 11 days ago

Copy link

Collaborator

rust-timer commented 11 days ago

Finished benchmarking commit (393a3fc): 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

Copy link

Member

fee1-dead left a comment

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.

compiler/rustc_middle/src/ty/util.rs

Outdated

| ty::Bound(..)

| ty::Param(_)

| ty::Placeholder(_)

| ty::Never

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

compiler-errors reacted with thumbs up emoji

Copy link

Contributor

Author

compiler-errors commented 11 days ago •

edited

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.

Copy link

Collaborator