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

Add bidirectional where clauses on RPITIT synthesized GATs #112682

Merged
merged 6 commits into from Jun 30, 2023

Conversation

spastorino
Copy link
Member

@spastorino spastorino commented Jun 15, 2023

Given the following:

struct MyStruct<'a, T>(&'a T);

trait MyTrait<'a, T> {
    fn my_fn<'b, 'c, 'd, V>(item: &'c String) -> impl Sized + 'a + 'b + 'c where V: 'b, V: 'd;
}

impl<'a, T> MyTrait<'a, T> for MyStruct<'a, T> {
    fn my_fn<'b, 'c, 'd, V>(_: &'c String) -> impl Sized + 'a + 'b + 'c
    where
        V: 'b,
        V: 'd,
    {
        unimplemented!();
    }
}

We need the desugaring to be:

trait MyTrait<'a, T> {
    type MyFn<'bf, 'df, Vf, 'a2, 'b2, 'c2>: Sized + 'a2 + 'b2 + 'c2 where Vf: 'b2, 'a2: 'a, 'a: 'a2, 'b2: 'bf, 'bf: 'b2;

    fn my_fn<'b, 'c, 'd, V>(item: &'c String) -> MyStruct<'a>::MyFn<'b, 'd, V, 'a, 'b, 'c> where V: 'b, V: 'd {
        type opaque<'a3, 'b3, 'c3>;
    };
}
    
impl<'a, T> MyIter<'a, T> for MyStruct<'a, T> {
    type MyFn<'bf, 'df, Vf, 'a2, 'b2, 'c2> = impl Sized + 'a2 + 'b2 + 'c2 where Vf: b2, 'a2: 'a, 'a: 'a2, 'b2: 'bf, 'bf: 'b2;

    fn my_fn<'b, 'c, 'd, V>(_: &'c String) -> MyStruct<'a>::MyFn<'a, 'b, 'c, V> where V: 'b, V: 'd {
        type opaque<'a3, 'b3, 'c3>;
        unimplemented!();
    }
}

This PR adds the where clauses for the MyFn generated GATs.

This is a draft with a very ugly solution so we can make comments over concrete code.

r? @compiler-errors

@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 Jun 15, 2023
@spastorino spastorino changed the title Add bidirectional where clauses on RPITIT synthesized GATs [WIP] Add bidirectional where clauses on RPITIT synthesized GATs Jun 15, 2023
@compiler-errors compiler-errors marked this pull request as draft June 15, 2023 20:26
@spastorino spastorino changed the title [WIP] Add bidirectional where clauses on RPITIT synthesized GATs Add bidirectional where clauses on RPITIT synthesized GATs Jun 15, 2023
};

for (arg, dup_def) in bidir_outlives {
let orig_region = icx.astconv().ast_region_to_region(arg, None);
Copy link
Member Author

Choose a reason for hiding this comment

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

@compiler-errors you told me that this could be done using named_region or something like that. I was reusing this idea https://github.com/rust-lang/rust/pull/112682/files#diff-6d9f47a8bce6ee31fd3e867514607c8503a11cf91616ce418393ba9304a22c16R435. In case Github doesn't take you there line 435 on the right side.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess you meant named_bound_var.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that's fine. I think using named_bound_var might be easier but I guess it doesn't matter.

@rust-log-analyzer

This comment has been minimized.

@spastorino
Copy link
Member Author

The job x86_64-gnu-llvm-14 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

Uhh this is likely ungreat perf wise.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@spastorino spastorino marked this pull request as ready for review June 21, 2023 22:26
static_assert_size!(Item<'_>, 80);
static_assert_size!(ItemKind<'_>, 48);
static_assert_size!(Item<'_>, 88);
static_assert_size!(ItemKind<'_>, 56);
Copy link
Member Author

Choose a reason for hiding this comment

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

@compiler-errors this is ungreat :). Unsure how bad would be the impact, we should probably move bidir_outlives out from the OpaqueTy variant or came up with better ideas to improve the size of ItemKind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is the best way of doing it.

@rust-log-analyzer

This comment has been minimized.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 22, 2023
…nd-region-debug, r=compiler-errors

Print def_id on EarlyBoundRegion debug

It's not the first time that I can't make sense out of the default debug print on `EarlyBoundRegion`. As I was working on rust-lang#112682 I needed this.

I was doing some git archeology and found that we used to print everything https://github.com/spastorino/rust/blob/dfbc9608ce5c9655a36b63f6cc9694f5e4ad9890/src/librustc/util/ppaux.rs#L425-L430 but we lost the ability in some refactor midway.
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0c07919285caf69c4888d889cd23b4f62578282f): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 1

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.5% [3.5%, 3.5%] 1
Regressions ❌
(secondary)
2.8% [2.1%, 3.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.5% [3.5%, 3.5%] 1

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: 663.81s -> 660.746s (-0.46%)

@rustbot rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Jun 26, 2023
@spastorino
Copy link
Member Author

@compiler-errors now I think this is ready to merge. Perf is back to normal.

@compiler-errors
Copy link
Member

I still need to review this for correctness, but yeah, I saw the perf results.

@spastorino
Copy link
Member Author

Rebased ☝️ after conflicts on predicates_of.rs

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Can you rename bidir_outlives in the hir::OpaqueTy to something like lifetime_mapping, and add a doc comment that explains what it is, and note that it's only populated if in_trait is true (because it's unneeded for other opaques)?

r=me after that and the nits below

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/collect/predicates_of.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/src/collect/predicates_of.rs Outdated Show resolved Hide resolved
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2023

📌 Commit 6f4a51e has been approved by compiler-errors

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 Jun 29, 2023
@bors
Copy link
Contributor

bors commented Jun 29, 2023

⌛ Testing commit 6f4a51e with merge 3307274...

@bors
Copy link
Contributor

bors commented Jun 30, 2023

☀️ Test successful - checks-actions
Approved by: compiler-errors
Pushing 3307274 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 30, 2023
@bors bors merged commit 3307274 into rust-lang:master Jun 30, 2023
9 of 12 checks passed
@rustbot rustbot added this to the 1.72.0 milestone Jun 30, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (3307274): 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.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.9% [-1.9%, -1.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-1.9%, 0.8%] 2

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.6% [-0.7%, -0.5%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.6% [-0.7%, -0.5%] 4

Binary size

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

Bootstrap: 660.993s -> 662.146s (0.17%)

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 10, 2023
…-errors

 Replace RPITIT current impl with new strategy that lowers as a GAT

This PR replaces the current implementation of RPITITs with the new implementation that we had under -Zlower-impl-trait-in-trait-to-assoc-ty flag that lowers the RPIT as a GAT on the trait and on the impls that implement that trait.

Opening this PR as a draft because this goes after rust-lang#112682, ~rust-lang#112981~ and ~rust-lang#112983~.
As soon as those are merged, I can rebase and we should run perf, crater and test a lot.

r? `@compiler-errors`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants