preserve overflow in builtin Field candidates

[amirHdev]

View all comments

preserve overflow from the builtin Field candidate instead of treating it as NoSolution
the Sized requirements are now evaluated inside the candidate probe and coherence correctly rejects overlapping impls when evaluation overflows.

fixes #162125

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
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 76 candidates
• Random selection from 18 candidates

[BennoLossin]

Thanks for fixing this, I also figured out the solution in the zulip thread and I think our code looks pretty much the same, except for early return style vs bigger if.

my solution

    fn consider_builtin_field_candidate(
        ecx: &mut EvalCtxt<'_, D>,
        goal: Goal<I, Self>,
    ) -> Result<Candidate<I>, NoSolutionOrRerunNonErased> {
        if goal.predicate.polarity != ty::ClausePolarity::Positive {
            return Err(NoSolution.into());
        }

        if let ty::Adt(def, args) = goal.predicate.self_ty().kind()
            && let Some(FieldInfo { base, ty, .. }) =
                def.field_representing_type_info(ecx.cx(), args)
            && match base.kind() {
                ty::Adt(def, _) => def.is_struct() && !def.is_packed(),
                ty::Tuple(..) => true,
                _ => false,
            }
        {
            ecx.probe_builtin_trait_candidate(BuiltinImplSource::Misc).enter(|ecx| {
                let cx = ecx.cx();
                let sized_trait = ecx.cx().require_trait_lang_item(SolverTraitLangItem::Sized);
                // FIXME: add better support for builtin impls of traits that check for the bounds
                // on the trait definition in std.

                // NOTE: these bounds have to be kept in sync with the definition of the `Field`
                // trait in `library/core/src/field.rs` as well as the old trait solver `fn
                // assemble_candidates_for_field_trait` in
                // `compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs`.
                ecx.add_goal(
                    GoalSource::ImplWhereBound,
                    goal.with(cx, ty::TraitRef::new(ecx.cx(), sized_trait, [base])),
                )?;
                ecx.add_goal(
                    GoalSource::ImplWhereBound,
                    goal.with(cx, TraitRef::new(ecx.cx(), sized_trait, [ty])),
                )?;
                ecx.evaluate_added_goals_and_make_canonical_response(Certainty::Yes)
            })
        } else {
            Err(NoSolution.into())
        }
    }

Maybe @lcnr would make for a better reviewer?

[amirHdev]

Closing this since I realized @BennoLossin had already worked it out :)

I missed that in the Zulip thread
Thanks for digging into it, Benno

[BennoLossin]

Ah no worries, I was glad that I didn't have to spin up a PR :) so feel free to reopen this one

[BennoLossin]

@amirHdev do you want to finish this work? if so, just reopen the PR

[amirHdev]

Reopening as suggested thanks Benno :) I'll finish this up here

And thanks for the earlier Zulip investigation as well

[JohnTitor]

@rustbot reroll

[BennoLossin]

let's see if I'm able to do this, if not, @amirHdev lcnr is probably the most appropriate reviewer for this.

r? @lcnr

edit: yay it worked :)

[lcnr]

what if that's an infer var @BennoLossin, is that possible?

View changes since the review

[amirHdev]

base comes from args.type_at(0)
it can still be an infer var here.

I'll move this check back inside the probe

[BennoLossin]

yeah that sounds correct

[BennoLossin]

actually, if the field representing type info exists, then base is always an adt or tuple, so I don't think htis needs to be moved in

[lcnr]

yeah, this should handle infer vars then 😁 and we should get a test where that happens, maybe by makig this match ICE on ty::Infer and then getting a test to panic.

also, what are other allowed base types here. Could we make this match exhaustive/unreachable! in the catch all?

[BennoLossin]

I don't understand why we need to handle infer vars here. the type should be concrete... what would an example of ty::Infer be? something like this?

#![feature(field_projections)]
use std::field::field_of;
fn main() {
    let _: field_of!(_, field);
}

error: cannot use `_` in this position
 --> src/main.rs:4:22
  |
4 |     let _: field_of!(_, field);
  |   

[lcnr]

didn't read your second message in depth 😅

okay, then this should be an unreachable!. That's also fine with me

[amirHdev]

as field_representing_type_info only gives us an ADT or tuple here we can keep the other ADT cases as NoSolution and made everything else unreachable!()

[lcnr]

r=me on the refactor 👍

View changes since this review

[lcnr]

@rustbot author

[lcnr]

@bors r+ rollup

[rust-bors]

📌 Commit 8bd4b8a has been tentatively approved by lcnr

It will be put into the queue for this repository once PR CI succeeds.

[rust-bors]

❌ Commit 8bd4b8a has been unapproved due to PR CI failure. Reapprove it with @bors r+ force if you want to ignore the failure.

[rustbot]

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.

[amirHdev]

@lcnr lol. the CI failure was stale stderr after the recent diagnostic span change
fixed

[lcnr]

@bors r+ rollup

[rust-bors]

📌 Commit 01ad9b7 has been tentatively approved by lcnr

It will be put into the queue for this repository once PR CI succeeds.