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

Only check outlives goals on impl compared to trait #109356

Merged
merged 1 commit into from
Aug 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 39 additions & 12 deletions compiler/rustc_hir_analysis/src/check/compare_impl_item.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::potentially_plural_count;
use crate::errors::LifetimesOrBoundsMismatchOnTrait;
use hir::def_id::{DefId, LocalDefId};
use rustc_data_structures::fx::{FxHashMap, FxIndexSet};
use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexSet};
use rustc_errors::{
pluralize, struct_span_err, Applicability, DiagnosticId, ErrorGuaranteed, MultiSpan,
};
Expand Down Expand Up @@ -265,7 +265,6 @@ fn compare_method_predicate_entailment<'tcx>(
infer::HigherRankedType,
tcx.fn_sig(impl_m.def_id).instantiate_identity(),
);
let unnormalized_impl_fty = Ty::new_fn_ptr(tcx, ty::Binder::dummy(unnormalized_impl_sig));

let norm_cause = ObligationCause::misc(impl_m_span, impl_m_def_id);
let impl_sig = ocx.normalize(&norm_cause, param_env, unnormalized_impl_sig);
Expand Down Expand Up @@ -309,16 +308,44 @@ fn compare_method_predicate_entailment<'tcx>(
}

if check_implied_wf == CheckImpliedWfMode::Check && !(impl_sig, trait_sig).references_error() {
// We need to check that the impl's args are well-formed given
// the hybrid param-env (impl + trait method where-clauses).
ocx.register_obligation(traits::Obligation::new(
infcx.tcx,
ObligationCause::dummy(),
param_env,
ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(
unnormalized_impl_fty.into(),
))),
));
// See #108544. Annoying, we can end up in cases where, because of winnowing,
// we pick param env candidates over a more general impl, leading to more
// stricter lifetime requirements than we would otherwise need. This can
// trigger the lint. Instead, let's only consider type outlives and
// region outlives obligations.
Copy link
Contributor

@lcnr lcnr Mar 22, 2023

Choose a reason for hiding this comment

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

this should be unsound?

for some argument to be well formed, we could have a trait obligation with a nested outlives constraint which only holds because of the implied bounds of the method?

i could imagine only checking the outlives bounds returned by https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/context/struct.TyCtxt.html#method.implied_outlives_bounds, but even that feels dangerous.

I don't think we should keep an unsoundness to patch a deficiency of the current trait solver, especially if that deficiency can be solved in a different way.

Can we instead strip trivial clauses from the trait method param env when substituting? That should solve this issue without adding any soundness concerns? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

opened #109491 to experiment with that approach

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this is unsound (though, arguably, imperfect).

We check that everything is well formed elsewhere, so there's no unsoundness related to ill-formed types.

The only implied bounds we currently have are those involving regions (or super trait bounds, but those should also be checked elsewhere).

Therefore, we don't necessarily have to care about non-region predicates for this lint.

That being said, while in theory, skipping "trivial clauses" is arguably a "nicer" solution, the implementation in #109491 is not simple. And I think that approach may also have issues when the param env predicates introduce ambiguity that doesn't exist in an empty environment. Though, I'm not sure if this would be caught by other checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is unsound (though, arguably, imperfect).

We check that everything is well formed elsewhere, so there's no unsoundness related to ill-formed types.

The only implied bounds we currently have are those involving regions (or super trait bounds, but those should also be checked elsewhere).

Therefore, we don't necessarily have to care about non-region predicates for this lint.

We do care about them if these other predicates end up causing us to add additional outlives predicates. Considering that we end up computing Projection obligations inside of compute_implied_outlives_bounds, this can be the case.

if obligation.predicate.has_non_region_infer() {
match obligation.predicate.kind().skip_binder() {
ty::PredicateKind::Clause(ty::Clause::Projection(..))
| ty::PredicateKind::AliasRelate(..) => {
ocx.register_obligation(obligation.clone());
}
_ => {}
}
}

That being said, while in theory, skipping "trivial clauses" is arguably a "nicer" solution, the implementation in #109491 is not simple.

I disagree, the implementation in #109491 is (after enabling the use of the canonical solver mode to get non-fatal overflow) simple. We're checking whether a goal holds in an empty environment. While this needs more lines of code, the behavior of these is clear and also relied on in other areas of the compiler. That approach is also far more obvious to be sound as we don't drop requirements, we only drop assumptions.

And I think that approach may also have issues when the param env predicates introduce ambiguity that doesn't exist in an empty environment. Though, I'm not sure if this would be caught by other checks.

You mean that the approach in #109491 does not completely fix the issue of incorrect ambiguity/selecting the wrong candidate when proving the wf bounds? That is true and something that can be incrementally improved if people encounter that issue .

I very strongly prefer trivially sound (given that the rest of the trait solver is sound) over maybe sound but may accept slightly more code.

I've spend a few hours looking at the code and experimenting with some tests to convince myself that your approach is sound (or to get a counter example) and wasn't able to do either of these. Apparently removing the ocx.register_obligation doesn't actually break any existing tests but an assertion that the obligation has no inference variables does fail. cc @aliemjay as you may know whether that register_obligation is currently still needed.

Copy link
Member

Choose a reason for hiding this comment

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

We do care about them if these other predicates end up causing us to add additional outlives predicates. Considering that we end up computing Projection obligations inside of compute_implied_outlives_bounds, this can be the case.

In implied_outlives_bounds query, ocx.register_obligations will add additional predicates as requirements not assumptions. i.e. the additional outlives predicates will end up in QueryResponse::region_constraints rather than QueryResponse::value.

Apparently removing the ocx.register_obligation doesn't actually break any existing tests but an assertion that the obligation has no inference variables does fail. cc @aliemjay as you may know whether that register_obligation is currently still needed.

After fiddling with it for a while I managed to come up with a test:

pub trait Iter {
    type Item;
}

impl<X, I> Iter for I
where
    I: IntoIterator<Item = X>,
{
    type Item = X;
}

pub struct Map<I>(I)
where
    I: Iter,
    I::Item: 'static;

fn test<X>(_: Map<Vec<X>>) {}

and filed an issue #109799 for why the condition predicate.has_non_region_infer() is unnecessary.

Copy link
Contributor

@lcnr lcnr Apr 20, 2023

Choose a reason for hiding this comment

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

Extending the approach in #109491 to deal with generic bounds ended up causing weird trait solver errors so I changed my opinion here. I think that - at least with the current solver - this PR is the better choice.

Please add the following as a test.

// check-pass

// Similar to issue-108544.rs except that we have a generic `T` which
// previously caused an overeager fast-path to trigger.
use std::borrow::Cow;

pub trait Trait<T: Clone> {
    fn method(self) -> Option<Cow<'static, T>>
    where
        Self: Sized;
}

impl<'a, T: Clone> Trait<T> for Cow<'a, T> {
    fn method(self) -> Option<Cow<'static, T>> {
        None
    }
}

fn main() {}

Copy link
Contributor

Choose a reason for hiding this comment

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

also please add a fixme to look into reverting this once the new solver is stable

Suggested change
// region outlives obligations.
// region outlives obligations.
//
// FIXME(-Ztrait-solver=next): Try removing this hack again once
// the new solver is stable.

//
// FIXME(-Ztrait-solver=next): Try removing this hack again once
// the new solver is stable.
let mut wf_args: smallvec::SmallVec<[_; 4]> =
unnormalized_impl_sig.inputs_and_output.iter().map(|ty| ty.into()).collect();
// Annoyingly, asking for the WF predicates of an array (with an unevaluated const (only?))
// will give back the well-formed predicate of the same array.
let mut wf_args_seen: FxHashSet<_> = wf_args.iter().copied().collect();
while let Some(arg) = wf_args.pop() {
let Some(obligations) = rustc_trait_selection::traits::wf::obligations(
infcx,
param_env,
impl_m_def_id,
0,
arg,
impl_m_span,
) else {
continue;
};
for obligation in obligations {
match obligation.predicate.kind().skip_binder() {
ty::PredicateKind::Clause(
ty::ClauseKind::RegionOutlives(..) | ty::ClauseKind::TypeOutlives(..),
) => ocx.register_obligation(obligation),
ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(arg)) => {
if wf_args_seen.insert(arg) {
wf_args.push(arg)
}
}
_ => {}
}
}
}
}

// Check that all obligations are satisfied by the implementation's
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// check-pass
// See issue #109356. We don't want a false positive to the `implied_bounds_entailment` lint.

use std::borrow::Cow;

pub trait Trait {
fn method(self) -> Option<Cow<'static, str>>
where
Self: Sized;
}

impl<'a> Trait for Cow<'a, str> {
// If we're not careful here, we'll check `WF(return-type)` using the trait
// and impl where clauses, requiring that `Cow<'a, str>: Sized`. This is
// obviously true, but if we pick the `Self: Sized` clause from the trait
// over the "inherent impl", we will require `'a == 'static`, which triggers
// the `implied_bounds_entailment` lint.
fn method(self) -> Option<Cow<'static, str>> {
None
}
}

fn main() {}
15 changes: 15 additions & 0 deletions tests/ui/implied-bounds/trait-where-clause-implied.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// check-pass

pub trait Trait<'a, 'b> {
fn method(self, _: &'static &'static ())
where
'a: 'b;
}

impl<'a> Trait<'a, 'static> for () {
// On first glance, this seems like we have the extra implied bound that
// `'a: 'static`, but we know this from the trait method where clause.
fn method(self, _: &'static &'a ()) {}
}

fn main() {}