Const drop #88558

fee1-dead · 2021-09-01T12:06:40Z

The changes are pretty primitive at this point. But at least it works. ^-^

Problems with the current change that I can think of now:

~const Drop shouldn't change anything in the non-const world.
types that do not have drop glues shouldn't fail to satisfy ~const Drop in const contexts. struct S { a: u8, b: u16 } This might not fail for needs_non_const_drop, but it will fail in rustc_trait_selection.
The current change accepts types that have const Drop impls but have non-const Drop glue.

Fixes #88424.

Significant Changes:

~const Drop is no longer treated as a normal trait bound. In non-const contexts, this bound has no effect, but in const contexts, this restricts the input type and all of its transitive fields to either a) have a const Drop impl or b) can be trivially dropped (i.e. no drop glue)
T: ~const Drop will not be linted like T: Drop.
Instead of recursing and iterating through the type in rustc_mir::transform::check_consts, we use the trait system to special case ~const Drop. See rustc_trait_selection::...::candidate_assembly#assemble_const_drop_candidates and others.

Changes not related to const Dropping and/or changes that are insignificant:

Node.constness_for_typeck no longer returns hir::Constness::Const for type aliases in traits. This was previously used to hack how we determine default bound constness for items. But because we now use an explicit opt-in, it is no longer needed.
Removed is_const_impl_raw query. We have impl_constness, and the only existing use of that query uses HirId, which means we can just operate it with hir.
ty::Destructor now has a field constness, which represents the constness of the destructor.

r? @oli-obk

oli-obk · 2021-09-01T12:49:55Z

can we desugar ~const Drop to an auto-trait bound for ConstDrop which we then properly define for everything that needs it, but never expose it to users?

src/test/ui/rfc-2632-const-trait-impl/const-drop.rs

oli-obk · 2021-09-03T13:06:43Z

In non-const contexts, this bound has no effect

This could be problematic.

struct NotDrop(String) // has drop glue, but no Drop impl

trait MyTrait {}
impl<T: Drop> MyTrait for T {}
impl MyTrait for NotDrop {}

works on stable and can be very useful for some pin related code (source: rust-lang/rfcs#2632 (comment)).

fee1-dead · 2021-09-03T15:14:38Z

nonono. I did not make T: Drop unusable. I only special cased T: ~const Drop in non-const contexts. See this.

fee1-dead · 2021-09-03T15:25:17Z

In fact I see this as a justification for explicit opt-in over implicit & opt-out. We have T: Drop which means "has explicit drop impl; might have drop glue". We then define T: (const-if-const) Drop to be "has const drop glue or can be trivially dropped". If we go the other way (i.e. the implicit and opt-out syntax), then we might need to introduce something like ConstDrop trait and that would have to be another new lang item as well.

bors · 2021-09-09T00:22:36Z

☔ The latest upstream changes (presumably #80522) made this pull request unmergeable. Please resolve the merge conflicts.

rust-timer · 2021-09-15T08:34:40Z

Finished benchmarking commit (cdeba02): comparison url.

Summary: This change led to large relevant mixed results 🤷 in compiler performance.

Moderate improvement in instruction counts (up to -1.7% on full builds of externs)
Large regression in instruction counts (up to 2.7% on full builds of html5ever)

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

oli-obk · 2021-09-15T09:04:53Z

compiler/rustc_trait_selection/src/traits/select/mod.rs

-    /// The weird return type of this function allows it to be used with the `try` (`?`)
-    /// operator within certain functions.
-    fn check_recursion_limit<T: Display + TypeFoldable<'tcx>, V: Display + TypeFoldable<'tcx>>(
+    fn check_recursion_depth<T: Display + TypeFoldable<'tcx>>(


infcx is very sensitive to changes in hot code paths, maybe this change caused it? Can you do a PR without this change (duplicating for your use case) and see how it goes? Or maybe just inline(always) check_recursion_limit?

Let me open a pr adding inline(always) and see how that goes. I don't think anything other than this code caused the regression

Opened #88962.

fee1-dead · 2021-09-15T13:08:54Z

Okay, I looked through the perf results, and there seems to be a regression for mir_promoted and/or mir_const_qualif. Perhaps the way to fix this is to add a fast path for NeedsNonConstDrop::in_any_value_of_ty.

Invoking the trait system is probably really slow...

oli-obk · 2021-09-15T13:17:30Z

compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs

    const IS_CLEARED_ON_MOVE: bool = true;

    fn in_qualifs(qualifs: &ConstQualifs) -> bool {
        qualifs.needs_drop
    }

    fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool {
-        ty.needs_drop(cx.tcx, cx.param_env)


maybe keep this for a fast reject?

oli-obk · 2021-09-16T06:57:01Z

regression fixed by #88965 and #88963

@rustbot label: +perf-regression-triaged

inline(always) on check_recursion_limit r? `@oli-obk` rust-lang#88558 caused a regression, this PR adds `#[inline(always)]` to `check_recursion_limit`, a possible suspect of that regression.

Fast reject for NeedsNonConstDrop Hopefully fixes the regression in rust-lang#88558. I've always wanted to help with the performance of rustc, but it doesn't feel the same when you are fixing a regression caused by your own PR... r? `@oli-obk`

Changes from rust-lang#88558 allowed using `~const Drop` in constants by introducing a new `NeedsNonConstDrop` qualif. The new qualif was also used for promotion purposes, and allowed promotion to happen for values that needs to be dropped but which do have a const drop impl. Since for promoted the drop implementation is never executed, this lead to observable change in behaviour. For example: ```rust struct Panic(); impl const Drop for Panic { fn drop(&mut self) { panic!(); } } fn main() { let _ = &Panic(); } ``` Restore the use of `NeedsDrop` qualif during promotion to avoid the issue.

…-obk Do not promote values with const drop that need to be dropped Changes from rust-lang#88558 allowed using `~const Drop` in constants by introducing a new `NeedsNonConstDrop` qualif. The new qualif was also used for promotion purposes, and allowed promotion to happen for values that needs to be dropped but which do have a const drop impl. Since for promoted the drop implementation is never executed, this lead to observable change in behaviour. For example: ```rust struct Panic(); impl const Drop for Panic { fn drop(&mut self) { panic!(); } } fn main() { let _ = &Panic(); } ``` Restore the use of `NeedsDrop` qualif during promotion to avoid the issue.

…fee1-dead Check `const Drop` impls considering `~const` Bounds 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 rust-lang#92922, thanks `@DrMeepster.` r? `@fee1-dead` `@oli-obk` <sup>(edit: guess I can't request review from two people, lol)</sup> since each of you wrote and reviewed rust-lang#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 rust-lang#92881

rust-highfive assigned oli-obk Sep 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2021

This comment has been minimized.

Sign in to view

oli-obk reviewed Sep 1, 2021

View reviewed changes

src/test/ui/rfc-2632-const-trait-impl/const-drop.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the const-drop branch from 8ec3353 to ad926e2 Compare September 2, 2021 18:10

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the const-drop branch from 891f84e to 11045cd Compare September 3, 2021 10:24

This comment has been minimized.

Sign in to view

fee1-dead force-pushed the const-drop branch from 29661bd to 28ef0d6 Compare September 3, 2021 10:52

fee1-dead marked this pull request as ready for review September 3, 2021 16:02

fee1-dead mentioned this pull request Sep 9, 2021

rustc doesn't seem to understand ~const bounds on associated types #88655

Open

fee1-dead added 11 commits September 9, 2021 05:21

cleanup hir hack

9c1a916

Remove unused query

d9797d2

Const dropping

104e40f

Do not lint for ~const Drop bounds

9125fbf

Add a simple test case

f6f5180

fmt

48a3ba9

better test

894ce92

Const drop selection candidates

a13b13f

Failing test

4eab5c1

fmt

f0a5212

Use trait select logic instead of query

1ca83c6

bors merged commit cdeba02 into rust-lang:master Sep 15, 2021

rustbot added this to the 1.57.0 milestone Sep 15, 2021

rustbot added the perf-regression Performance regression. label Sep 15, 2021

oli-obk reviewed Sep 15, 2021

View reviewed changes

fee1-dead deleted the const-drop branch September 15, 2021 09:09

fee1-dead mentioned this pull request Sep 15, 2021

inline(always) on check_recursion_limit #88962

Merged

fee1-dead mentioned this pull request Sep 15, 2021

Coerce const FnDefs to implement const Fn traits #88963

Merged

oli-obk reviewed Sep 15, 2021

View reviewed changes

fee1-dead mentioned this pull request Sep 15, 2021

Fast reject for NeedsNonConstDrop #88965

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Sep 16, 2021

ghost mentioned this pull request Sep 27, 2021

ICE: upvar_tys called before capture types are inferred with const fn trait bound #89290

Closed

tmiasko mentioned this pull request Oct 18, 2021

Do not promote values with const drop that need to be dropped #89988

Merged

compiler-errors mentioned this pull request Jan 18, 2022

Check const Drop impls considering ~const Bounds #93028

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const drop #88558

Const drop #88558

fee1-dead commented Sep 1, 2021 •

edited

Loading

This comment has been minimized.

oli-obk commented Sep 1, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 3, 2021

fee1-dead commented Sep 3, 2021

fee1-dead commented Sep 3, 2021 •

edited

Loading

bors commented Sep 9, 2021

rust-timer commented Sep 15, 2021

oli-obk Sep 15, 2021

fee1-dead Sep 15, 2021

fee1-dead Sep 15, 2021

fee1-dead commented Sep 15, 2021 •

edited

Loading

oli-obk Sep 15, 2021

oli-obk commented Sep 16, 2021

Const drop #88558

Const drop #88558

Conversation

fee1-dead commented Sep 1, 2021 • edited Loading

This comment has been minimized.

oli-obk commented Sep 1, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Sep 3, 2021

fee1-dead commented Sep 3, 2021

fee1-dead commented Sep 3, 2021 • edited Loading

bors commented Sep 9, 2021

rust-timer commented Sep 15, 2021

oli-obk Sep 15, 2021

Choose a reason for hiding this comment

fee1-dead Sep 15, 2021

Choose a reason for hiding this comment

fee1-dead Sep 15, 2021

Choose a reason for hiding this comment

fee1-dead commented Sep 15, 2021 • edited Loading

oli-obk Sep 15, 2021

Choose a reason for hiding this comment

oli-obk commented Sep 16, 2021

fee1-dead commented Sep 1, 2021 •

edited

Loading

fee1-dead commented Sep 3, 2021 •

edited

Loading

fee1-dead commented Sep 15, 2021 •

edited

Loading