Add missing normalization for union fields types #106938

GuillaumeGomez · 2023-01-16T14:21:01Z

Overshadows #106808.
From the experiment #103985.

In short, it allows to use projections as a type for union's fields.

cc @compiler-errors
r? @oli-obk

compiler-errors

We already check union fields' copy implementations (and thus their validity) modulo regions, so this is fine imo.

Fixing that seems orthogonal and probably needs a more involved fix.

oli-obk · 2023-01-16T14:37:51Z

This PR allows

pub trait Identity {
    type Identity;
}

impl<T> Identity for T {
    type Identity = Self;
}

pub type Foo = u8;

pub union Bar {
    pub a: <Foo as Identity>::Identity,
    pub b: u8,
}

to compile. Before this PR, we did not allow projections in union fields' types, as we could not prove that <Foo as Identity>::Identity: Copy without normalizing, which we are doing now. The : Copy check was already being done by ignoring regions, so erasing them doesn't change anything in that regard. Note that this does not affect projections that can't be normalized, only ones that are fully known.

@rfcbot merge

rfcbot · 2023-01-16T14:37:53Z

Team member @oli-obk 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.

lcnr · 2023-01-16T14:45:10Z

compiler/rustc_hir_analysis/src/check/check.rs

@@ -121,7 +121,7 @@ fn check_union_fields(tcx: TyCtxt<'_>, span: Span, item_def_id: LocalDefId) -> b

        let param_env = tcx.param_env(item_def_id);
        for field in &def.non_enum_variant().fields {
-            let field_ty = field.ty(tcx, substs);
+            let field_ty = tcx.normalize_erasing_regions(param_env, field.ty(tcx, substs));


this would ICE if the union has fields which aren't well formed, e.g. something like <T as NotImplemented>::Assoc. Please add a test for this and check whether it currently causes an ICE. If so you can replace this with try_normalize_erasing_regions. I gues it's fine for normalization to ignore region obligations as we should detect these region errors in WF check.

While this also ignore

If you mean:

pub trait Identity { type Identity; } /*impl<T> Identity for T { type Identity = Self; }*/ pub type Foo = u8; pub union Bar { a: <Foo as Identity>::Identity, b: u8, }

Then no problem. I'll add a ui test for it though.

not sure when exactly normalize_erasing_regions ends up failing with an ICE, only know that it does. Maybe also from something like

trait Identity { type Identity; } trait NotImplemented {} impl<T: NotImplemented> Identity for T { type Identity = Self; } type Foo = u8; union Bar { a: <Foo as Identity>::Identity, b: u8, }

Working too, adding it as a test.

hmm, alright. If there's a way to ICE here we will just get a bug report, so that seems fine

dis you mean #107872 ?

That didn't take long. 🤣

Showed up as new in my daily ICE-screening ^^

GuillaumeGomez · 2023-01-16T17:10:37Z

Added the test suggested by @lcnr.

GuillaumeGomez · 2023-01-16T21:32:02Z

And fixed invalid location of the ui test I added.

GuillaumeGomez · 2023-01-17T13:01:13Z

Added the new UI test suggested by @lcnr.

rfcbot · 2023-01-17T21:13:13Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2023-01-27T21:13:14Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

oli-obk · 2023-02-08T15:50:46Z

@bors r+

bors · 2023-02-08T15:50:50Z

📌 Commit 6c63b94 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-02-09T07:08:23Z

⌛ Testing commit 6c63b94 with merge c40919b...

bors · 2023-02-09T09:58:13Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing c40919b to master...

rust-timer · 2023-02-09T11:18:03Z

Finished benchmarking commit (c40919b): 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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.1%	[-3.1%, -3.1%]	1
Improvements ✅ (secondary)	-2.4%	[-3.0%, -0.8%]	14
All ❌✅ (primary)	-3.1%	[-3.1%, -3.1%]	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)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. * Sadly, the patch to reduce the cargo verbosity no longer applies, so I've asked upstream about the proper way to get the old result. (so the build log becomes Quite Bloated for now). Upstream changes: Version 1.69.0 (2023-04-20) ========================== Language -------- - [Deriving built-in traits on packed structs works with `Copy` fields.] (rust-lang/rust#104429) - [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.] (rust-lang/rust#106774) - [Improve analysis of trait bounds for associated types.] (rust-lang/rust#103695) - [Allow associated types to be used as union fields.] (rust-lang/rust#106938) - [Allow `Self: Autotrait` bounds on dyn-safe trait methods.] (rust-lang/rust#107082) - [Treat `str` as containing `[u8]` for auto trait purposes.] (rust-lang/rust#107941) Compiler -------- - [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.] (rust-lang/rust#100178) - [Rework min_choice algorithm of member constraints.] (rust-lang/rust#105300) - [Support `true` and `false` as boolean flags in compiler arguments.] (rust-lang/rust#107043) - [Default `repr(C)` enums to `c_int` size.] (rust-lang/rust#107592) Libraries --------- - [Implement the unstable `DispatchFromDyn` for cell types, allowing downstream experimentation with custom method receivers.] (rust-lang/rust#97373) - [Document that `fmt::Arguments::as_str()` may return `Some(_)` in more cases after optimization, subject to change.] (rust-lang/rust#106823) - [Implement `AsFd` and `AsRawFd` for `Rc`.] (rust-lang/rust#107317) Stabilized APIs --------------- - [`CStr::from_bytes_until_nul`] (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul) - [`core::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html) These APIs are now stable in const contexts: - [`SocketAddr::new`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new) - [`SocketAddr::ip`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip) - [`SocketAddr::port`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port) - [`SocketAddr::is_ipv4`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4) - [`SocketAddr::is_ipv6`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6) - [`SocketAddrV4::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new) - [`SocketAddrV4::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip) - [`SocketAddrV4::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port) - [`SocketAddrV6::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new) - [`SocketAddrV6::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip) - [`SocketAddrV6::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port) - [`SocketAddrV6::flowinfo`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo) - [`SocketAddrV6::scope_id`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id) Cargo ----- - [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.] (rust-lang/cargo#11558) - [Cargo now suggests `cargo add` if you try to install a library crate.] (rust-lang/cargo#11410) - [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.] (rust-lang/cargo#11705) Rustdoc ----- - [Vertically compact trait bound formatting.] (rust-lang/rust#102842) - [Only include stable lints in `rustdoc::all` group.] (rust-lang/rust#106316) - [Compute maximum Levenshtein distance based on the query.] (rust-lang/rust#107141) - [Remove inconsistently-present sidebar tooltips.] (rust-lang/rust#107490) - [Search by macro when query ends with `!`.] (rust-lang/rust#108143) Compatibility Notes ------------------- - [The `rust-analysis` component from `rustup` now only contains a warning placeholder.] (rust-lang/rust#101841) This was primarily intended for RLS, and the corresponding `-Zsave-analysis` flag has been removed from the compiler as well. - [Unaligned references to packed fields are now a hard error.] (rust-lang/rust#102513) This has been a warning since 1.53, and denied by default with a future-compatibility warning since 1.62. - [Update the minimum external LLVM to 14.] (rust-lang/rust#107573) - [Cargo now emits errors on invalid characters in a registry token.] (rust-lang/cargo#11600) - [When `default-features` is set to false of a workspace dependency, and an inherited dependency of a member has `default-features = true`, Cargo will enable default features of that dependency.] (rust-lang/cargo#11409) - [Cargo denies `CARGO_HOME` in the `[env]` configuration table. Cargo itself doesn't pick up this value, but recursive calls to cargo would, which was not intended.] (rust-lang/cargo#11644) - [Debuginfo for build dependencies is now off if not explicitly set. This is expected to improve the overall build time.] (rust-lang/cargo#11252) 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. - [Move `format_args!()` into AST (and expand it during AST lowering)] (rust-lang/rust#106745)

Pkgsrc changes: * Adjust patches and cargo checksums to new versions. Upstream changes: Version 1.69.0 (2023-04-20) ========================== Language -------- - [Deriving built-in traits on packed structs works with `Copy` fields.] (rust-lang/rust#104429) - [Stabilize the `cmpxchg16b` target feature on x86 and x86_64.] (rust-lang/rust#106774) - [Improve analysis of trait bounds for associated types.] (rust-lang/rust#103695) - [Allow associated types to be used as union fields.] (rust-lang/rust#106938) - [Allow `Self: Autotrait` bounds on dyn-safe trait methods.] (rust-lang/rust#107082) - [Treat `str` as containing `[u8]` for auto trait purposes.] (rust-lang/rust#107941) Compiler -------- - [Upgrade `*-pc-windows-gnu` on CI to mingw-w64 v10 and GCC 12.2.] (rust-lang/rust#100178) - [Rework min_choice algorithm of member constraints.] (rust-lang/rust#105300) - [Support `true` and `false` as boolean flags in compiler arguments.] (rust-lang/rust#107043) - [Default `repr(C)` enums to `c_int` size.] (rust-lang/rust#107592) Libraries --------- - [Implement the unstable `DispatchFromDyn` for cell types, allowing downstream experimentation with custom method receivers.] (rust-lang/rust#97373) - [Document that `fmt::Arguments::as_str()` may return `Some(_)` in more cases after optimization, subject to change.] (rust-lang/rust#106823) - [Implement `AsFd` and `AsRawFd` for `Rc`.] (rust-lang/rust#107317) Stabilized APIs --------------- - [`CStr::from_bytes_until_nul`] (https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_bytes_until_nul) - [`core::ffi::FromBytesUntilNulError`] (https://doc.rust-lang.org/stable/core/ffi/struct.FromBytesUntilNulError.html) These APIs are now stable in const contexts: - [`SocketAddr::new`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.new) - [`SocketAddr::ip`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.ip) - [`SocketAddr::port`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.port) - [`SocketAddr::is_ipv4`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv4) - [`SocketAddr::is_ipv6`] (https://doc.rust-lang.org/stable/std/net/enum.SocketAddr.html#method.is_ipv6) - [`SocketAddrV4::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.new) - [`SocketAddrV4::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.ip) - [`SocketAddrV4::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV4.html#method.port) - [`SocketAddrV6::new`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.new) - [`SocketAddrV6::ip`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.ip) - [`SocketAddrV6::port`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.port) - [`SocketAddrV6::flowinfo`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.flowinfo) - [`SocketAddrV6::scope_id`] (https://doc.rust-lang.org/stable/std/net/struct.SocketAddrV6.html#method.scope_id) Cargo ----- - [Cargo now suggests `cargo fix` or `cargo clippy --fix` when compilation warnings are auto-fixable.] (rust-lang/cargo#11558) - [Cargo now suggests `cargo add` if you try to install a library crate.] (rust-lang/cargo#11410) - [Cargo now sets the `CARGO_BIN_NAME` environment variable also for binary examples.] (rust-lang/cargo#11705) Rustdoc ----- - [Vertically compact trait bound formatting.] (rust-lang/rust#102842) - [Only include stable lints in `rustdoc::all` group.] (rust-lang/rust#106316) - [Compute maximum Levenshtein distance based on the query.] (rust-lang/rust#107141) - [Remove inconsistently-present sidebar tooltips.] (rust-lang/rust#107490) - [Search by macro when query ends with `!`.] (rust-lang/rust#108143) Compatibility Notes ------------------- - [The `rust-analysis` component from `rustup` now only contains a warning placeholder.] (rust-lang/rust#101841) This was primarily intended for RLS, and the corresponding `-Zsave-analysis` flag has been removed from the compiler as well. - [Unaligned references to packed fields are now a hard error.] (rust-lang/rust#102513) This has been a warning since 1.53, and denied by default with a future-compatibility warning since 1.62. - [Update the minimum external LLVM to 14.] (rust-lang/rust#107573) - [Cargo now emits errors on invalid characters in a registry token.] (rust-lang/cargo#11600) - [When `default-features` is set to false of a workspace dependency, and an inherited dependency of a member has `default-features = true`, Cargo will enable default features of that dependency.] (rust-lang/cargo#11409) - [Cargo denies `CARGO_HOME` in the `[env]` configuration table. Cargo itself doesn't pick up this value, but recursive calls to cargo would, which was not intended.] (rust-lang/cargo#11644) - [Debuginfo for build dependencies is now off if not explicitly set. This is expected to improve the overall build time.] (rust-lang/cargo#11252) 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. - [Move `format_args!()` into AST (and expand it during AST lowering)] (rust-lang/rust#106745)

GuillaumeGomez added 2 commits January 16, 2023 15:18

Add missing normalization for union fields types

921efd2

Add ui test for projection used as union field type

4653bbf

rustbot assigned oli-obk Jan 16, 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 Jan 16, 2023

compiler-errors approved these changes Jan 16, 2023

View reviewed changes

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 Jan 16, 2023

This comment was marked as resolved.

Sign in to view

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 Jan 16, 2023

oli-obk 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 Jan 16, 2023

rust-lang deleted a comment from rfcbot Jan 16, 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 Jan 16, 2023

lcnr reviewed Jan 16, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

GuillaumeGomez force-pushed the normalize-projection-field-ty branch from da3c342 to 0bc2ae4 Compare January 16, 2023 17:35

Add failing test for invalid projection as union field type

6c63b94

GuillaumeGomez force-pushed the normalize-projection-field-ty branch from 0bc2ae4 to 6c63b94 Compare January 17, 2023 13:01

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 17, 2023

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jan 27, 2023

rfcbot added the to-announce Announce this issue on triage meeting label Jan 27, 2023

apiraino removed the to-announce Announce this issue on triage meeting label Feb 3, 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2023

bors merged commit c40919b into rust-lang:master Feb 9, 2023

rustbot added this to the 1.69.0 milestone Feb 9, 2023

GuillaumeGomez deleted the normalize-projection-field-ty branch February 9, 2023 09:59

matthiaskrgr mentioned this pull request Feb 10, 2023

rustdoc: failed to normalize #107872

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing normalization for union fields types #106938

Add missing normalization for union fields types #106938

GuillaumeGomez commented Jan 16, 2023 •

edited

compiler-errors left a comment

This comment was marked as resolved.

This comment was marked as resolved.

oli-obk commented Jan 16, 2023

rfcbot commented Jan 16, 2023 •

edited by spastorino

lcnr Jan 16, 2023

GuillaumeGomez Jan 16, 2023

lcnr Jan 17, 2023

GuillaumeGomez Jan 17, 2023

lcnr Jan 17, 2023

lqd Feb 10, 2023

compiler-errors Feb 10, 2023

GuillaumeGomez Feb 10, 2023

matthiaskrgr Feb 10, 2023

GuillaumeGomez Feb 10, 2023

GuillaumeGomez commented Jan 16, 2023

This comment has been minimized.

GuillaumeGomez commented Jan 16, 2023

GuillaumeGomez commented Jan 17, 2023

rfcbot commented Jan 17, 2023

rfcbot commented Jan 27, 2023

oli-obk commented Feb 8, 2023

bors commented Feb 8, 2023

bors commented Feb 9, 2023

bors commented Feb 9, 2023

rust-timer commented Feb 9, 2023

Add missing normalization for union fields types #106938

Add missing normalization for union fields types #106938

Conversation

GuillaumeGomez commented Jan 16, 2023 • edited

compiler-errors left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

oli-obk commented Jan 16, 2023

rfcbot commented Jan 16, 2023 • edited by spastorino

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 16, 2023

This comment has been minimized.

GuillaumeGomez commented Jan 16, 2023

GuillaumeGomez commented Jan 17, 2023

rfcbot commented Jan 17, 2023

rfcbot commented Jan 27, 2023

oli-obk commented Feb 8, 2023

bors commented Feb 8, 2023

bors commented Feb 9, 2023

bors commented Feb 9, 2023

rust-timer commented Feb 9, 2023

Overall result: no relevant changes - no action needed

GuillaumeGomez commented Jan 16, 2023 •

edited

rfcbot commented Jan 16, 2023 •

edited by spastorino