trait solver: limit eager placeholder handling

[Dnreikronos]

fixes #157840

with -zassumptions-on-binders, eager placeholder handling was walking up to the inference context's current universe, even when the current solver region constraint did not mention those universes. that made it ask for placeholder assumptions for unrelated universes and hit the unwrap() in get_placeholder_assumptions.

this bounds the eager pass to the max universe actually present in the region constraint. imo that feels like the less spooky fix: idk why we would try to handle placeholders for universes the constraint cannot name. ltm, this keeps the existing assumptions machinery intact instead of making missing entries silently mean ambiguity.

also adds the issue repro as a ui test. lgtm locally with x.py test tests/ui/assumptions_on_binders --stage 1 --force-rerun.

[rustbot]

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 20 candidates

[JonathanBrouwer]

r? types

[jackh726]

I'm not entirely sure if this is the right fix (I would actually expect that self.delegate.universe() to return the proper universe), but this is also quite new code. So going to reassign to Boxy.

r? @BoxyUwU

[Dnreikronos]

yeah, i think you're right that this probably is not the ideal long-term shape.

what this pr does is more of a local bound: eager placeholder handling only walks universes the current solver region constraint can actually name. imo that part still makes sense to me. self.delegate.universe() is just the highest universe the infcx has created, and solver work can bump that even if the final constraint no longer mentions that universe. then we ask get_placeholder_assumptions for a universe that is not relevant to the constraint and may not have an entry, which is the unwrap in the repro.

so yeah, idk if i would call this the best fix in the abstract. long term i'd expect us to have a cleaner invariant between created universes and placeholder assumptions, or maybe an explicit set of universes that eager handling should process.

but for this pr, i still prefer this local fix over making missing assumptions silently mean ambiguity. if the constraint cannot name the universe, i don't think this pass should care about it.

so this still lgtm to me as a short term fix, with the caveat that self.delegate.universe() being too global here is probably worth documenting.

[JorgeCepeda]

Offtopic: what does ltm mean?

[Dnreikronos]

Offtopic: what does ltm mean?

long term.
i just edited to be easier to understand, my bad....

[BoxyUwU]

this in theory makes sense as an optimization but it shouldn't make anything stop ICEing. We should not be entering binders inside of the trait solver without accounting for their assumptions 🤔

long term i'd expect us to have a cleaner invariant between created universes and placeholder assumptions, or maybe an explicit set of universes that eager handling should process.

the set of universes it should handle is all of them (other than input universes). can you look into where/why we're entering a binder and not inserting assumptions for it

[Dnreikronos]

this in theory makes sense as an optimization but it shouldn't make anything stop ICEing. We should not be entering binders inside of the trait solver without accounting for their assumptions 🤔

long term i'd expect us to have a cleaner invariant between created universes and placeholder assumptions, or maybe an explicit set of universes that eager handling should process.

the set of universes it should handle is all of them (other than input universes). can you look into where/why we're entering a binder and not inserting assumptions for it

yeah, you were right. i looked into where the unaccounted binder entry was happening, and it seems like the issue was in findparaminclause.

the bad path was basically: findparaminclause walks into a binder, then replace_bound_vars materializes a placeholder universe through boundvarreplacer, but that universe never got an entry in the placeholder assumptions map. later eager placeholder handling still walks all non-input universes, as it should, and then get_placeholder_assumptions hits the missing entry and ices.

i changed the patch so eager handling keeps using all non-input universes. instead, when findparaminclause calls replace_bound_vars, it tracks any previously-empty universe slot that got filled and registers empty placeholder assumptions for that universe. imo this is a better shape than bounding eager handling by the current constraint, since it fixes the missing bookkeeping directly and does not change the global/non-global classification by bailing on binders.

i also added the issue repro as a ui test. with this version it no longer ices and reports the overflow diagnostic instead.

pushed this version for you to validate. idk if long-term we want boundvarreplacer to report newly-created universes more explicitly, but for this pr i think this keeps the fix small and matches the existing empty-assumptions pattern. lgtm to me, but i'd like your take on whether empty assumptions is the right local accounting here.