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

Properly deal with weak alias types as self types of impls #120780

Merged
merged 2 commits into from
Feb 18, 2024

Conversation

fmease
Copy link
Member

@fmease fmease commented Feb 8, 2024

Fixes #114216.
Fixes #116100.

Not super happy about the two ad hoc “normalization” implementations for weak alias types:

  1. In inherent_impls: The “peeling”, normalization to “WHNF”: Semantically that's exactly what we want (neither proper normalization nor shallow normalization would be correct here). Basically a weak alias type is “nominal” (well...^^) if the WHNF is nominal. #97974 followed the same approach.
  2. In constrained_generic_params: Generic parameters are constrained by a weak alias type if the corresp. “normalized” type constrains them (where we only normalize weak alias types not arbitrary ones). Weak alias types are injective if the corresp. “normalized” type is injective.

Both have ad hoc overflow detection mechanisms.

Coherence is handled in #117164.

r? @oli-obk or types

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue. F-lazy_type_alias `#![feature(lazy_type_alias)]` labels Feb 8, 2024
@fmease
Copy link
Member Author

fmease commented Feb 12, 2024

Reminder to myself: Check if the following code leads to a cycle error:

#![feature(lazy_type_alias)]

struct I8<const F: i8>;

type Identity<T: ?Sized> = T;

impl Identity<I8<{ i8::MIN }>> {
    fn foo(&self) {}
}

Guillaume just sent me a document of them that records some issues encountered with the previous pre-ty::Weak approaches. Namely, the code above (ty::Weakty::Projection/#[lang = "identity"]) resulted in a cycle: https://hackmd.io/EI_RO0oKQ9OhssvpMrEDQA#Impl-block-on-a-projection

@fmease
Copy link
Member Author

fmease commented Feb 12, 2024

Update: No, it doesn't lead to cycles. Everything works flawlessly and let _ = I8::<-128>.foo(); is // check-pass.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2024

Check if the following code leads to a cycle error:

yea, the previous approach used full normalization, which causes issues. Just peeling away the weak aliases works around all of that.

Sad that we need these custom normalization routines, but that's kinda the conclusion we always came to when we discussed these cases...

@fmease fmease force-pushed the lta-in-impls branch 2 times, most recently from 3e9a8e5 to 1a0323c Compare February 16, 2024 22:41
@fmease
Copy link
Member Author

fmease commented Feb 17, 2024

Added an ensure_sufficient_stack.

Sad that we need these custom normalization routines, but that's kinda the conclusion we always came to when we discussed these cases...

Yea :/ and I'm pretty sure we need to add more of them if we want to emulate some of the existing behavior of eager type aliases. E.g. for #114220 I bet we don't want to properly normalize either.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Feb 17, 2024

📌 Commit 1a0323c has been approved by oli-obk

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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2024
@@ -0,0 +1,18 @@
// check-pass
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments now need an @

@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2024

@bors r-

ui tests now use //@ headers

@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 Feb 17, 2024
@fmease
Copy link
Member Author

fmease commented Feb 17, 2024

🤦 👍

@fmease
Copy link
Member Author

fmease commented Feb 17, 2024

Let's wait till green to make sure I didn't mess up the new ui_test syntax and Imma bors approve on behalf of you later.

@fmease
Copy link
Member Author

fmease commented Feb 17, 2024

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Feb 17, 2024

📌 Commit fde4556 has been approved by oli-obk

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 Feb 17, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
Properly deal with weak alias types as self types of impls

Fixes rust-lang#114216.
Fixes rust-lang#116100.

Not super happy about the two ad hoc “normalization” implementations for weak alias types:

1. In `inherent_impls`: The “peeling”, normalization to [“WHNF”][whnf]: Semantically that's exactly what we want (neither proper normalization nor shallow normalization would be correct here). Basically a weak alias type is “nominal” (well...^^) if the WHNF is nominal. [rust-lang#97974](rust-lang#97974) followed the same approach.
2. In `constrained_generic_params`: Generic parameters are constrained by a weak alias type if the corresp. “normalized” type constrains them (where we only normalize *weak* alias types not arbitrary ones). Weak alias types are injective if the corresp. “normalized” type is injective.

Both have ad hoc overflow detection mechanisms.

**Coherence** is handled in rust-lang#117164.

r? `@oli-obk` or types

[whnf]: https://en.wikipedia.org/wiki/Lambda_calculus_definition#Weak_head_normal_form
@bors
Copy link
Contributor

bors commented Feb 17, 2024

⌛ Testing commit fde4556 with merge 4993747...

@rust-log-analyzer
Copy link
Collaborator

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

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

@bors
Copy link
Contributor

bors commented Feb 17, 2024

💔 Test failed - checks-actions

@bors 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 Feb 17, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2024

@bors retry, empty log and possible apple-1 builder timeout

@fmease
Copy link
Member Author

fmease commented Feb 17, 2024

I think the retry didn't register
@bors retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2024
@bors
Copy link
Contributor

bors commented Feb 18, 2024

⌛ Testing commit fde4556 with merge d3df8ff...

@bors
Copy link
Contributor

bors commented Feb 18, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 18, 2024
@bors bors merged commit d3df8ff into rust-lang:master Feb 18, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 18, 2024
@fmease fmease deleted the lta-in-impls branch February 18, 2024 06:02
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d3df8ff): 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)
- - 0
Improvements ✅
(secondary)
-1.5% [-1.5%, -1.5%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 639.783s -> 642.411s (0.41%)
Artifact size: 308.82 MiB -> 308.84 MiB (0.00%)

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 20, 2024
Expand weak alias types before collecting constrained/referenced late bound regions + refactorings

Fixes rust-lang#114220.
Follow-up to rust-lang#120780.

r? `@oli-obk`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Rollup merge of rust-lang#121344 - fmease:lta-constr-by-input, r=oli-obk

Expand weak alias types before collecting constrained/referenced late bound regions + refactorings

Fixes rust-lang#114220.
Follow-up to rust-lang#120780.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-lazy_type_alias `#![feature(lazy_type_alias)]` 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
7 participants