change equate for binders to not rely on subtyping#118247
Merged
bors merged 3 commits intorust-lang:masterfrom Feb 29, 2024
Merged
change equate for binders to not rely on subtyping#118247bors merged 3 commits intorust-lang:masterfrom
bors merged 3 commits intorust-lang:masterfrom
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
summary by @spastorino and @lcnr
Context
The following code:
has UB from hitting the
unreachable_unchecked. This happens becauseTypeId::of::<One>()is not the same asTypeId::of::<Two>()despite them being considered the same types by the type checker.Currently the type checker considers binders to be equal if subtyping succeeds in both directions:
for<'a> T<'a> eq for<'b> U<'b>holds iffor<'a> exists<'b> T<'b> <: T'<a> AND for<'b> exists<'a> T<'a> <: T<'b>holds. This results infor<'a> fn(&'a (), &'a ())andfor<'a, 'b> fn(&'a (), &'b ())being equal in the type system.TypeIdis computed by looking at the structure of a type. Even though these types are semantically equal, they have a different structure resulting in them having differentTypeId. This can break invariants of unsafe code at runtime and is unsound when happening at compile time, e.g. when using const generics.So as seen in
main, we can assign a value of typeFoo::<One>to a binding of typeFoo<Two>given those are considered the same type but then when we callderef, it callsdowncast_refthat relies onTypeIdand we would hit theNonearm as these have differentTypeIds.As stated in #97156 (comment), this causes the API of existing crates to be unsound.
What should we do about this
The same type resulting in different
TypeIds is a significant footgun, breaking a very reasonable assumptions by authors of unsafe code. It will also be unsound by itself once they are usable in generic contexts with const generics.There are two options going forward here:
for<'a> fn(&'a (), &'a ())andfor<'a, 'b> fn(&'a (), &'b ())to be equal, but normalize them to a common representation so that theirTypeIdare also the same.for<'a> fn(&'a (), &'a ())andfor<'a, 'b> fn(&'a (), &'b ())still have differentTypeIds but are now also considered to not be semantically equal.Advantages of the first approach:
General thoughts:
Advantages of the second approach:
This PR goes with the second approach. A crater run did not result in any regressions. I am personally very hesitant about trying the first approach due to the above reasons. It feels like there are more unknowns when going that route.
Changing the way we equate binders
Relating bound variables from different depths already results in a universe error in equate. We therefore only need to make sure that there is 1-to-1 correspondence between bound variables when relating binders. This results in concrete types being structurally equal after anonymizing their bound variables.
We implement this by instantiating one of the binder with placeholders and the other with inference variables and then equating the instantiated types. We do so in both directions.
More formally, we change the typing rules as follows:
to
Fixes #97156
r? @lcnr