Avoid leaking opaque hidden types via auto trait candidates

[Shourya742]

closes: #134578

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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 74 candidates
• Random selection from 16 candidates

[Shourya742]

r? @lcnr

[Shourya742]

Turns out NoSolution can leak hidden types too, since the failed proof can go through caller bounds and end up comparing the hidden type with something visible to the caller.

For now I went conservative, if the hidden-type proof fails and there are caller bounds around, I return ambiguity. That’s probably way broader than necessary, since totally unrelated bounds would get caught by this too. Curious if on what are your thoughts on this.

View changes since the review

[lcnr]

I don't get what you mean here 🤔 can you provide an example where NoSolution causes problems?

[Shourya742]

In both the cases, the fudge result is different depending on whether the hidden type proof returns ambiguity or propagates NoSolution.

This is without the hack, where we return the error as is.

2ms DEBUG rustc_infer::infer::snapshot::fudge return=Ok((ObligationCause { span: /home/gh-Shourya742/rust/tests/ui/impl-trait/auto-trait-leakage/opaque-hidden-ty-inference.rs:25:18: 25:45 (#0), body_def_id: DefId(0:11 ~ opaque_hidden_ty_inference[daf7]::leak), code: ImplDerived(ImplDerivedCause { derived: DerivedCause { parent_trait_pred: Binder { value: TraitPredicate(<dep::WaddupGamers<T, {closure@dep::define<T>::{closure#0}}> as std::marker::Unpin>, polarity:Positive), bound_vars: [] }, parent_code: BuiltinDerived(DerivedCause { parent_trait_pred: Binder { value: TraitPredicate(<impl Sized as std::marker::Unpin>, polarity:Positive), bound_vars: [] }, parent_code: WhereClauseInExpr(DefId(0:5 ~ opaque_hidden_ty_inference[daf7]::require_auto), /home/gh-Shourya742/rust/tests/ui/impl-trait/auto-trait-leakage/opaque-hidden-ty-inference.rs:12:20: 12:25 (#0), HirId(DefId(0:11 ~ opaque_hidden_ty_inference[daf7]::leak).4), 1) }) }, impl_or_alias_def_id: DefId(20:9 ~ opaque_auto_trait_leakage[0084]::{impl#0}), impl_def_predicate_index: Some(0), span: /home/gh-Shourya742/rust/tests/ui/impl-trait/auto-trait-leakage/auxiliary/opaque-auto-trait-leakage.rs:3:14: 3:23 (#0) }) }, Obligation(predicate=Binder { value: ProjectionPredicate(Alias { kind: ProjectionTy { def_id: DefId(20:13 ~ opaque_auto_trait_leakage[0084]::Leak::Assoc) }, args: [T/#0], .. }, Term::Ty(Closure(DefId(20:20 ~ opaque_auto_trait_leakage[0084]::define::{closure#0}), [T/#0, i8, Binder { value: extern "RustCall" fn(()), bound_vars: [] }, ()]))), bound_vars: [] }, depth=2)))

The important part is that the diagnostic cause is now ImplDerived, with the parent obligation:

<dep::WaddupGamers<T, {closure@dep::define<T>::{closure#0}}> as std::marker::Unpin>

and the derived obligation:

<T as Leak>::Assoc = Closure(... define::{closure#0} ...)

So the hidden closure becomes part of the diagnostic obligation.

With the hack, where we turn this hidden type NoSolution into ambiguity, the fudge result stays at the original require_auto obligation:

1ms DEBUG rustc_infer::infer::snapshot::fudge return=Ok((ObligationCause { span: /home/gh-Shourya742/rust/tests/ui/impl-trait/auto-trait-leakage/opaque-hidden-ty-inference.rs:25:18: 25:45 (#0), body_def_id: DefId(0:11 ~ opaque_hidden_ty_inference[daf7]::leak), code: WhereClauseInExpr(DefId(0:5 ~ opaque_hidden_ty_inference[daf7]::require_auto), /home/gh-Shourya742/rust/tests/ui/impl-trait/auto-trait-leakage/opaque-hidden-ty-inference.rs:12:20: 12:25 (#0), HirId(DefId(0:11 ~ opaque_hidden_ty_inference[daf7]::leak).4), 1) }, Obligation(predicate=Binder { value: TraitPredicate(<impl Sized as std::marker::Unpin>, polarity:Positive), bound_vars: [] }, depth=0)))

Here the captured diagnostic obligation is only:

<impl Sized as std::marker::Unpin>

With ambiguity we do not capture the hidden WaddupGamers<T, closure> impl derived obligation as the diag cause. Without the hack, the hard NoSolution lets diag's select that hidden impl derived obligation, which is how the closure leaks into the diag.

[Shourya742]

Not sure this is the right fix, but hard failure here lets diagnostics pick up details the caller shouldn’t see.

[lcnr]

what is the code snippet whose behavior changes?

[Shourya742]

This thing,

The hack, if bounds are present return ambiguous, (this is what is done in this PR)

            let param_env_may_leak_hidden_ty = !goal.param_env.caller_bounds().is_empty();
            return match candidate {
                Ok(candidate) if has_only_region_constraints(candidate.result) => Ok(candidate),
                Ok(_) => ecx.forced_ambiguity(MaybeInfo::AMBIGUOUS),
                Err(NoSolutionOrRerunNonErased::NoSolution(_)) if param_env_may_leak_hidden_ty => {    
                            ecx.forced_ambiguity(MaybeInfo::AMBIGUOUS)
                }
                Err(err) => Err(err)
              }

Without hack, just propagate error (but this cause fudge to normalize and make obligation part of diag)

            return match candidate {
                Ok(candidate) if has_only_region_constraints(candidate.result) => Ok(candidate),
                Ok(_) => ecx.forced_ambiguity(MaybeInfo::AMBIGUOUS),
                Err(err) => Err(err)
              }

[lcnr]

please remove the ty::Opaque branch from instantiate_constituent_tys_for_auto_trait as it should now be unreachable 🤔

View changes since the review

[Shourya742]

Done in this 2e546e0

[lcnr]

hmm, please move this entire branch into a sub function and change this function to an if else

View changes since the review

[Shourya742]

Done in this 2e546e0