Double check that hidden types match the expected hidden type #113661

oli-obk · 2023-07-13T16:26:29Z

Fixes #113278 specifically, but I left a TODO for where we should also add some hardening.

It feels a bit like papering over the issue, but at least this way we don't get unsoundness, but just surprising errors. Errors will be improved and given spans before this PR lands.

r? @compiler-errors @lcnr

rustbot · 2023-07-13T16:26:38Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2023-07-13T16:26:42Z

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

compiler/rustc_hir_analysis/src/check/check.rs

lcnr · 2023-07-17T09:10:11Z

compiler/rustc_const_eval/src/util/compare_types.rs

+            "{}, {}",
+            tcx.type_of(key.def_id).subst(tcx, key.substs),
+            ty.hidden_type.ty
+        );


I feel like we should hit this bug? 🤔 or do we avoid it because we have OpaqueCast everywhere we would assign to/from opaques?

I was surprised, too, but I didn't debug it.

do we run the validator before optimizations by default 😅 iirc we only do it with -Zvalidate-mir 🤔

compiler/rustc_hir_analysis/src/check/check.rs

tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs

compiler/rustc_hir_analysis/src/check/check.rs

lcnr · 2023-07-24T15:01:58Z

compiler/rustc_hir_analysis/src/check/check.rs

+/// // desugared to
+///
+/// // hidden type is `&'bDup ()`
+/// // During wfcheck the hidden type of `Inner` is `&'a ()`, but


Suggested change

/// // During wfcheck the hidden type of `Inner` is `&'a ()`, but

/// // During wfcheck the hidden type of `Inner` is `&'b ()`, but

i feel like it is a bit weird to talk about 'bDup if it is unclear where 'b comes from. I prefer using 'a and 'aDup everywhere

well... unless I double confused myself, your diff is wrong, so it's good that I made it explicit 😆

how can the inner type in wf check be 'a if we wf check in the scope of the opaque type which does not have a 'a in scope? 😅

also, "During wfcheck of Outer ...", would make this clearer I think

because RPITs aren't desugerable currently (ok I guess that's what you meant by "unclear where 'b comes from"), they only work by construction and are cheating. I rewrote the example to be more like how it actually works, is that clearer?

lcnr

r=me after final comment nits

lcnr

accurately documenting the desugaring is messy (because the desugaring is messy :3)

because RPITs aren't desugerable currently (ok I guess that's what you meant by "unclear where 'b comes from"), they only work by construction and are cheating. I rewrote the example to be more like how it actually works, is that clearer?

idk 😅 i don't know how much of my nits are "the actual impl is a mess" and how much is "the comment is wrong/incomplete"

lcnr · 2023-07-25T07:55:35Z

compiler/rustc_hir_analysis/src/check/check.rs

@@ -545,17 +545,23 @@ fn sanity_check_found_hidden_type<'tcx>(
 /// }
 /// fn func<'a>(x: &'a ()) -> impl Id<Assoc = impl Sized + 'a> { x }
 /// // desugared to
+/// fn func<'a>(x: &'a () -> Outer<'a, Assoc = Inner<'a>> {


Suggested change

/// fn func<'a>(x: &'a () -> Outer<'a, Assoc = Inner<'a>> {

/// fn func<'a>(x: &'a () -> Outer<'a, 'a> {

? 🤔

yea... I don't know how to represent the fact that we walk the item bounds of Outer

lcnr · 2023-07-25T07:55:59Z

compiler/rustc_hir_analysis/src/check/check.rs

-/// // hidden type is `&'cDup ()`
-/// type Inner<'c, 'cDup> = impl Sized + 'cDup;
-/// fn func<'a>(x: &'a () -> Outer<'static, 'a> { x }
+///     // hidden type is `&'aDupOuter ()`


This feels wrong, if the hidden type of Outer is &'aDupOuter (), shouldn't wf check of Outer define the hidden type of Inner to be <&'aDupOuter () as Id>::Assoc which is &'aDupOuter (), so this should worK?

No, because we figure out the hidden type of Inner anew during wf check of Outer, we don't get it from type_of. We get the hidden type of Outer, which is &'a (), so the id projection is also &'a ()

lcnr · 2023-07-25T07:56:21Z

compiler/rustc_hir_analysis/src/check/check.rs

+///     // So we walk the signature of `func` to find the use of `Inner<'a>`
+///     // and then use that to replace the lifetimes in the hidden type, obtaining
+///     // `&'a ()`.
+///     type Outer<'aDupOuter> = impl Id<Assoc = Inner<'aDupOuter>>;


is the 'a param of Inner 'a or 'aDupOuter here?

'a, it is inherited from the function.

oli-obk · 2023-07-25T13:43:38Z

@bors r=lcnr

bors · 2023-07-25T13:43:40Z

📌 Commit df4bfd9 has been approved by lcnr

It is now in the queue for this repository.

bors · 2023-07-25T16:18:28Z

⌛ Testing commit df4bfd9 with merge 50bb51b2201e3aa6ca651a9446f62088ca32d9f0...

bors · 2023-07-25T16:53:00Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-07-25T16:54:02Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

oli-obk · 2023-07-25T17:04:31Z

@bors retry nothing failed? Not sure what happened

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#112995 (Check for `<&NotClone as Clone>::clone()` calls and suggest to add Clone trait appropriately) - rust-lang#113578 (Don't say that a type is uncallable if its fn signature has errors in it) - rust-lang#113661 (Double check that hidden types match the expected hidden type) - rust-lang#114044 (factor out more stable impls) - rust-lang#114062 (CI: split nested GHA groups instead of panicking) r? `@ghost` `@rustbot` modify labels: rollup

… r=jackh726 Paper over an accidental regression r? types cc rust-lang#115781 (do not close issue until beta backport has been performed) The PR reasons are explained with comments in the source. In order to keep the diff simple, this PR effectively reverts rust-lang#113661, but only for RPITs. I will submit a follow up PR that fixes this correctly instead of just disabling the newly added check for RPITs. This PR should be significantly easier to review for beta backport

Rollup merge of rust-lang#115844 - oli-obk:opaque_lifetime_ambiguity, r=jackh726 Paper over an accidental regression r? types cc rust-lang#115781 (do not close issue until beta backport has been performed) The PR reasons are explained with comments in the source. In order to keep the diff simple, this PR effectively reverts rust-lang#113661, but only for RPITs. I will submit a follow up PR that fixes this correctly instead of just disabling the newly added check for RPITs. This PR should be significantly easier to review for beta backport

rustbot assigned compiler-errors Jul 13, 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jul 13, 2023

This comment has been minimized.

Sign in to view

oli-obk force-pushed the tait_wtf branch from 4559df9 to 53d0ecd Compare July 13, 2023 19:50

This comment has been minimized.

Sign in to view

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jul 14, 2023

oli-obk commented Jul 14, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

lcnr reviewed Jul 17, 2023

View reviewed changes

oli-obk force-pushed the tait_wtf branch 2 times, most recently from 92e7e6d to 7c75895 Compare July 20, 2023 09:36

This comment has been minimized.

Sign in to view

oli-obk force-pushed the tait_wtf branch from 7c75895 to 2d0b97c Compare July 20, 2023 14:48

lcnr reviewed Jul 21, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

tests/ui/type-alias-impl-trait/nested-rpit-with-lifetimes.rs Outdated Show resolved Hide resolved

Double check that hidden types match the expected hidden type

44e2150

oli-obk force-pushed the tait_wtf branch from 2d0b97c to 44e2150 Compare July 21, 2023 14:26

lcnr reviewed Jul 24, 2023

View reviewed changes

compiler/rustc_hir_analysis/src/check/check.rs Show resolved Hide resolved

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

Explain what the heck is going on with this lifetime remapping business

10d0ff9

oli-obk force-pushed the tait_wtf branch 2 times, most recently from fa82e44 to 82fa94e Compare July 24, 2023 14:02

This comment has been minimized.

Sign in to view

Some documentation nits

5b4549d

oli-obk force-pushed the tait_wtf branch from 82fa94e to 5b4549d Compare July 24, 2023 14:21

lcnr reviewed Jul 24, 2023

View reviewed changes

Explain RPITs in the way they actually work

30f7878

lcnr reviewed Jul 25, 2023

View reviewed changes

Try explaining where Inner is in the signature better

df4bfd9

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 Jul 25, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 25, 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 Jul 25, 2023

matthiaskrgr mentioned this pull request Jul 25, 2023

Rollup of 5 pull requests #114076

Merged

bors merged commit 0b13deb into rust-lang:master Jul 26, 2023
11 of 12 checks passed

rustbot added this to the 1.73.0 milestone Jul 26, 2023

matthiaskrgr mentioned this pull request Jul 27, 2023

ICE: compiler/rustc_const_eval/src/util/compare_types.rs #114121

Closed

oli-obk deleted the tait_wtf branch July 27, 2023 08:13

lqd mentioned this pull request Jul 31, 2023

ICE: Region parameter out of range when substituting in region #114285

Closed

oli-obk mentioned this pull request Aug 7, 2023

Stop using identity args for RPIT wf checks #114574

Closed

Nashenas88 mentioned this pull request Aug 7, 2023

Two conflicting concrete types for the same opaque type #114597

Closed

apiraino mentioned this pull request Sep 14, 2023

regression: concrete type differs from previous defining opaque type use #115781

Closed

oli-obk mentioned this pull request Sep 14, 2023

Paper over an accidental regression #115844

Merged

aliemjay mentioned this pull request Mar 24, 2024

TAITs with different args incorrectly assumed to be equal #122876

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Double check that hidden types match the expected hidden type #113661

Double check that hidden types match the expected hidden type #113661

oli-obk commented Jul 13, 2023

rustbot commented Jul 13, 2023

rustbot commented Jul 13, 2023

This comment has been minimized.

This comment has been minimized.

lcnr Jul 17, 2023

oli-obk Jul 17, 2023

lcnr Jul 17, 2023

matthiaskrgr Jul 27, 2023

This comment has been minimized.

This comment has been minimized.

lcnr Jul 24, 2023

oli-obk Jul 24, 2023

lcnr Jul 24, 2023 •

edited

oli-obk Jul 24, 2023

lcnr left a comment

lcnr left a comment •

edited

lcnr Jul 25, 2023

oli-obk Jul 25, 2023

lcnr Jul 25, 2023

oli-obk Jul 25, 2023

lcnr Jul 25, 2023

oli-obk Jul 25, 2023

oli-obk commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

rust-log-analyzer commented Jul 25, 2023

oli-obk commented Jul 25, 2023

	/// // During wfcheck the hidden type of `Inner` is `&'a ()`, but
	/// // During wfcheck the hidden type of `Inner` is `&'b ()`, but

	/// fn func<'a>(x: &'a () -> Outer<'a, Assoc = Inner<'a>> {
	/// fn func<'a>(x: &'a () -> Outer<'a, 'a> {

Double check that hidden types match the expected hidden type #113661

Double check that hidden types match the expected hidden type #113661

Conversation

oli-obk commented Jul 13, 2023

rustbot commented Jul 13, 2023

rustbot commented Jul 13, 2023

This comment has been minimized.

This comment has been minimized.

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

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

lcnr left a comment • edited

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

oli-obk commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

bors commented Jul 25, 2023

rust-log-analyzer commented Jul 25, 2023

oli-obk commented Jul 25, 2023

lcnr Jul 24, 2023 •

edited

lcnr left a comment •

edited