Skip to content

-Znext-solver Ignore region constraints from the nested goals in leakcheck#155749

Open
ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
ShoyuVanilla:leakcheck-vis
Open

-Znext-solver Ignore region constraints from the nested goals in leakcheck#155749
ShoyuVanilla wants to merge 1 commit intorust-lang:mainfrom
ShoyuVanilla:leakcheck-vis

Conversation

@ShoyuVanilla
Copy link
Copy Markdown
Member

@ShoyuVanilla ShoyuVanilla commented Apr 24, 2026

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Apr 24, 2026
@ShoyuVanilla ShoyuVanilla changed the title -Znext-solver Ignore region constraints from the nested goals in leakcheck -Znext-solver Ignore region constraints from the nested goals in leakcheck Apr 24, 2026
@rust-log-analyzer

This comment has been minimized.

@ShoyuVanilla ShoyuVanilla force-pushed the leakcheck-vis branch 2 times, most recently from e2650aa to 54588cf Compare April 27, 2026 16:23
rust-bors Bot pushed a commit that referenced this pull request Apr 29, 2026
Canonicalize free regions from inputs as placeholders in root univ



Context: The box named *coroutine witness Send lifetime requirements now considered by leakcheck* [this roadmap](https://raw.githubusercontent.com/hexcatnl/roadmap/e380fef94b47c02a056b4c8f05124a9db475b990/next-solver.svg)

Fixes (only for the next-solver) #106569 
Prerequisite of #155749
rust-bors Bot pushed a commit that referenced this pull request Apr 29, 2026
Canonicalize free regions from inputs as placeholders in root univ



Context: The box named *coroutine witness Send lifetime requirements now considered by leakcheck* [this roadmap](https://raw.githubusercontent.com/hexcatnl/roadmap/e380fef94b47c02a056b4c8f05124a9db475b990/next-solver.svg)

Fixes (only for the next-solver) #106569 
Prerequisite of #155749
@rust-bors

This comment has been minimized.

@ShoyuVanilla
Copy link
Copy Markdown
Member Author

r? lcnr

@ShoyuVanilla ShoyuVanilla marked this pull request as ready for review April 30, 2026 03:23
@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 30, 2026

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

Some changes occurred to the core trait solver

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

changes to the core type system

cc @lcnr

@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 30, 2026
.for_each(|c| each_edge(c.sub, c.sup));
region_constraints.data().constraints[i].0.iter_outlives().for_each(
|Constraint { kind: _, sub, sup, visible_for_leak_check }| {
each_edge(sub, sup, visible_for_leak_check)
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

maybe don't even provide any constraints that are not visible to leak hceck to each_edge, feels cleaner to me,
i.e.

Suggested change
each_edge(sub, sup, visible_for_leak_check)
match visible_for_leak_check {
ty::VisibleForLeakCheck::Yes =>
each_edge(sub, sup),
ty::VisibleForLeakCheck::No => {}
}

View changes since the review

Comment on lines +138 to +139
pub type QueryRegionConstraint<'tcx> =
(ty::RegionConstraint<'tcx>, ConstraintCategory<'tcx>, ty::VisibleForLeakCheck);
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

not in this PR, but mind adding a FIXME: Convert this into a struct

View changes since the review

// We ignore constraints from the nested goals in leak check. This is to match
// with the old solver's behavior, which has separated evaluation and fulfillment,
// and the former doesn't consider outlives obligations from the later.
let vis = if is_root_goal {
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

why special-case root goals here?

View changes since the review

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 thought that we should only ignore region constraints from nested goals and if I mark them as VisibleForLeakCheck::No for root goals as well, they would be so in the InferCtxt outside of the solver and then ignored from the leak check for other goals as well, but after thinking twice, they wouldn't cause universe leaks in other goals anyway 😅 Yeah, so might be okay to remove the spacial casing here

.into_iter()
.filter(|&(outlives, _)| seen.insert(outlives))
.map(|(outlives, _)| outlives)
.filter(|&(outlives, _, vis)| seen.insert((outlives, vis)))
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

this may cause us to end up with the same constraint with vis: Yes and vis: No 🤔 I guess it's fine as long as that doesn't cause issues

View changes since the review

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 did this deliberately as we could already have the same constraints with different visibilities without this and if that would be the case, the resulting visibility would be order-dependent 🤔

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.

could eagerly collect into a list, and use a hashmap pointing to the index of the element, on a hit we Or the visibility

t1,
r2,
constraint_category,
ty::VisibleForLeakCheck::Yes,
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

slightly concerning 🤔

how hard is it to pass VisibleForLeakCheck to here?

View changes since the review

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 guess it wouldn't be that hard but I thought visibilities are not real concerns here in borrowck

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.

they are not 😅 I would love us to somehow defensively guard against this... as rn u need to globally reason about this code for whether it's correct

what do you think you make it into a three-state enum with Yes, No, Unreachable and have the leak_check ICE when encountering Unreachable

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.

Ah, yes, such global context would be very confusing and fragile to future changes 😅 That would be more desirable 👍

ty: Ty<'tcx>,
region: ty::Region<'tcx>,
category: ConstraintCategory<'tcx>,
vis: ty::VisibleForLeakCheck,
Copy link
Copy Markdown
Contributor

@lcnr lcnr Apr 30, 2026

Choose a reason for hiding this comment

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

components_must_outlive can always be ty::VisibleForLeakcheck::No/ for TypeOutlives in general 🤔

View changes since the review

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 guess they might be decomposed into RegionOutlivesPredicate with component-wise thingy. Would it be okay?

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.

it matches the old solver which currently never considers type outlives in the leak check 🤔

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.

but also, we only ever use this in places which won't ever leak check anymore, so 🤷

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.

Maybe we could try VisibleForLeakCheck::Unreachable here as well?

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.

jup

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Apr 30, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

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

Labels

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 (-Znext-solver)

Projects

None yet

4 participants