treatment of subtyping in trait matching is unsound #5781
Labels
A-typesystem
Area: The type system
Milestone
Comments
|
nominating for maturity milestone 1: Well defined. |
|
I have a local fix for this that simply makes trait matching invariant. It causes a number of compilation errors. The proper fix is to infer variance (#3598). I agree with the nomination that without settling this, the language is not well-defined. |
|
accepted for well-defined milestone |
|
Note: work on variance inference is underway, so this should be addressable soon-ish. |
|
PR #10153 includes a general variance inference pass that should allow us to address this |
edwardw
added a commit
to edwardw/rust
that referenced
this issue
Mar 15, 2014
Currently, rustc only does so for regions substitution. Applies the same
rule to type and self parameter substitution.
With all three substitutions on, the following becomes legitimate:
```rust
fn foo<'a, 'b>(x: &'b Option<&'a int>) -> Option<&'b int> {
*x
}
```
`x` here is contravariant with respect to the region parameter `b`, so
it is eligible to the return position, also contravariant.
It is also found that rustc handled one special case incorrectly. In
variance inference pass, constraints are added and then solved. But rustc
didn't add any constraint for nullary generic enum and struct so they
were classified as bivariant, the default. Then the following two
implementations of `Foo`:
```rust
pub struct S<T>;
impl Foo for S<int> {
fn bar(&self) {}
}
impl Foo for S<uint> {
fn bar(&self) {}
}
```
would be identical therefor conflicting under type substitution, which
is incorrect.
Closes rust-lang#3598.
Closes rust-lang#5781.
edwardw
added a commit
to edwardw/rust
that referenced
this issue
Mar 16, 2014
Currently, rustc only does so for regions substitution. Applies the same
rule to type and self parameter substitution.
With all three substitutions on, the following becomes legitimate:
```rust
fn foo<'a, 'b>(x: &'b Option<&'a int>) -> Option<&'b int> {
*x
}
```
`x` here is contravariant with respect to the region parameter `b`, so
it is eligible to the return position, also contravariant.
It is also found that rustc handled one special case incorrectly. In
variance inference pass, constraints are added and then solved. But rustc
didn't add any constraint for nullary generic enum and struct so they
were classified as bivariant, the default. Then the following two
implementations of `Foo`:
```rust
pub struct S<T>;
impl Foo for S<int> {
fn bar(&self) {}
}
impl Foo for S<uint> {
fn bar(&self) {}
}
```
would be identical therefor conflicting under type substitution, which
is incorrect.
Closes rust-lang#3598.
Closes rust-lang#5781.
edwardw
added a commit
to edwardw/rust
that referenced
this issue
Mar 16, 2014
Currently, rustc only does so for regions substitution. Applies the same
rule to type and self parameter substitution.
With all three substitutions on, the following becomes legitimate:
```rust
fn foo<'a, 'b>(x: &'b Option<&'a int>) -> Option<&'b int> {
*x
}
```
`x` here is contravariant with respect to the region parameter `b`, so
it is eligible to the return position, also contravariant.
It is also found that rustc handled one special case incorrectly. In
variance inference pass, constraints are added and then solved. But rustc
didn't add any constraint for nullary generic enum and struct so they
were classified as bivariant, the default. Then the following two
implementations of `Foo`:
```rust
pub struct S<T>;
impl Foo for S<int> {
fn bar(&self) {}
}
impl Foo for S<uint> {
fn bar(&self) {}
}
```
would be identical therefor conflicting under type substitution, which
is incorrect.
Closes rust-lang#3598.
Closes rust-lang#5781.
edwardw
added a commit
to edwardw/rust
that referenced
this issue
Mar 17, 2014
Currently, rustc only does so for regions substitution. Applies the same
rule to type and self parameter substitution.
With all three substitutions on, the following becomes legitimate:
```rust
fn foo<'a, 'b>(x: &'b Option<&'a int>) -> Option<&'b int> {
*x
}
```
`x` here is contravariant with respect to the region parameter `b`, so
it is eligible to the return position, also contravariant.
It is also found that rustc handled one special case incorrectly. In
variance inference pass, constraints are added and then solved. But rustc
didn't add any constraint for nullary generic enum and struct so they
were classified as bivariant, the default. Then the following two
implementations of `Foo`:
```rust
pub struct S<T>;
impl Foo for S<int> {
fn bar(&self) {}
}
impl Foo for S<uint> {
fn bar(&self) {}
}
```
would be identical therefor conflicting under type substitution, which
is incorrect.
Closes rust-lang#3598.
Closes rust-lang#5781.
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
|
I believe this was accidentally fixed by DST, as @edwardw's code changes to variance were effectively introduced. I have rebased all test cases here and they are correctly rejected. Will close after a PR to add tests. |
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
I believe that rust-lang#5781 got fixed by the DST work. It duplicated the variance inference work in rust-lang#12828. Therefore, all that is left in rust-lang#5781 is adding a test. Closes rust-lang#5781.
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
I believe that rust-lang#5781 got fixed by the DST work. It duplicated the variance inference work in rust-lang#12828. Therefore, all that is left in rust-lang#5781 is adding the tests mentioned in that issue and its dependent issues. Closes rust-lang#5781.
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
This can break code that looked like:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any + Send> = ...;
x.f();
Change such code to:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any> = ...;
x.f();
That is, upcast before calling methods.
This is a conservative solution to rust-lang#5781. A more proper treatment (see
the xfail'd `trait-contravariant-self.rs`) would take variance into
account. This change fixes the soundness hole.
Some library changes had to be made to make this work. In particular,
`Box<Any>` is no longer showable, and only `Box<Any+Send>` is showable.
Eventually, this restriction can be lifted; for now, it does not prove
too onerous, because `Any` is only used for propagating the result of
task failure.
This patch also adds a test for the variance inference work in rust-lang#12828,
which accidentally landed as part of DST.
Closes rust-lang#5781.
[breaking-change]
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
This can break code that looked like:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any + Send> = ...;
x.f();
Change such code to:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any> = ...;
x.f();
That is, upcast before calling methods.
This is a conservative solution to rust-lang#5781. A more proper treatment (see
the xfail'd `trait-contravariant-self.rs`) would take variance into
account. This change fixes the soundness hole.
Some library changes had to be made to make this work. In particular,
`Box<Any>` is no longer showable, and only `Box<Any+Send>` is showable.
Eventually, this restriction can be lifted; for now, it does not prove
too onerous, because `Any` is only used for propagating the result of
task failure.
This patch also adds a test for the variance inference work in rust-lang#12828,
which accidentally landed as part of DST.
Closes rust-lang#5781.
[breaking-change]
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 26, 2014
This can break code that looked like:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any + Send> = ...;
x.f();
Change such code to:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any> = ...;
x.f();
That is, upcast before calling methods.
This is a conservative solution to rust-lang#5781. A more proper treatment (see
the xfail'd `trait-contravariant-self.rs`) would take variance into
account. This change fixes the soundness hole.
Some library changes had to be made to make this work. In particular,
`Box<Any>` is no longer showable, and only `Box<Any+Send>` is showable.
Eventually, this restriction can be lifted; for now, it does not prove
too onerous, because `Any` is only used for propagating the result of
task failure.
This patch also adds a test for the variance inference work in rust-lang#12828,
which accidentally landed as part of DST.
Closes rust-lang#5781.
[breaking-change]
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 28, 2014
This can break code that looked like:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any + Send> = ...;
x.f();
Change such code to:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any> = ...;
x.f();
That is, upcast before calling methods.
This is a conservative solution to rust-lang#5781. A more proper treatment (see
the xfail'd `trait-contravariant-self.rs`) would take variance into
account. This change fixes the soundness hole.
Some library changes had to be made to make this work. In particular,
`Box<Any>` is no longer showable, and only `Box<Any+Send>` is showable.
Eventually, this restriction can be lifted; for now, it does not prove
too onerous, because `Any` is only used for propagating the result of
task failure.
This patch also adds a test for the variance inference work in rust-lang#12828,
which accidentally landed as part of DST.
Closes rust-lang#5781.
[breaking-change]
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jun 28, 2014
This can break code that looked like:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any + Send> = ...;
x.f();
Change such code to:
impl Foo for Box<Any> {
fn f(&self) { ... }
}
let x: Box<Any> = ...;
x.f();
That is, upcast before calling methods.
This is a conservative solution to rust-lang#5781. A more proper treatment (see
the xfail'd `trait-contravariant-self.rs`) would take variance into
account. This change fixes the soundness hole.
Some library changes had to be made to make this work. In particular,
`Box<Any>` is no longer showable, and only `Box<Any+Send>` is showable.
Eventually, this restriction can be lifted; for now, it does not prove
too onerous, because `Any` is only used for propagating the result of
task failure.
This patch also adds a test for the variance inference work in rust-lang#12828,
which accidentally landed as part of DST.
Closes rust-lang#5781.
[breaking-change]
pcwalton
added a commit
to pcwalton/rust
that referenced
this issue
Jul 1, 2014
… borrows still in scope". This issue was fixed by PR rust-lang#12828 and rust-lang#5781. All that was left was to add tests. Closes rust-lang#12223.
flip1995
pushed a commit
to flip1995/rust
that referenced
this issue
Jul 14, 2020
Fix a broken link in CONTRIBUTING.md changelog: none
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We currently assume (at least in vtable.rs. though not in infer) that self types are contravariant. This is not sound since the
Selftype can appear anywhere, including return types.For example, this program compiles:
Note that we have "produced" a
*uinteven though there is no function in this program that returns one. In this case, it's harmless, but of course one can construct other more harmful examples (particularly if we add other forms of subtyping such as struct inheritance or datasort refinements).The fix is a straight-forward modification (search for FIXMEs) but it invalidates a number of existing impls based around
*const Tand I didn't feel like dealing with the fallout as part of the patch I'm working on.See also #3598---effectively we need a way to infer/declare variance for the Self parameter as well.
The text was updated successfully, but these errors were encountered: