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

Method call resolution behavioral changes on custom DSTs in Rust 1.70 -> 1.71 #114928

Closed
ZhennanWu opened this issue Aug 17, 2023 · 5 comments · Fixed by #114941
Closed

Method call resolution behavioral changes on custom DSTs in Rust 1.70 -> 1.71 #114928

ZhennanWu opened this issue Aug 17, 2023 · 5 comments · Fixed by #114941
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@ZhennanWu
Copy link

I tried this code:

use std::any::TypeId;
use std::any::Any;
use std::hint::black_box;
struct A<T:?Sized+'static> {
    a: i32,
    b: T
}

impl<T:?Sized+'static> A<T> {
    fn bb(&self) -> TypeId {
        self.b.type_id()
    }
}

pub fn main() {
    let mut a0 = A{a: 8, b: 9 as i32};
    let mut a: &mut A<dyn Any> = &mut a0;
    println!("{:?}",a.bb());
    println!("{:?}",a.b.type_id());
    println!("{:?}",std::any::TypeId::of::<i32>());
}

I expected to see this happen: The three printed TypeIds should be identical, as with the behavior in Rust <=1.70

Instead, this happened:

Program returned: 0
TypeId { t: 2368704253749761200 }
TypeId { t: 5817408772836814867 }
TypeId { t: 5817408772836814867 }

Meta

https://godbolt.org/z/8vvr37Ynf switch between rust versions.

I could not find reference in the 1.71 RELEASES that could explain such behavioral changes.

@ZhennanWu ZhennanWu added the C-bug Category: This is a bug. label Aug 17, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 17, 2023
@tmiasko tmiasko added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-mir-opt-inlining Area: MIR inlining and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 17, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 17, 2023
@tmiasko tmiasko added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 17, 2023
@apiraino
Copy link
Contributor

if my bisection is correct, this surfaced in commit 5546cb6

cc @saethlin (makes sense?)

@saethlin
Copy link
Member

I do not believe that is the correct PR to point at. Bisecting anything related to MIR opts can be tricky because you need to stabilize the MIR opt pipeline against changes like in that PR which don't actually implement any new behavior, they just change the meaning of MIR opt levels.

Try bisecting with -Zmir-opt-level=3? The above PR just changes the opt level 3 inliner behavior to be the default.

@compiler-errors
Copy link
Member

Reproduces since before 2021-01-01 with -Zmir-opt-level=3

@compiler-errors
Copy link
Member

This has to do with the fact that this impl: https://doc.rust-lang.org/std/any/trait.Any.html#impl-Any-for-T ... applies even when T = dyn Any.

Therefore in a polymorphic function like bb, we're eagerly inlining type_id from that impl even though during monomorphization we'd prefer the <dyn Any as Any>::type_id built-in candidate instead.

This seems problematic.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Aug 18, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
…yn, r=lcnr

Don't resolve generic impls that may be shadowed by dyn built-in impls

**NOTE:** This is a hack. This is not trying to be a general fix for the issue that we've allowed overlapping built-in trait object impls and user-written impls for quite a long time, and traits like `Any` rely on this (rust-lang#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (rust-lang#114928) that interacts with this pre-existing issue.

Builtin `dyn Trait` impls may overlap with user-provided blanket impls (`impl<T: ?Sized> Trait for T`) in generic contexts. This leads to bugs when instances are resolved in polymorphic contexts, since we typically prefer object candidates over impl candidates.

This PR implements a (hacky) heuristic to `resolve_associated_item` to account for that unfortunate hole in the type system -- we now bail with ambiguity if we try to resolve a non-rigid instance whose self type is not known to be sized. This makes sure we can still inline instances like `impl<T: Sized> Trait for T`, which can never overlap with `dyn Trait`'s built-in impl, but we avoid inlining an impl that may be shadowed by a `dyn Trait`.

Fixes rust-lang#114928
@bors bors closed this as completed in 66b7bdf Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 19, 2023
Rollup merge of rust-lang#114941 - compiler-errors:inline-shadowed-by-dyn, r=lcnr

Don't resolve generic impls that may be shadowed by dyn built-in impls

**NOTE:** This is a hack. This is not trying to be a general fix for the issue that we've allowed overlapping built-in trait object impls and user-written impls for quite a long time, and traits like `Any` rely on this (rust-lang#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (rust-lang#114928) that interacts with this pre-existing issue.

Builtin `dyn Trait` impls may overlap with user-provided blanket impls (`impl<T: ?Sized> Trait for T`) in generic contexts. This leads to bugs when instances are resolved in polymorphic contexts, since we typically prefer object candidates over impl candidates.

This PR implements a (hacky) heuristic to `resolve_associated_item` to account for that unfortunate hole in the type system -- we now bail with ambiguity if we try to resolve a non-rigid instance whose self type is not known to be sized. This makes sure we can still inline instances like `impl<T: Sized> Trait for T`, which can never overlap with `dyn Trait`'s built-in impl, but we avoid inlining an impl that may be shadowed by a `dyn Trait`.

Fixes rust-lang#114928
@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 Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants