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

compiler-errors · 2023-08-17T18:38:01Z

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 (#57893) -- this PR specifically aims to mitigate a new unsoundness that is uncovered by the MIR inliner (#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 #114928

rustbot · 2023-08-17T18:38:07Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

compiler-errors · 2023-08-17T18:38:41Z

tests/mir-opt/dont_inline_type_id.call.Inline.diff

+ _2 = &(*_1);
+- _0 = <T as Any>::type_id(move _2) -> [return: bb1, unwind unreachable];
+ StorageLive(_3);
+ _3 = std::intrinsics::type_id::<T>() -> [return: bb1, unwind unreachable];


This is the problematic inlined line -- when this function is eventually monomorphized for T = dyn Any, we're acquiring the type-id of dyn Any rather than going thru the vtable.

lcnr

please move this check to instance resolve instead and add these two tests to make sure that they don't also end up triggering this bug

trait Trait {
    fn foo(&self) {}
}

impl<T: ?Sized> Trait for T {
    fn foo(&self) {
        println!("generic: {}", std::any::type_name::<T>());
    }
}


fn bar<T: ?Sized>() -> fn(&T) {
    Trait::foo
    // If const prop where to propagate the instance
}

fn main() {
    bar::<dyn Trait>()(&1);
}

#![feature(inline_const)]
trait Trait {
    fn foo(&self) {}
}

impl<T: ?Sized> Trait for T {
    fn foo(&self) {
        println!("generic: {}", std::any::type_name::<T>());
    }
}


fn bar<T: ?Sized>() -> fn(&T) {
    const { Trait::foo as fn(&T) }
}

fn main() {
    bar::<dyn Trait>()(&1);
}

compiler/rustc_mir_transform/src/inline.rs

lcnr · 2023-08-18T09:19:03Z

r? @lcnr

lcnr · 2023-08-18T20:42:53Z

r=me after a types FCP. While this change is not breaking existing code, it is a new special-case of the types system so I think it's a good idea to notify the rest of the types team of this.

compiler-errors · 2023-08-21T23:01:43Z

@rfcbot fcp merge

See PR description :>

compiler-errors · 2023-08-21T23:01:55Z

@rfcbot fcp cancel

rfcbot · 2023-08-21T23:01:56Z

@compiler-errors proposal cancelled.

compiler-errors · 2023-08-21T23:02:14Z

@rfcbot fcp merge

see PR description :>

rfcbot · 2023-08-21T23:02:15Z

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.

compiler-errors · 2023-09-19T05:12:37Z

@bors r-

compiler-errors · 2023-09-19T05:44:04Z

@bors r=lcnr

bors · 2023-09-19T05:44:05Z

📌 Commit a30ad3a has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-09-19T05:51:03Z

⌛ Testing commit a30ad3a with merge 39efb1c...

…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 · 2023-09-19T07:37:13Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 39efb1c to master...

bors · 2023-09-19T07:38:14Z

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

oli-obk · 2023-09-19T08:27:48Z

@bors retry

rust-timer · 2023-09-19T09:00:21Z

Finished benchmarking commit (39efb1c): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -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)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.6%	[-1.6%, -1.6%]	1
All ❌✅ (primary)	-	-	0

Binary size

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)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.0%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.1%	[-0.1%, -0.0%]	3

Bootstrap: 633.02s -> 632.908s (-0.02%)
Artifact size: 317.91 MiB -> 317.94 MiB (0.01%)

…llaumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#112725 (rustdoc-search: add support for type parameters) - rust-lang#114941 (Don't resolve generic impls that may be shadowed by dyn built-in impls) - rust-lang#115625 (Explain HRTB infer limitations of old solver) - rust-lang#115839 (Bump libc to 0.2.148) - rust-lang#115924 (Don't complain on a single non-exhaustive 1-ZST) - rust-lang#118636 (panic when encountering an illegal cpumask in thread::available_parallelism) r? `@ghost` `@rustbot` modify labels: rollup

RalfJung · 2023-09-19T11:23:33Z

compiler/rustc_middle/src/ty/sty.rs

@@ -2945,6 2945,33 @@ impl<'tcx> Ty<'tcx> {
 _ => false,
 }
 }
+
+ pub fn is_known_rigid(self) -> bool {


Would be good to have a doc comment explaining what "rigid" means.

For instance, (_, _) would be considered rigid by this check, right?

Yeah, I can add this.

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

…Jung Add note to `is_known_rigid` Adds a note requested by `@RalfJung` in rust-lang#114941 (comment) Let me know if there are any other fns that need documentation, I could throw them into this PR too :)

Rollup merge of rust-lang#116041 - compiler-errors:rigid-note, r=RalfJung Add note to `is_known_rigid` Adds a note requested by `@RalfJung` in rust-lang#114941 (comment) Let me know if there are any other fns that need documentation, I could throw them into this PR too :)

Pkgsrc changes: * Remove NetBSD-8 support (embedded LLVm requires newer C than what is in -8; it's conceivable that this could still build with an external LLVM) * undo powerpc 9.0 file naming tweak, since we no longer support -8. * Remove patch to LLVM for powerpc now included by upstream. * Minor adjustments, checksum changes etc. Upstream changes: Version 1.74.1 (2023-12-07) =========================== - [Resolved spurious STATUS_ACCESS_VIOLATIONs in LLVM] (rust-lang/rust#118464) - [Clarify guarantees for std::mem::discriminant] (rust-lang/rust#118006) - [Fix some subtyping-related regressions] (rust-lang/rust#116415) Version 1.74.0 (2023-11-16) ========================== Language -------- - [Codify that `std::mem::Discriminant<T>` does not depend on any lifetimes in T] (rust-lang/rust#104299) - [Replace `private_in_public` lint with `private_interfaces` and `private_bounds` per RFC 2145] (rust-lang/rust#113126) Read more in [RFC 2145](https://rust-lang.github.io/rfcs/2145-type-privacy.html). - [Allow explicit `#[repr(Rust)]`] (rust-lang/rust#114201) - [closure field capturing: don't depend on alignment of packed fields] (rust-lang/rust#115315) - [Enable MIR-based drop-tracking for `async` blocks] (rust-lang/rust#107421) Compiler -------- - [stabilize combining bundle and whole-archive link modifiers] (rust-lang/rust#113301) - [Stabilize `PATH` option for `--print KIND=PATH`] (rust-lang/rust#114183) - [Enable ASAN/LSAN/TSAN for `*-apple-ios-macabi`] (rust-lang/rust#115644) - [Promote loongarch64-unknown-none* to Tier 2] (rust-lang/rust#115368) - [Add `i686-pc-windows-gnullvm` as a tier 3 target] (rust-lang/rust#115687) Libraries --------- - [Implement `From<OwnedFd/Handle>` for ChildStdin/out/err] (rust-lang/rust#98704) - [Implement `From<{&,&mut} [T; N]>` for `Vec<T>` where `T: Clone`] (rust-lang/rust#111278) - [impl Step for IP addresses] (rust-lang/rust#113748) - [Implement `From<[T; N]>` for `Rc<[T]>` and `Arc<[T]>`] (rust-lang/rust#114041) - [`impl TryFrom<char> for u16`] (rust-lang/rust#114065) - [Stabilize `io_error_other` feature] (rust-lang/rust#115453) - [Stabilize the `Saturating` type] (rust-lang/rust#115477) - [Stabilize const_transmute_copy] (rust-lang/rust#115520) Stabilized APIs --------------- - [`core::num::Saturating`] (https://doc.rust-lang.org/stable/std/num/struct.Saturating.html) - [`impl From<io::Stdout> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From-for-Stdio) - [`impl From<io::Stderr> for std::process::Stdio`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From-for-Stdio) - [`impl From<OwnedHandle> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From-for-Stdio) - [`impl From<OwnedFd> for std::process::Child{Stdin, Stdout, Stderr}`] (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From-for-Stdio) - [`std::ffi::OsString::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsString::into_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsString.html#method.into_encoded_bytes) - [`std::ffi::OsStr::from_encoded_bytes_unchecked`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.from_encoded_bytes_unchecked) - [`std::ffi::OsStr::as_encoded_bytes`] (https://doc.rust-lang.org/stable/std/ffi/struct.OsStr.html#method.as_encoded_bytes) - [`std::io::Error::other`] (https://doc.rust-lang.org/stable/std/io/struct.Error.html#method.other) - [`impl TryFrom<char> for u16`] (https://doc.rust-lang.org/stable/std/primitive.u16.html#impl-TryFrom-for-u16) - [`impl<T: Clone, const N: usize> From<&[T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From<&[T; N]>-for-Vec) - [`impl<T: Clone, const N: usize> From<&mut [T; N]> for Vec<T>`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#impl-From<&mut [T; N]>-for-Vec) - [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`] (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From<[T; N]>-for-Arc<[T], Global>) - [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`] (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From<[T; N]>-for-Rc<[T], Global>) These APIs are now stable in const contexts: - [`core::mem::transmute_copy`] (https://doc.rust-lang.org/beta/std/mem/fn.transmute_copy.html) - [`str::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.str.html#method.is_ascii) - [`[u8]::is_ascii`] (https://doc.rust-lang.org/beta/std/primitive.slice.html#method.is_ascii) Cargo ----- - [fix: Set MSRV for internal packages] (rust-lang/cargo#12381) - [config: merge lists in precedence order] (rust-lang/cargo#12515) - [fix(update): Clarify meaning of --aggressive as --recursive] (rust-lang/cargo#12544) - [fix(update): Make `-p` more convenient by being positional] (rust-lang/cargo#12545) - [feat(help): Add styling to help output ] (rust-lang/cargo#12578) - [feat(pkgid): Allow incomplete versions when unambigious] (rust-lang/cargo#12614) - [feat: stabilize credential-process and registry-auth] (rust-lang/cargo#12649) - [feat(cli): Add '-n' to dry-run] (rust-lang/cargo#12660) - [Add support for `target.'cfg(..)'.linker`] (rust-lang/cargo#12535) - [Stabilize `--keep-going`] (rust-lang/cargo#12568) - [feat: Stabilize lints] (rust-lang/cargo#12648) Rustdoc ------- - [Add warning block support in rustdoc] (rust-lang/rust#106561) - [Accept additional user-defined syntax classes in fenced code blocks] (rust-lang/rust#110800) - [rustdoc-search: add support for type parameters] (rust-lang/rust#112725) - [rustdoc: show inner enum and struct in type definition for concrete type] (rust-lang/rust#114855) Compatibility Notes ------------------- - [Raise minimum supported Apple OS versions] (rust-lang/rust#104385) - [make Cell::swap panic if the Cells partially overlap] (rust-lang/rust#114795) - [Reject invalid crate names in `--extern`] (rust-lang/rust#116001) - [Don't resolve generic impls that may be shadowed by dyn built-in impls] (rust-lang/rust#114941) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. None this cycle.

rustbot assigned cjgillot Aug 17, 2023

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 Aug 17, 2023

compiler-errors commented Aug 17, 2023

View reviewed changes

lcnr reviewed Aug 17, 2023

View reviewed changes

compiler/rustc_mir_transform/src/inline.rs Outdated Show resolved Hide resolved

rustbot assigned lcnr and unassigned cjgillot Aug 18, 2023

compiler-errors changed the title ~~Don't inline impls that may be shadowed by dyn built-in impls~~ Don't resolve generic impls that may be shadowed by dyn built-in impls Aug 21, 2023

rust-cloud-vms bot force-pushed the inline-shadowed-by-dyn branch 2 times, most recently from be4fc7b to e8f9d94 Compare August 21, 2023 22:59

This comment was marked as outdated.

Sign in to view

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 Aug 21, 2023

rfcbot removed 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 Aug 21, 2023

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 Aug 21, 2023

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 Aug 21, 2023

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 Sep 19, 2023

This comment has been minimized.

Sign in to view

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 Sep 19, 2023

Don't resolve generic instances if they may be shadowed by dyn

a30ad3a

rust-cloud-vms bot force-pushed the inline-shadowed-by-dyn branch from 4209263 to a30ad3a Compare September 19, 2023 05:42

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 Sep 19, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2023

GuillaumeGomez mentioned this pull request Sep 19, 2023

Rollup of 6 pull requests #115960

Merged

RalfJung reviewed Sep 19, 2023

View reviewed changes

bors merged commit 66b7bdf into rust-lang:master Sep 19, 2023
11 of 12 checks passed

rustbot added this to the 1.74.0 milestone Sep 19, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Sep 21, 2023

compiler-errors mentioned this pull request Sep 21, 2023

Add note to is_known_rigid #116041

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

compiler-errors commented Aug 17, 2023 •

edited

Loading

rustbot commented Aug 17, 2023

compiler-errors Aug 17, 2023

lcnr left a comment •

edited

Loading

lcnr commented Aug 18, 2023

lcnr commented Aug 18, 2023

compiler-errors commented Aug 21, 2023

This comment was marked as outdated.

compiler-errors commented Aug 21, 2023

rfcbot commented Aug 21, 2023

compiler-errors commented Aug 21, 2023

rfcbot commented Aug 21, 2023 •

edited

Loading

This comment has been minimized.

compiler-errors commented Sep 19, 2023

compiler-errors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

oli-obk commented Sep 19, 2023

rust-timer commented Sep 19, 2023

RalfJung Sep 19, 2023

compiler-errors Sep 19, 2023

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

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

Conversation

compiler-errors commented Aug 17, 2023 • edited Loading

rustbot commented Aug 17, 2023

compiler-errors Aug 17, 2023

Choose a reason for hiding this comment

lcnr left a comment • edited Loading

Choose a reason for hiding this comment

lcnr commented Aug 18, 2023

lcnr commented Aug 18, 2023

compiler-errors commented Aug 21, 2023

This comment was marked as outdated.

compiler-errors commented Aug 21, 2023

rfcbot commented Aug 21, 2023

compiler-errors commented Aug 21, 2023

rfcbot commented Aug 21, 2023 • edited Loading

This comment has been minimized.

compiler-errors commented Sep 19, 2023

compiler-errors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

bors commented Sep 19, 2023

oli-obk commented Sep 19, 2023

rust-timer commented Sep 19, 2023

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

RalfJung Sep 19, 2023

Choose a reason for hiding this comment

compiler-errors Sep 19, 2023

Choose a reason for hiding this comment

compiler-errors commented Aug 17, 2023 •

edited

Loading

lcnr left a comment •

edited

Loading

rfcbot commented Aug 21, 2023 •

edited

Loading