Reject specialized Drop impls. #23638
Conversation
|
r? @Aatch (rust_highfive has picked a reviewer for you, use r? to override) |
|
(@nikomatsakis heh, there are still all those fixme's in there for me to add proper error codes. I will post a follow-up commit with those; but please do review the overall strategy in the meantime if you have time.) |
|
a thought for how to enforce #23638 (comment) -- instead of the current check, make skolemized versions of the type/region parameters, create inference variables for the impl parameters, and try to unify (we'll have to check regions too) |
|
@nikomatsakis ironically that is what I tried to do originally... (well, sort of) (update 25 minutes later): actually what I tried to do originally was much more ad-hoc. What you describe sounds much better. Will try it now. |
|
@pnkfelix it almost seems like it'd be easier to just let drops be more specialized and sometimes not run them |
|
@pnkfelix for the regions, we could probably use the skolemization machinery aimed at late-bound regions to avoid having to run region resolve |
|
(seems like more-or-less the perfect tool for this job, since it is designed to determine whether something is more or less polymorphic) |
|
(or you could prob just check that each type/region parameter is distinct) |
@nikomatsakis yeah I keep debating about whether I'm wasting my time trying to do it via skolemization. (I am currently justifying it as a good exercise to better acquaint myself with the machinery, but if i don't get it working soon, I will probably resort to the brute-force rejection of duplicate actual parameters) |
Well except that's not what the current implementation does in these cases, I think. To my knowledge it just blindly runs the destructors. (We could certainly try to implement what you suggest, but I think this approach is still necessary, if only as the basis for a lint.) |
|
@pnkfelix I agree this is not current behavior of trans |
|
r+ |
pnkfelix
force-pushed
the
fsk-reject-specialized-drops
branch
from
379d7e8
to
c6cd92c
Compare
|
@bors r=nikomatsakis c6cd92c |
|
☔ The latest upstream changes (presumably #23654) made this pull request unmergeable. Please resolve the merge conflicts. |
one of the helper method impls.
This was necessary to avoid specialized `Drop` impls for the two structs.
See Issue 8142 for discussion.
This makes it illegal for a Drop impl to be more specialized than the
original item.
So for example, all of the following are now rejected (when they would
have been blindly accepted before):
```rust
struct S<A> { ... };
impl Drop for S<i8> { ... } // error: specialized to concrete type
struct T<'a> { ... };
impl Drop for T<'static> { ... } // error: specialized to concrete region
struct U<A> { ... };
impl<A:Clone> Drop for U<A> { ... } // error: added extra type requirement
struct V<'a,'b>;
impl<'a,'b:a> Drop for V<'a,'b> { ... } // error: added extra region requirement
```
Due to examples like the above, this is a [breaking-change].
(The fix is to either remove the specialization from the `Drop` impl,
or to transcribe the requirements into the struct/enum definition;
examples of both are shown in the PR's fixed to `libstd`.)
----
This is likely to be the last thing blocking the removal of the
`#[unsafe_destructor]` attribute.
Includes two new error codes for the new dropck check.
Update run-pass tests to accommodate new dropck pass.
Update tests and docs to reflect new destructor restriction.
----
Implementation notes:
We identify Drop impl specialization by not being as parametric as the
struct/enum definition via unification.
More specifically:
1. Attempt unification of a skolemized instance of the struct/enum
with an instance of the Drop impl's type expression where all of
the impl's generics (i.e. the free variables of the type
expression) have been replaced with unification variables.
2. If unification fails, then reject Drop impl as specialized.
3. If unification succeeds, check if any of the skolemized
variables "leaked" into the constraint set for the inference
context; if so, then reject Drop impl as specialized.
4. Otherwise, unification succeeded without leaking skolemized
variables: accept the Drop impl.
We identify whether a Drop impl is injecting new predicates by simply
looking whether the predicate, after an appropriate substitution,
appears on the struct/enum definition.
pnkfelix
force-pushed
the
fsk-reject-specialized-drops
branch
from
c6cd92c
to
1955e05
Compare
Reject specialized Drop impls. See Issue rust-lang#8142 for discussion. This makes it illegal for a Drop impl to be more specialized than the original item. So for example, all of the following are now rejected (when they would have been blindly accepted before): ```rust struct S<A> { ... }; impl Drop for S<i8> { ... } // error: specialized to concrete type struct T<'a> { ... }; impl Drop for T<'static> { ... } // error: specialized to concrete region struct U<A> { ... }; impl<A:Clone> Drop for U<A> { ... } // error: added extra type requirement struct V<'a,'b>; impl<'a,'b:a> Drop for V<'a,'b> { ... } // error: added extra region requirement ``` Due to examples like the above, this is a [breaking-change]. (The fix is to either remove the specialization from the `Drop` impl, or to transcribe the requirements into the struct/enum definition; examples of both are shown in the PR's fixed to `libstd`.) ---- This is likely to be the last thing blocking the removal of the `#[unsafe_destructor]` attribute. Fix rust-lang#8142 Fix rust-lang#23584
… 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
… 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
Reject specialized Drop impls.
See Issue #8142 for discussion.
This makes it illegal for a Drop impl to be more specialized than the original item.
So for example, all of the following are now rejected (when they would have been blindly accepted before):
Due to examples like the above, this is a [breaking-change].
(The fix is to either remove the specialization from the
Dropimpl, or to transcribe the requirements into the struct/enum definition; examples of both are shown in the PR's fixed tolibstd.)This is likely to be the last thing blocking the removal of the
#[unsafe_destructor]attribute.Fix #8142
Fix #23584