expand free alias types in the auto-trait orphan check#157835
Conversation
The auto-trait branch of the orphan check inspected the unexpanded self
type, so an impl whose self type was a free (lazy) type alias was
rejected even when the alias resolved to an otherwise valid nominal
type, e.g. `unsafe impl Sync for Alias {}` with `type Alias = Local;`.
Expand free alias types before classifying the self type, mirroring the
locality check above. Expansion still reveals genuinely problematic self
types (trait objects, opaque types, type parameters), so those keep
being rejected.
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
r? me |
|
Shouldn't affect perf but better be certain: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…nsion, r=<try> expand free alias types in the auto-trait orphan check
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7e108df): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 4.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 1.6%, secondary 22.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 519.155s -> 516.812s (-0.45%) |
|
@fmease, wassup! |
…_alias_expansion, r=fmease expand free alias types in the auto-trait orphan check fixes rust-lang#157756. auto-trait impls are only allowed on nominal types. the check for that looked at the self type as written, without expanding it, so a lazy type alias got rejected even when it pointed straight at a type that's fine on its own: ```rust #![feature(lazy_type_alias)] struct Local; type Alias = Local; unsafe impl Sync for Alias {} // used to be E0321 ``` the locality check right above it already normalizes the self type, so the two were looking at different things. imo the cleanest fix is to expand free aliases first, with the same `expand_free_alias_tys` coherence already calls in a couple of other spots. anything that should still be rejected (trait objects, opaque types, bare type params) survives the expansion and errors like before. one thing worth a second look: `impl_for_weak_alias.rs` used `type Alias = (impl Sized, u8)` and expected an error. idk if that was deliberate, but the alias expands to a tuple, and a tuple is fine for a local auto trait if you write it by hand, so it passes now and i made it check-pass. lmk if i'm missing why it should still be rejected.
Rollup of 18 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #152860 (Port the `without_debuginfo` test from `backtrace-rs` to the testsuite) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)
…_alias_expansion, r=fmease expand free alias types in the auto-trait orphan check fixes rust-lang#157756. auto-trait impls are only allowed on nominal types. the check for that looked at the self type as written, without expanding it, so a lazy type alias got rejected even when it pointed straight at a type that's fine on its own: ```rust #![feature(lazy_type_alias)] struct Local; type Alias = Local; unsafe impl Sync for Alias {} // used to be E0321 ``` the locality check right above it already normalizes the self type, so the two were looking at different things. imo the cleanest fix is to expand free aliases first, with the same `expand_free_alias_tys` coherence already calls in a couple of other spots. anything that should still be rejected (trait objects, opaque types, bare type params) survives the expansion and errors like before. one thing worth a second look: `impl_for_weak_alias.rs` used `type Alias = (impl Sized, u8)` and expected an error. idk if that was deliberate, but the alias expands to a tuple, and a tuple is fine for a local auto trait if you write it by hand, so it passes now and i made it check-pass. lmk if i'm missing why it should still be rejected.
…uwer Rollup of 20 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158377 (add `-Zforce-intrinsic-fallback` flag) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)
…uwer Rollup of 19 pull requests Successful merges: - #158692 (Add release notes for 1.96.1) - #134021 (Implement `IntoIterator` for `[&[mut]] Box<[T; N], A>`) - #155932 (MIR Call terminator: evaluate destination place before arguments) - #155989 (Update `transmute_copy` to ub_checks and `?Sized`) - #156777 (Add -Zautodiff_post_passes flag to limit which llvm passes to run after enzyme to make autodiff tests more robust) - #157151 (JSON target specs: remove 'x86-softfloat' compatibility alias) - #157835 (expand free alias types in the auto-trait orphan check) - #157857 (Stabilize `#[my_macro] mod foo;` (part of `proc_macro_hygiene`)) - #158434 (delegation: refactor AST -> HIR lowering) - #158552 (make some tidy errors around python easier to understand) - #158624 (borrowck: Introduce BlameConstraint::to_obligation_cause_from_path()) - #158704 (Optimize `ArrayChunks::try_rfold` with `DoubleEndedIterator::next_chunk_back`) - #158711 (library: Comment on libtest's dicey internal soundness) - #158751 (rustdoc: Fix crash when trying to inline foreign item which cannot have attributes) - #158539 (Move `SizeHint` and `IoHandle` to `core::io`) - #158659 (refactor the normalization in `coerce_shared_info`) - #158689 (resolver: don't use `Finalize` when resolving visibilities during AST expansion) - #158698 (Update TypeVisitable implementation) - #158706 (Tweaks to MIR building scope API)
Rollup merge of #157835 - Dnreikronos:orphan_auto_trait_free_alias_expansion, r=fmease expand free alias types in the auto-trait orphan check fixes #157756. auto-trait impls are only allowed on nominal types. the check for that looked at the self type as written, without expanding it, so a lazy type alias got rejected even when it pointed straight at a type that's fine on its own: ```rust #![feature(lazy_type_alias)] struct Local; type Alias = Local; unsafe impl Sync for Alias {} // used to be E0321 ``` the locality check right above it already normalizes the self type, so the two were looking at different things. imo the cleanest fix is to expand free aliases first, with the same `expand_free_alias_tys` coherence already calls in a couple of other spots. anything that should still be rejected (trait objects, opaque types, bare type params) survives the expansion and errors like before. one thing worth a second look: `impl_for_weak_alias.rs` used `type Alias = (impl Sized, u8)` and expected an error. idk if that was deliberate, but the alias expands to a tuple, and a tuple is fine for a local auto trait if you write it by hand, so it passes now and i made it check-pass. lmk if i'm missing why it should still be rejected.
fixes #157756.
auto-trait impls are only allowed on nominal types. the check for that looked at the self type as written, without expanding it, so a lazy type alias got rejected even when it pointed straight at a type that's fine on its own:
the locality check right above it already normalizes the self type, so the two were looking at different things. imo the cleanest fix is to expand free aliases first, with the same
expand_free_alias_tyscoherence already calls in a couple of other spots. anything that should still be rejected (trait objects, opaque types, bare type params) survives the expansion and errors like before.one thing worth a second look:
impl_for_weak_alias.rsusedtype Alias = (impl Sized, u8)and expected an error. idk if that was deliberate, but the alias expands to a tuple, and a tuple is fine for a local auto trait if you write it by hand, so it passes now and i made it check-pass. lmk if i'm missing why it should still be rejected.