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

Canonicalize all nested goals together at once when making response #124689

Closed

Conversation

compiler-errors
Copy link
Member

Since we canonicalize each added goal separately, any time we add two goals that should be linked by an inference variable local to our evaluation, they end up unlinked in the nested goals of a inspect candidate. For example,

impl<T, U> Foo for T
where
    T: Constrain<Output = U>,
    U: Bar,
{
}

impl Constrain for () {
    type Output = ();
}

when proving (): Foo, we expect two nested goals: <() as Constrain>::Output = ?0 and ?0: Bar. However, we end up actually instantiating the second goal as ?1: Bar, leaving it totally unconstrained. This leads to anomalous states, such as a candidate with Certainty::Yes but with Certainty::Maybe in its nested goals❗

This PR aims to fix this problem by canonicalizing all of the nested goals all together at once, which means that we'll canonicalize any duplicated instances of a variable as a single variable.

r? lcnr

@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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels May 4, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 4, 2024

Some changes occurred to the core trait solver

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

@compiler-errors

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member Author

🤦 needed to collect all goals

fn all_added_goals(
&self,
) -> impl Iterator<Item = (GoalSource, Goal<'tcx, ty::Predicate<'tcx>>)> + '_ {
std::iter::from_coroutine(
Copy link
Member Author

Choose a reason for hiding this comment

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

Chose to use a generator here but I could very well write this using a flat map or something, if you'd prefer. Or just return a vec.

@lcnr
Copy link
Contributor

lcnr commented May 4, 2024

I think we should instead add the fresh vars to the proof tree in fresh_args_for_item, we already do this when creating unconstrained vars for structurally_normalize

@compiler-errors
Copy link
Member Author

oh lol that's so much easier to do

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants