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.

Sign up for GitHub

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

remove EvaluationResult::EvaluatedTo{Unknown,Recur} #114249

Closed
wants to merge 1 commit into from
Closed

remove EvaluationResult::EvaluatedTo{Unknown,Recur} #114249

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
79 changes: 2 additions & 77 deletions compiler/rustc_middle/src/traits/select.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,7 @@ pub enum SelectionCandidate<'tcx> {
///
/// The evaluation results are ordered:
/// - `EvaluatedToOk` implies `EvaluatedToOkModuloRegions`
/// implies `EvaluatedToAmbig` implies `EvaluatedToUnknown`
/// - `EvaluatedToErr` implies `EvaluatedToRecur`
/// implies `EvaluatedToAmbig`
/// - the "union" of evaluation results is equal to their maximum -
/// all the "potential success" candidates can potentially succeed,
/// so they are noops when unioned with a definite error, and within
Expand All @@ -193,61 +192,7 @@ pub enum EvaluationResult {
EvaluatedToOkModuloOpaqueTypes,
/// Evaluation is known to be ambiguous -- it *might* hold for some
/// assignment of inference variables, but it might not.
///
/// While this has the same meaning as `EvaluatedToUnknown` -- we can't
/// know whether this obligation holds or not -- it is the result we
/// would get with an empty stack, and therefore is cacheable.
EvaluatedToAmbig,
/// Evaluation failed because of recursion involving inference
/// variables. We are somewhat imprecise there, so we don't actually
/// know the real result.
///
/// This can't be trivially cached for the same reason as `EvaluatedToRecur`.
EvaluatedToUnknown,
/// Evaluation failed because we encountered an obligation we are already
/// trying to prove on this branch.
///
/// We know this branch can't be a part of a minimal proof-tree for
/// the "root" of our cycle, because then we could cut out the recursion
/// and maintain a valid proof tree. However, this does not mean
/// that all the obligations on this branch do not hold -- it's possible
/// that we entered this branch "speculatively", and that there
/// might be some other way to prove this obligation that does not
/// go through this cycle -- so we can't cache this as a failure.
///
/// For example, suppose we have this:
///
/// ```rust,ignore (pseudo-Rust)
/// pub trait Trait { fn xyz(); }
/// // This impl is "useless", but we can still have
/// // an `impl Trait for SomeUnsizedType` somewhere.
/// impl<T: Trait + Sized> Trait for T { fn xyz() {} }
///
/// pub fn foo<T: Trait + ?Sized>() {
/// <T as Trait>::xyz();
/// }
/// ```
///
/// When checking `foo`, we have to prove `T: Trait`. This basically
/// translates into this:
///
/// ```plain,ignore
/// (T: Trait + Sized →_\impl T: Trait), T: Trait ⊢ T: Trait
/// ```
///
/// When we try to prove it, we first go the first option, which
/// recurses. This shows us that the impl is "useless" -- it won't
/// tell us that `T: Trait` unless it already implemented `Trait`
/// by some other means. However, that does not prevent `T: Trait`
/// does not hold, because of the bound (which can indeed be satisfied
/// by `SomeUnsizedType` from another crate).
//
// FIXME: when an `EvaluatedToRecur` goes past its parent root, we
// ought to convert it to an `EvaluatedToErr`, because we know
// there definitely isn't a proof tree for that obligation. Not
// doing so is still sound -- there isn't any proof tree, so the
// branch still can't be a part of a minimal one -- but does not re-enable caching.
EvaluatedToRecur,
/// Evaluation failed.
EvaluatedToErr,
}
Expand All @@ -266,27 +211,7 @@ impl EvaluationResult {
}

pub fn may_apply(self) -> bool {
match self {
EvaluatedToOkModuloOpaqueTypes
| EvaluatedToOk
| EvaluatedToOkModuloRegions
| EvaluatedToAmbig
| EvaluatedToUnknown => true,

EvaluatedToErr | EvaluatedToRecur => false,
}
}

pub fn is_stack_dependent(self) -> bool {
match self {
EvaluatedToUnknown | EvaluatedToRecur => true,

EvaluatedToOkModuloOpaqueTypes
| EvaluatedToOk
| EvaluatedToOkModuloRegions
| EvaluatedToAmbig
| EvaluatedToErr => false,
}
self != EvaluatedToErr
}
}

Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_trait_selection/src/traits/query/evaluate_obligation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> {
let mut fulfill_cx = crate::solve::FulfillmentCtxt::new(self);
fulfill_cx.register_predicate_obligation(self, obligation.clone());
// True errors
// FIXME(-Ztrait-solver=next): Overflows are reported as ambig here, is that OK?
if !fulfill_cx.select_where_possible(self).is_empty() {
Ok(EvaluationResult::EvaluatedToErr)
} else if !fulfill_cx.select_all_or_error(self).is_empty() {
Expand Down
14 changes: 4 additions & 10 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
stack.update_reached_depth(stack_arg.1);
return Ok(EvaluatedToOk);
} else {
return Ok(EvaluatedToRecur);
return Ok(EvaluatedToErr);
}
}
return Ok(EvaluatedToOk);
Expand Down Expand Up @@ -838,7 +838,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
}
ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig),
ProjectAndUnifyResult::Recursive => Ok(EvaluatedToRecur),
ProjectAndUnifyResult::Recursive => Ok(EvaluatedToErr),
ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr),
}
}
Expand Down Expand Up @@ -1157,7 +1157,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
Some(EvaluatedToOk)
} else {
debug!("evaluate_stack --> recursive, inductive");
Some(EvaluatedToRecur)
Some(EvaluatedToErr)
}
} else {
None
Expand Down Expand Up @@ -1207,7 +1207,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
})
{
debug!("evaluate_stack --> unbound argument, recursive --> giving up",);
return Ok(EvaluatedToUnknown);
return Ok(EvaluatedToAmbig);
}

match self.candidate_from_obligation(stack) {
Expand Down Expand Up @@ -1306,12 +1306,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
dep_node: DepNodeIndex,
result: EvaluationResult,
) {
// Avoid caching results that depend on more than just the trait-ref
// - the stack can create recursion.
if result.is_stack_dependent() {
return;
Copy link
Member

Choose a reason for hiding this comment

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

How is it sound to remove this? Aren't we caching things that should not be cached?

}

// Neither the global nor local cache is aware of intercrate
// mode, so don't do any caching. In particular, we might
// re-use the same `InferCtxt` with both an intercrate
Expand Down