Skip to content

Make region equality emits Eq constraints#155258

Open
ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
ShoyuVanilla:eq-constraint
Open

Make region equality emits Eq constraints#155258
ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
ShoyuVanilla:eq-constraint

Conversation

@ShoyuVanilla
Copy link
Copy Markdown
Member

@ShoyuVanilla ShoyuVanilla commented Apr 13, 2026

View all comments

For context, see..

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Apr 13, 2026
@ShoyuVanilla
Copy link
Copy Markdown
Member Author

r? lcnr

@ShoyuVanilla ShoyuVanilla marked this pull request as ready for review April 13, 2026 20:52
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 13, 2026

Some changes occurred to the core trait solver

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 13, 2026
//~| ERROR mismatched `self` parameter type
//~| NOTE expected struct `Foo<'a, 'b>`
//~| NOTE found struct `Foo<'b, 'a>`
//~| NOTE lifetime mismatch
Copy link
Copy Markdown
Member Author

@ShoyuVanilla ShoyuVanilla Apr 13, 2026

Choose a reason for hiding this comment

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

View changes since the review

Other tests are untouched but this has been affected somehow (I guess this might be due to having some duplication with both 'a: 'b, 'b: 'a and 'a == 'b)
I think this might be okay as it's unchanged anyway modulo diagnostics deduplication

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems fine 🤷

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This somehow got returned back again while destructuring eq bounds 😂

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ShoyuVanilla
Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Apr 14, 2026
Make region equality emits Eq constraints
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 14, 2026

☀️ Try build successful (CI)
Build commit: 52727e5 (52727e55499277ba04069f2dfd8ca8cbbc4e76b6, parent: 338dff3e3a375cb4a3c68be825058c582262655a)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (52727e5): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.0%, 0.3%] 6
Improvements ✅
(primary)
-0.3% [-0.5%, -0.2%] 23
Improvements ✅
(secondary)
-0.5% [-1.3%, -0.0%] 27
All ❌✅ (primary) -0.3% [-0.5%, -0.2%] 23

Max RSS (memory usage)

Results (secondary -0.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [0.9%, 5.7%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.1% [-3.6%, -2.8%] 4
All ❌✅ (primary) - - 0

Cycles

Results (primary -2.4%, secondary -5.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.4% [-2.4%, -2.4%] 1
Improvements ✅
(secondary)
-5.3% [-8.9%, -1.9%] 10
All ❌✅ (primary) -2.4% [-2.4%, -2.4%] 1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 489.33s -> 490.845s (0.31%)
Artifact size: 394.21 MiB -> 394.03 MiB (-0.04%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 14, 2026
@ShoyuVanilla
Copy link
Copy Markdown
Member Author

Oh, looks like having less number of constraints by collapsing into Eq helps the perf a bit

Copy link
Copy Markdown
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

I would keep assumptions as bidirectional outlives and never have Eq in there

can you make sure we always exhaustively match on ConstraintKind 🤔 I do dislike constraint kind as a concept and feel like we should just match on the actual region instead of separately storing that in the ConstraintKind

View changes since this review

Comment on lines +48 to +51
/// - We want to uplift bidirectional constraints to the caller instead of unifying them
/// when solving nested goals, otherwise we often lose implied bounds.
/// - But we still want to know they are equal from the caller. This is crucial when we
/// are proving other nested goals which are sensitive to region equalities.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this comment is slightly confusing/unhelpful to me right now. It is hard to express why exactly we need this, because really, it is a hack required by the better solution not being possible

"otherwise we often lose implied bounds", what do you mean, what is often, link to a relevant example

I think the framing here isn't quite right. For me the issue is that having existential regions which aren't actually existential is not something which we can support.

Also, "know they are equal" seems slightly odd to me. Know they are equal means "unify existential regions with regions they are equal to, to resolve them to the same thing" here I think? Maybe make that more clear

I also think our conversation on zulip has been useful here, so maybe just link to that 😁

@ShoyuVanilla
Copy link
Copy Markdown
Member Author

I would keep assumptions as bidirectional outlives and never have Eq in there

Okay, I'll keep them as they are.

can you make sure we always exhaustively match on ConstraintKind 🤔 I do dislike constraint kind as a concept and feel like we should just match on the actual region instead of separately storing that in the ConstraintKind

Yeah, I also thought ConstraintKind is very iffy while working on this 😅 Maybe it would be great to clean them up as a follow up. For now, I'll make sure they are exhaustively matched

@lcnr
Copy link
Copy Markdown
Contributor

lcnr commented Apr 14, 2026

yeah, I feel like ConstraintKind just shouldn't exist :3 the fact that we currently use it instead of matching on the RegionKind at the relevant points is a mess imo, though generally the way lexical region resolution works is a mess and should be replaced at some point

@ShoyuVanilla ShoyuVanilla force-pushed the eq-constraint branch 2 times, most recently from 35c064f to 0f3c1ee Compare April 14, 2026 13:26
}

// Index in-edges in reverse order, to match what current tests expect.
// (It's unclear whether this is important or not.)
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

rustc is such a good codebase 🙃

can you add a fixme to merge these two loops?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm afraid I might be the one who often writes such good codes 😂

ConstraintKind::RegSubVar => {
let sup_vid = c.sup.as_var();

let reg_sub_var = |sub: Region<'tcx>,
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

instead of making this closures, can you change the iterators to iter over self.data.contraints.iter().flat_map(|constraints| constraints.convert_to_outlives()) which does the eq to bidir outlives mapping.

And then use that everywhere u eq should just be handled as outlives

// coroutine well-formedness.
if self.tcx.sess.opts.unstable_opts.higher_ranked_assumptions {
storage.data.constraints.retain(|(c, _)| match c.kind {
storage.data.constraints.retain_mut(|(c, _)| match c.kind {
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

hmm, alternatively, can you already do this eq to bidir outlives when constructing the storage and then just always assert we only have outlives constraints everywhere else?

&& outlives_env
.higher_ranked_assumptions()
.contains(&ty::OutlivesPredicate(sup_type.into(), sub_region))
.contains(&ty::OutlivesPredicate(sup_type.into(), sub_region).into())
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

these new .into() calls should be unnecessary, are they?

ConstraintKind::VarSubVar
| ConstraintKind::RegSubVar
| ConstraintKind::VarSubReg
| ConstraintKind::RegSubReg => {}
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

here we could also change the iterator to do a flat_map on map to bidir?

}
} else {
region_constraints.data().constraints.iter().for_each(|(c, _)| each_edge(c.sub, c.sup))
region_constraints.data().constraints.iter().for_each(|(c, _)| {
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

deps1.larger.insert(RegionTarget::RegionVid(vid_b));
deps1.smaller.insert(RegionTarget::RegionVid(vid_b));
}

Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

could this also just destructure eq to bidir outlives to avoid this code?

Comment on lines +85 to +91
.flat_map(|(constraint, _)| match constraint {
ty::RegionConstraint::Outlives(outlives) => iter::once(outlives).chain(None),
ty::RegionConstraint::Eq(eq) => {
let [outlives1, outlives2] = eq.into_bidirectional_outlives();
iter::once(outlives1).chain(Some(outlives2))
}
})
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

this match should be a method given that we should use it a bunch :3

.or_default()
.push(c.sup);
}
}
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 14, 2026

Choose a reason for hiding this comment

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

View changes since the review

this can also act on the destructured eq bounds :3

//~| ERROR mismatched `self` parameter type
//~| NOTE expected struct `Foo<'a, 'b>`
//~| NOTE found struct `Foo<'b, 'a>`
//~| NOTE lifetime mismatch
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seems fine 🤷

@rust-log-analyzer

This comment has been minimized.

@ShoyuVanilla ShoyuVanilla force-pushed the eq-constraint branch 2 times, most recently from 1cce45b to ee76953 Compare April 14, 2026 17:18
@ShoyuVanilla
Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rust-bors

This comment has been minimized.

rust-bors bot pushed a commit that referenced this pull request Apr 14, 2026
Make region equality emits Eq constraints
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2026
@rust-bors
Copy link
Copy Markdown
Contributor

rust-bors bot commented Apr 14, 2026

☀️ Try build successful (CI)
Build commit: d50a4bb (d50a4bb74fc592f8e09fcb66ace3b73da8846142, parent: 12f35ad39ed3e39df4d953c46d4f6cc6c82adc96)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (d50a4bb): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.0%, 0.3%] 6
Improvements ✅
(primary)
-0.3% [-0.4%, -0.2%] 17
Improvements ✅
(secondary)
-0.6% [-1.3%, -0.0%] 21
All ❌✅ (primary) -0.3% [-0.4%, -0.2%] 17

Max RSS (memory usage)

Results (secondary 1.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.7% [3.7%, 5.9%] 4
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.6% [-3.1%, -1.4%] 4
All ❌✅ (primary) - - 0

Cycles

Results (primary 6.8%, secondary -3.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
6.8% [2.0%, 10.5%] 6
Regressions ❌
(secondary)
3.1% [2.3%, 4.0%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.7% [-9.0%, -1.7%] 6
All ❌✅ (primary) 6.8% [2.0%, 10.5%] 6

Binary size

Results (primary -0.0%, secondary -0.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.0% [-0.0%, -0.0%] 11
Improvements ✅
(secondary)
-0.1% [-0.1%, -0.1%] 4
All ❌✅ (primary) -0.0% [-0.0%, -0.0%] 11

Bootstrap: 489.936s -> 490.267s (0.07%)
Artifact size: 394.18 MiB -> 394.06 MiB (-0.03%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 14, 2026
ConstraintKind::RegSubVar
| ConstraintKind::RegSubReg
| ConstraintKind::RegEqReg => {}
}
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 15, 2026

Choose a reason for hiding this comment

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

can you also map use bidir for both of the loops here?

View changes since the review

Comment on lines +467 to +470
self.add_constraint(
Constraint { kind: ConstraintKind::VarEqVar, sub: a, sup: b },
origin,
);
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 15, 2026

Choose a reason for hiding this comment

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

mind adding a fixme that we could only emit this constraints if unify_var_var returns an error?

View changes since the review

Copy link
Copy Markdown
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

r=me after these nits

View changes since this review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

perf-regression Performance regression. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants