Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fulfillment to check Drop impl compatibility #110577

Merged
merged 3 commits into from
May 6, 2023

Conversation

compiler-errors
Copy link
Member

Use an ObligationCtxt to ensure that a Drop impl does not have stricter requirements than the ADT that it's implemented for, rather than using a SimpleEqRelation to (more or less) syntactically equate predicates on an ADT with predicates on an impl.

r? types

Some background

The old code reads:

// An earlier version of this code attempted to do this checking
// via the traits::fulfill machinery. However, it ran into trouble
// since the fulfill machinery merely turns outlives-predicates
// 'a:'b and T:'b into region inference constraints. It is simpler
// just to look for all the predicates directly.

I'm not sure what this means, but perhaps in the 8 years since that this comment was written (cc #23638) it's gotten easier to process region constraints after doing fulfillment? I don't know how this logic differs from anything we do in the compare_impl_item module. Ironically, later on it says:

// However, it may be more efficient in the future to batch
// the analysis together via the fulfill (see comment above regarding
// the usage of the fulfill machinery), rather than the
// repeated `.iter().any(..)` calls.

Also:

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 20, 2023
@rust-cloud-vms rust-cloud-vms bot force-pushed the drop-impl-fulfill branch from 06d9150 to 85d8079 Compare April 20, 2023 04:49
@lcnr
Copy link
Contributor

lcnr commented Apr 20, 2023

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned spastorino Apr 20, 2023
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me after nits

compiler/rustc_hir_analysis/src/check/dropck.rs Outdated Show resolved Hide resolved
@rust-cloud-vms rust-cloud-vms bot force-pushed the drop-impl-fulfill branch from 85d8079 to c691133 Compare April 21, 2023 04:27
@lcnr
Copy link
Contributor

lcnr commented Apr 21, 2023

wait, not r=me 😅 time for fcp 😁

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add some tests showing how we now check Drop semantically

thinking of some interesting cases:

  • adding a 'static: 'a bound
  • explicitly require a super trait
  • have an additional Vec<T>: Clone bound implied by T: Clone
  • explicitly state transitive dependency for outlives, 'a, 'b: 'a, 'c: 'b, adding 'c: 'a in the Drop impül
  • different way to specify equality of a group of lifetimes, e.g. 'a: 'b, 'b: 'c, 'c: 'a vs 'a: 'b + 'c, 'b: 'a, 'c: 'a
  • implied lt bound in the definition, explicit one in the impl

with this we need an FCP as reverting this change will be breaking. I do really prefer this over the status quo. We already need some hacks here as e.g. unevaluated constants are never structurally eqiual so we need at least some sort of semantic equality her.

@compiler-errors
Copy link
Member Author

Added tests.

Per description and lcnr's comment above, this PR changes the way we do the Drop impl check to no longer require syntactic correspondence between every impl bound and a bound on the ADT, but instead use fulfillment to ensure that the Drop impl's bounds are semantically implied by the ADT it's being implemented for.

Since we now accept more Drop impls, this represents a committment to a larger set of accepted Drop impls, but in reality, this makes the Drop impl check align with the way we check predicate entailment for other items such as methods and associated types.

I can't see any reason why we would prefer the check that we have right now or ever move back to it-- it's both fragile to experimental and future trait system features (constification, changes to implied bounds, non-lifetime-binders, generic const exprs) and also more confusing to understand since it's one of the only places in the type-checking layer that we actually care about syntactic rather than semantic implication.

So with that...

@rfcbot fcp merge

@compiler-errors

This comment was marked as duplicate.

@compiler-errors compiler-errors added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 21, 2023
@compiler-errors
Copy link
Member Author

Oh, lol, it didn't start a compiler fcp luckily.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Apr 21, 2023

Team member @compiler-errors has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 21, 2023
@oli-obk
Copy link
Contributor

oli-obk commented Apr 21, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 21, 2023
@bors
Copy link
Contributor

bors commented Apr 21, 2023

⌛ Trying commit f302ed27f4c28968f630ef1edd12aba3675ebcf5 with merge 516aada95cba5f300f9ea07f80a58cd961ea7736...

@bors
Copy link
Contributor

bors commented Apr 22, 2023

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (516aada95cba5f300f9ea07f80a58cd961ea7736): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.3% [1.6%, 3.1%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.3% [-2.3%, -2.3%] 1
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 22, 2023
@nikomatsakis
Copy link
Contributor

@rfcbot fcp reviewed

cc @spastorino this is relevant to the "always applicable negative impls" discussion we were having recently

@rust-cloud-vms rust-cloud-vms bot force-pushed the drop-impl-fulfill branch from f302ed2 to f4c77b8 Compare May 4, 2023 15:01
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented May 4, 2023

📌 Commit f4c77b8f45098ba57cf99a7850c752847cb6ba1f has been approved by lcnr

It is now in the queue for this repository.

@bors 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 May 4, 2023
@bors
Copy link
Contributor

bors commented May 4, 2023

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

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 4, 2023
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented May 4, 2023

📌 Commit 2e346b6 has been approved by lcnr

It is now in the queue for this repository.

@bors 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 May 4, 2023
@compiler-errors
Copy link
Member Author

@bors rollup=maybe

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request May 6, 2023
… r=lcnr

Use fulfillment to check `Drop` impl compatibility

Use an `ObligationCtxt` to ensure that a `Drop` impl does not have stricter requirements than the ADT that it's implemented for, rather than using a `SimpleEqRelation` to (more or less) syntactically equate predicates on an ADT with predicates on an impl.

r? types

### Some background

The old code reads:

```rust
// An earlier version of this code attempted to do this checking
// via the traits::fulfill machinery. However, it ran into trouble
// since the fulfill machinery merely turns outlives-predicates
// 'a:'b and T:'b into region inference constraints. It is simpler
// just to look for all the predicates directly.
```

I'm not sure what this means, but perhaps in the 8 years since that this comment was written (cc rust-lang#23638) it's gotten easier to process region constraints after doing fulfillment? I don't know how this logic differs from anything we do in the `compare_impl_item` module. Ironically, later on it says:

```rust
// However, it may be more efficient in the future to batch
// the analysis together via the fulfill (see comment above regarding
// the usage of the fulfill machinery), rather than the
// repeated `.iter().any(..)` calls.
```

Also:
* Removes `SimpleEqRelation` which was far too syntactical in its relation.
* Fixes rust-lang#110557
bors added a commit to rust-lang-ci/rust that referenced this pull request May 6, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#110577 (Use fulfillment to check `Drop` impl compatibility)
 - rust-lang#110610 (Add Terminator conversion from MIR to SMIR, part #1)
 - rust-lang#110985 (Fix spans in LLVM-generated inline asm errors)
 - rust-lang#110989 (Make the BUG_REPORT_URL configurable by tools )
 - rust-lang#111167 (debuginfo: split method declaration and definition)
 - rust-lang#111230 (add hint for =< as <=)
 - rust-lang#111279 (More robust debug assertions for `Instance::resolve` on built-in traits with non-standard trait items)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bcc9aa0 into rust-lang:master May 6, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 6, 2023
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label May 25, 2023
@compiler-errors compiler-errors deleted the drop-impl-fulfill branch August 11, 2023 19:59
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 25, 2024
…arams, r=compiler-errors

Graciously handle `Drop` impls introducing more generic parameters than the ADT

Follow up to rust-lang#110577
Fixes rust-lang#126378
Fixes rust-lang#126889

## Motivation

A current issue with the way we check drop impls do not specialize any of their generic parameters is that when the `Drop` impl introduces *more* generic parameters than are present on the ADT, we fail to prove any bounds involving those parameters. This can be demonstrated with the following [code on stable](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=139b65e4294634d7286a3282bc61e628) which fails due to the fact that `<T as Trait>::Assoc == U` is not present in `Foo`s `ParamEnv` even though arguably there is no reason it cannot compiler:
```rust
struct Foo<T: Trait>(T);

trait Trait {
    type Assoc;
}

impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T> {
    //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U` but the struct ...
    fn drop(&mut self) {}
}

fn main() {}
```

I think the motivation for supporting this code is somewhat lacking, it might be useful in practice for deeply nested associated types where you might want to be able to write:
`where T: Trait<Assoc: Other<AnotherAssoc: MoreTrait<YetAnotherAssoc: InnerTrait<Final = U>>>>`
in order to be able to just use `U` in the function body instead of writing out the whole nested associated type. Regardless I don't think there is really any reason to *not* support this code and it is relatively easy to support it.

What I find slightly more compelling is the fact that when defining a const parameter `const N: u8` we desugar that to having a where clause requiring the constant `N` is typed as `u8` (`ClauseKind::ConstArgHasType`). As we *always* desugar const parameters to have these bounds, if we attempt to prove that some const parameter `N` is of type `u8` and there is no bound on `N` in the enviroment that generally indicates usage of an incorrect `ParamEnv` (this has caught a bug already).

Given that, if we write the following code:
```rust
#![feature(associated_const_equality)]
struct Foo<T: Trait>(T);

trait Trait {
    const ASSOC: usize;
}

impl<T: Trait<ASSOC = N>, const N: usize> Drop for Foo<T> {
    fn drop(&mut self) {}
}

fn main() {}
```

The `Drop` impl would have this desugared where clause about `N` being of type `usize`, and if we were to try to prove that where clause in `Foo`'s `ParamEnv` we would ICE as there would not be any `ConstArgHasType` in the environment (which generally indicates improper `ParamEnv` usage. As this is otherwise well formed code (the `T: Trait<ASSOC = N>` causes `N` to be constrained) we have to handle this *somehow* and I believe the only principled way to support this is the changes I have made to `dropck.rs` that would cause these code examples to compiler (Perhaps we could just throw out all `ConstArgHasType` where clauses from the predicates we prove but that makes me nervous even if it might actually be okay).

## The changes

Currently the way `dropck.rs` works is that take the `ParamEnv` of the ADT and instantiate it with the generic arguments used on the self ty of the `impl`. We then instantiate the predicates of the drop impl with the identity params to the impl,  e.g. in the original example `<T as Trait>::Assoc == U` stays as `<T as Trait>::Assoc == U`. We then attempt to prove all the where clauses in the instantiated env of the self type ADT.

This PR changes us to first instantiate the impl with infer vars, then we equate the self type (with infer vars as its generic arguments) with the self type as written by the user. This causes all generic parameters on the impl that are constrained via associated type/const equality bounds to be left as inference variables while all other parameters are still `Ty`/`Const`/`Region`

Finally when instantiating the predicates on the impl, instead of using the identity arguments, we use the list of inference variables of which some have been inferred to the impl parameters. In practice this means that we wind up proving `<T as Trait>::Assoc == ?x` which can succeed just fine. In the const generics example we would wind up trying to prove `ConstArgHasType(?x: usize)` instead of `ConstArgHasType(N: usize)` which avoids the ICE as it is expected to encounter goals of the form `?x: usize`.

At a higher level the way I justify/think about this is that as we are proving goals in the environment of the ADT (`Foo` in the above examples), we do not expect to encounter generic parameters from a different environment so we must "deal with them" somehow. In this PR we handle them by replacing them with inference variables as they should either *actually* be unconstrained (and we will error later) or they are constrained to be equal to some associated type/const.

To go along with this it would be nice if we were not instantiating the adt's env with the generic arguments to the ADT in the `Drop` impl as it would make it clearer we are proving bounds in the adt's env instead of the `Drop` impl's. Instead we would map the predicates on the drop impl to be valid in the environment of the adt. In practice this causes diagnostic regressions as all of the generic parameters in errors refer to the ones defined on the adt; attempting to map these back to the ones on the impl, while possible, is involved as writing a `TypeFolder` over `FulfillmentError` is non trivial.

## Edge cases

There are some subtle interactions here:

One is that we should not allow `<T as Trait>::Assoc == U` to be present on the `Drop` if `U` is constrained by the self type of the impl and the bound is not present in the ADT's environment. demonstrated with the [following code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af839e2c3e43e03a624825c58af84dff):
```rust
trait Trait {
    type Assoc;
}

struct Foo<T: Trait, U: ?Sized>(T, U);

impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T, U> {
    //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U`
    fn drop(&mut self) {}
}

fn main() {}
```
This is tested at `tests/ui/dropck/constrained_by_assoc_type_equality_and_self_ty.rs`.

Another weirdness is that we permit the following code to compile now:
```rust
struct Foo<T>(T);

impl<'a, T: 'a> Drop for Foo<T> {
    fn drop(&mut self) {}
}
```
This is caused by the fact that we permit unconstrained lifetime parameters in trait implementations as long as they are not used in associated types (so we do not wind up erroring on this code like we perhaps ought to), combined with the fact that as we are now proving `T: '?x` instead of `T: 'a` which allows proving the bound via `'?x= 'empty` wheras previously it would have failed.

This is tested as part of `tests/ui/dropck/reject-specialized-drops-8142.rs`.

---

r? `@compiler-errors`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 26, 2024
Rollup merge of rust-lang#127220 - BoxyUwU:dropck_handle_extra_impl_params, r=compiler-errors

Graciously handle `Drop` impls introducing more generic parameters than the ADT

Follow up to rust-lang#110577
Fixes rust-lang#126378
Fixes rust-lang#126889

## Motivation

A current issue with the way we check drop impls do not specialize any of their generic parameters is that when the `Drop` impl introduces *more* generic parameters than are present on the ADT, we fail to prove any bounds involving those parameters. This can be demonstrated with the following [code on stable](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=139b65e4294634d7286a3282bc61e628) which fails due to the fact that `<T as Trait>::Assoc == U` is not present in `Foo`s `ParamEnv` even though arguably there is no reason it cannot compiler:
```rust
struct Foo<T: Trait>(T);

trait Trait {
    type Assoc;
}

impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T> {
    //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U` but the struct ...
    fn drop(&mut self) {}
}

fn main() {}
```

I think the motivation for supporting this code is somewhat lacking, it might be useful in practice for deeply nested associated types where you might want to be able to write:
`where T: Trait<Assoc: Other<AnotherAssoc: MoreTrait<YetAnotherAssoc: InnerTrait<Final = U>>>>`
in order to be able to just use `U` in the function body instead of writing out the whole nested associated type. Regardless I don't think there is really any reason to *not* support this code and it is relatively easy to support it.

What I find slightly more compelling is the fact that when defining a const parameter `const N: u8` we desugar that to having a where clause requiring the constant `N` is typed as `u8` (`ClauseKind::ConstArgHasType`). As we *always* desugar const parameters to have these bounds, if we attempt to prove that some const parameter `N` is of type `u8` and there is no bound on `N` in the enviroment that generally indicates usage of an incorrect `ParamEnv` (this has caught a bug already).

Given that, if we write the following code:
```rust
#![feature(associated_const_equality)]
struct Foo<T: Trait>(T);

trait Trait {
    const ASSOC: usize;
}

impl<T: Trait<ASSOC = N>, const N: usize> Drop for Foo<T> {
    fn drop(&mut self) {}
}

fn main() {}
```

The `Drop` impl would have this desugared where clause about `N` being of type `usize`, and if we were to try to prove that where clause in `Foo`'s `ParamEnv` we would ICE as there would not be any `ConstArgHasType` in the environment (which generally indicates improper `ParamEnv` usage. As this is otherwise well formed code (the `T: Trait<ASSOC = N>` causes `N` to be constrained) we have to handle this *somehow* and I believe the only principled way to support this is the changes I have made to `dropck.rs` that would cause these code examples to compiler (Perhaps we could just throw out all `ConstArgHasType` where clauses from the predicates we prove but that makes me nervous even if it might actually be okay).

## The changes

Currently the way `dropck.rs` works is that take the `ParamEnv` of the ADT and instantiate it with the generic arguments used on the self ty of the `impl`. We then instantiate the predicates of the drop impl with the identity params to the impl,  e.g. in the original example `<T as Trait>::Assoc == U` stays as `<T as Trait>::Assoc == U`. We then attempt to prove all the where clauses in the instantiated env of the self type ADT.

This PR changes us to first instantiate the impl with infer vars, then we equate the self type (with infer vars as its generic arguments) with the self type as written by the user. This causes all generic parameters on the impl that are constrained via associated type/const equality bounds to be left as inference variables while all other parameters are still `Ty`/`Const`/`Region`

Finally when instantiating the predicates on the impl, instead of using the identity arguments, we use the list of inference variables of which some have been inferred to the impl parameters. In practice this means that we wind up proving `<T as Trait>::Assoc == ?x` which can succeed just fine. In the const generics example we would wind up trying to prove `ConstArgHasType(?x: usize)` instead of `ConstArgHasType(N: usize)` which avoids the ICE as it is expected to encounter goals of the form `?x: usize`.

At a higher level the way I justify/think about this is that as we are proving goals in the environment of the ADT (`Foo` in the above examples), we do not expect to encounter generic parameters from a different environment so we must "deal with them" somehow. In this PR we handle them by replacing them with inference variables as they should either *actually* be unconstrained (and we will error later) or they are constrained to be equal to some associated type/const.

To go along with this it would be nice if we were not instantiating the adt's env with the generic arguments to the ADT in the `Drop` impl as it would make it clearer we are proving bounds in the adt's env instead of the `Drop` impl's. Instead we would map the predicates on the drop impl to be valid in the environment of the adt. In practice this causes diagnostic regressions as all of the generic parameters in errors refer to the ones defined on the adt; attempting to map these back to the ones on the impl, while possible, is involved as writing a `TypeFolder` over `FulfillmentError` is non trivial.

## Edge cases

There are some subtle interactions here:

One is that we should not allow `<T as Trait>::Assoc == U` to be present on the `Drop` if `U` is constrained by the self type of the impl and the bound is not present in the ADT's environment. demonstrated with the [following code](https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=af839e2c3e43e03a624825c58af84dff):
```rust
trait Trait {
    type Assoc;
}

struct Foo<T: Trait, U: ?Sized>(T, U);

impl<T: Trait<Assoc = U>, U: ?Sized> Drop for Foo<T, U> {
    //~^ ERROR: `Drop` impl requires `<T as Trait>::Assoc == U`
    fn drop(&mut self) {}
}

fn main() {}
```
This is tested at `tests/ui/dropck/constrained_by_assoc_type_equality_and_self_ty.rs`.

Another weirdness is that we permit the following code to compile now:
```rust
struct Foo<T>(T);

impl<'a, T: 'a> Drop for Foo<T> {
    fn drop(&mut self) {}
}
```
This is caused by the fact that we permit unconstrained lifetime parameters in trait implementations as long as they are not used in associated types (so we do not wind up erroring on this code like we perhaps ought to), combined with the fact that as we are now proving `T: '?x` instead of `T: 'a` which allows proving the bound via `'?x= 'empty` wheras previously it would have failed.

This is tested as part of `tests/ui/dropck/reject-specialized-drops-8142.rs`.

---

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: bound types encountered in super_relate_tys