Skip to content

Do not increase depth when evaluating nested goals of NormalizesTo#157718

Open
ShoyuVanilla wants to merge 1 commit into
rust-lang:mainfrom
ShoyuVanilla:nested-normalizes-to-without-depth-increase
Open

Do not increase depth when evaluating nested goals of NormalizesTo#157718
ShoyuVanilla wants to merge 1 commit into
rust-lang:mainfrom
ShoyuVanilla:nested-normalizes-to-without-depth-increase

Conversation

@ShoyuVanilla

@ShoyuVanilla ShoyuVanilla commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

cc #156619 (comment)

Only the last commit is relevant since this is based on #156619

r? lcnr

@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 Jun 10, 2026
@ShoyuVanilla ShoyuVanilla mentioned this pull request Jun 10, 2026
Merged
@rust-bors

This comment has been minimized.

Sign in to view
lcnr
lcnr reviewed Jun 16, 2026
View reviewed changes
Comment thread compiler/rustc_type_ir/src/search_graph/mod.rs Outdated
Comment on lines +641 to +645
parent.required_depth =
parent.required_depth.max(match parent.increase_depth_for_nested {
IncreaseDepthForNested::Yes => required_depth_for_nested + 1,
IncreaseDepthForNested::No => required_depth_for_nested,
});

@lcnr lcnr Jun 16, 2026 •
edited by rustbot
Loading

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.

I've been thinking about about this as this feels brittle. Instead of required_depth, could we change the search graph to track the "min_reached_available_depth" and then required depth is computed as "available_depth - min_reached_available_depth"

that way the way depth is actually tracked doesn't matter anymore

View changes since the review

@ShoyuVanilla ShoyuVanilla Jun 16, 2026

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.

Sounds good to me. I'll try that way 👍

@ShoyuVanilla ShoyuVanilla force-pushed the nested-normalizes-to-without-depth-increase branch from e59d7fa to dc07fda Compare June 17, 2026 15:32
@ShoyuVanilla ShoyuVanilla marked this pull request as ready for review June 17, 2026 15:33
@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 Jun 17, 2026
lcnr
lcnr reviewed Jun 18, 2026
View reviewed changes
Comment thread compiler/rustc_type_ir/src/search_graph/mod.rs Outdated
&mut self.stack,
step_kind_from_parent,
0,
None,

@lcnr lcnr Jun 18, 2026 •
edited by rustbot
Loading

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.

can you put the min_reachable_for_nested in the UpdateParentGoalCtxt instead?

View changes since the review

lcnr
lcnr reviewed Jun 18, 2026
View reviewed changes

@lcnr lcnr left a comment •
edited by rustbot
Loading

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.

does wgpu compile with this change now?

View changes since this review

@ShoyuVanilla

ShoyuVanilla commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

does wgpu compile with this change now?

View changes since this review

Hmm, this doesn't help it at all 🤔
wgpu 25.0.2 is compiled with -Zmin-recursion-limit=141 but not with any lower number regardless of this change.
Maybe the deepest recursion in it isn't a normalization or I'm doing things wrong.
I'll look into that crate more.

@ShoyuVanilla

ShoyuVanilla commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Yeah, we only have <= 1 NormalizesTo goal for each overflowing goal's search graph stack for that crate

@lcnr

lcnr commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

oh 🤔 interesting xd do we have AliasRelate goals there? these do still exist rn after all

@ShoyuVanilla

ShoyuVanilla commented Jun 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Yes, they do still exist. But we have very few of them anyway (<= 1 for each stack, total 3)

@lcnr

lcnr commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

okay, so there are some other goals whose depth we don't properly increase in the old solver but now do in the new one 🤔

what a pain

@ShoyuVanilla

ShoyuVanilla commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

Indeed. I'll take a look this weekend

@ShoyuVanilla ShoyuVanilla force-pushed the nested-normalizes-to-without-depth-increase branch from dc07fda to ee25de1 Compare June 18, 2026 14:26
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ShoyuVanilla ShoyuVanilla force-pushed the nested-normalizes-to-without-depth-increase branch from ee25de1 to e23e501 Compare June 18, 2026 15:03
@rustbot

rustbot commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

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.

@rust-log-analyzer

rust-log-analyzer commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

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

Click to see the possible cause of the failure (guessed by this bot) 
  IMAGE: x86_64-gnu-llvm-21
##[endgroup]
    Updating crates.io index
error: failed to get `adler2` as a dependency of package `miniz_oxide v0.8.8`
    ... which satisfies dependency `miniz_oxide = "^0.8.5"` of package `flate2 v1.1.9`
    ... which satisfies dependency `flate2 = "^1.1.9"` of package `citool v0.1.0 (/home/runner/work/rust/rust/src/ci/citool)`

Caused by:
  failed to load source for dependency `adler2`

Caused by:
  unable to update registry `crates-io`

Caused by:
  download of ad/le/adler2 failed

Caused by:
  curl failed

Caused by:

@ShoyuVanilla

ShoyuVanilla commented Jun 19, 2026

Copy link
Copy Markdown
Member Author

https://rust-lang.zulipchat.com/#narrow/channel/364551-t-types.2Ftrait-system-refactor/topic/The.20old.20solver.20don't.20record.20required.20depth.20on.20cache/with/604759762

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

Reviewers

@lcnr lcnr lcnr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@lcnr lcnr

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ShoyuVanilla @lcnr @rust-log-analyzer @rustbot