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

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

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Aug 17, 2023

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 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
@rustbot
Copy link
Collaborator

rustbot commented Aug 17, 2023

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

_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];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2023

r? @lcnr

@rustbot rustbot assigned lcnr and unassigned cjgillot Aug 18, 2023
@lcnr
Copy link
Contributor

lcnr commented Aug 18, 2023

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.

@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 18, 2023
@compiler-errors 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 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
@compiler-errors
Copy link
Member Author

@rfcbot fcp merge

See PR description :>

@rfcbot

This comment was marked as outdated.

@rfcbot 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
@compiler-errors
Copy link
Member Author

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Aug 21, 2023

@compiler-errors proposal cancelled.

@rfcbot 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 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
@compiler-errors
Copy link
Member Author

@rfcbot fcp merge

see PR description :>

@rfcbot
Copy link

rfcbot commented Aug 21, 2023

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.

Sorry, something went wrong.

@rfcbot 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 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
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

@bors r-

@bors 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
@rust-cloud-vms rust-cloud-vms bot force-pushed the inline-shadowed-by-dyn branch from 4209263 to a30ad3a Compare September 19, 2023 05:42
@compiler-errors
Copy link
Member Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Sep 19, 2023

📌 Commit a30ad3a has been approved by lcnr

It is now in the queue for this repository.

@bors 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
Copy link
Contributor

bors commented Sep 19, 2023

⌛ Testing commit a30ad3a with merge 39efb1c...

bors added a commit to rust-lang-ci/rust that referenced this pull request 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
Copy link
Contributor

bors commented Sep 19, 2023

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

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

bors commented Sep 19, 2023

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

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2023

@bors retry

@rust-timer
Copy link
Collaborator

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%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023
…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#115946 (panic when encountering an illegal cpumask in thread::available_parallelism)

r? `@ghost`
`@rustbot` modify labels: rollup
@@ -2945,6 +2945,33 @@ impl<'tcx> Ty<'tcx> {
_ => false,
}
}

pub fn is_known_rigid(self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can add this.

@bors bors merged commit 66b7bdf into rust-lang:master Sep 19, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 19, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 21, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…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 :)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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 :)
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 8, 2024
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%3CStdout%3E-for-Stdio)
- [`impl From<io::Stderr> for std::process::Stdio`]
  (https://doc.rust-lang.org/stable/std/process/struct.Stdio.html#impl-From%3CStderr%3E-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%3CStderr%3E-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%3CStderr%3E-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%3Cchar%3E-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%3C%26%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`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%3C%26mut+%5BT;+N%5D%3E-for-Vec%3CT,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Arc<[T]>`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Arc.html#impl-From%3C%5BT;+N%5D%3E-for-Arc%3C%5BT%5D,+Global%3E)
- [`impl<T, const N: usize> From<[T; N]> for Rc<[T]>`]
  (https://doc.rust-lang.org/stable/std/rc/struct.Rc.html#impl-From%3C%5BT;+N%5D%3E-for-Rc%3C%5BT%5D,+Global%3E)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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