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

Update to fix regression 90319 and correctly emit overflow errors when not inside an error reporting context #91238

Closed
wants to merge 1 commit into from
Closed
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
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,9 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> {
Ok(result) => result,
Err(OverflowError::Canonical) => {
let mut selcx = SelectionContext::with_query_mode(&self, TraitQueryMode::Standard);
if self.is_tainted_by_errors() {
selcx.is_suggestion(true)
}
selcx.evaluate_root_obligation(obligation).unwrap_or_else(|r| match r {
OverflowError::Canonical => {
span_bug!(
Expand Down
32 changes: 31 additions & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ pub struct SelectionContext<'cx, 'tcx> {
/// policy. In essence, canonicalized queries need their errors propagated
/// rather than immediately reported because we do not have accurate spans.
query_mode: TraitQueryMode,

/// Are we in a selection context to try to make a suggestion for error reporting
/// and we don't want to emit errors until we are complete (Overflow)
is_suggestion: bool,
}

// A stack that walks back up the stack frame.
Expand Down Expand Up @@ -224,6 +228,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
allow_negative_impls: false,
is_in_const_context: false,
query_mode: TraitQueryMode::Standard,
is_suggestion: false,
}
}

Expand All @@ -236,6 +241,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
allow_negative_impls: false,
is_in_const_context: false,
query_mode: TraitQueryMode::Standard,
is_suggestion: false,
}
}

Expand All @@ -252,6 +258,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
allow_negative_impls,
is_in_const_context: false,
query_mode: TraitQueryMode::Standard,
is_suggestion: false,
}
}

Expand All @@ -268,6 +275,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
allow_negative_impls: false,
is_in_const_context: false,
query_mode,
is_suggestion: false,
}
}

Expand All @@ -283,9 +291,31 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
allow_negative_impls: false,
is_in_const_context: matches!(constness, hir::Constness::Const),
query_mode: TraitQueryMode::Standard,
is_suggestion: false,
}
}

pub fn with_suggestion(
infcx: &'cx InferCtxt<'cx, 'tcx>,
suggestion: bool,
) -> SelectionContext<'cx, 'tcx> {
SelectionContext {
infcx,
freshener: infcx.freshener_keep_static(),
intercrate: false,
intercrate_ambiguity_causes: None,
allow_negative_impls: false,
is_in_const_context: false,
query_mode: TraitQueryMode::Standard,
is_suggestion: suggestion,
}
}

/// Enable suggestion mode for use during error reporting selection contexts
/// to prevent reporting overflow errors during method suggestions
pub fn is_suggestion(&mut self, suggestion: bool) {
self.is_suggestion = suggestion;
}
/// Enables tracking of intercrate ambiguity causes. These are
/// used in coherence to give improved diagnostics. We don't do
/// this until we detect a coherence error because it can lead to
Expand Down Expand Up @@ -1093,7 +1123,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
if !self.infcx.tcx.recursion_limit().value_within_limit(depth) {
match self.query_mode {
TraitQueryMode::Standard => {
if self.infcx.is_tainted_by_errors() {
if self.is_suggestion {
return Err(OverflowError::ErrorReporting);
}
self.infcx.report_overflow_error(error_obligation, true);
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_typeck/src/check/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Err(e) => e,
};

self.set_tainted_by_errors();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually moved it to the top of probe_for_return in the probe module as having it in this location meant that it was being hit when we weren't in an error context during trait bound fulfillment.

Moving it to the new location meant it only got set when we were actively looking for a method suggestion in an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does keeping this call to set_tainted_by_errors cause any undesired effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tried it but can test it out - I think you're right that it can be kept there, I'm fairly sure that both spot I've tried it in are in error reporting pathways anyway.

I think the bigger issue is that set_tainted_by_errors is being called elsewhere before this point due to name resolution being unable to resolve Thing

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we try getting this back in to see the effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied in the wrong place - see my comment on the other review but I'm going to push a change tomorrow that I think works better

let expr = expr.peel_drop_temps();
let cause = self.misc(expr.span);
let expr_ty = self.resolve_vars_with_obligations(checked_ty);
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_typeck/src/check/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"probe(self_ty={:?}, return_type={}, scope_expr_id={})",
self_ty, return_type, scope_expr_id
);
self.set_tainted_by_errors();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estebank I moved it here because we only hit it when we're in an error reporting context here as probe_for_return_type is only used to generate suggestions whereas in the old location we could hit the set_tainted_by_errors call when we weren't doing error reporting

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the code in demand_coerce_diag once we move after the try_coerce we are always in an error reporting pathway.

Copy link
Contributor Author

@tom7980 tom7980 Dec 9, 2021

Choose a reason for hiding this comment

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

Yes you are correct - I think either location is fine but if you prefer it to be here then I can move it. I think however that we probably need a new flag to suppress errors that happen in the same pass, as is_tainted_by_errors can be set before here between name resolution and type checking which is what is causing the ICE currently as the check before the overflow error is for is_tainted_by_errors()

let method_names = self
.probe_op(
span,
Expand Down Expand Up @@ -1469,7 +1470,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
let cause = traits::ObligationCause::misc(self.span, self.body_id);
let predicate = ty::Binder::dummy(trait_ref).to_poly_trait_predicate();
let obligation = traits::Obligation::new(cause, self.param_env, predicate);
traits::SelectionContext::new(self).select(&obligation)
traits::SelectionContext::with_suggestion(self, self.is_suggestion.0).select(&obligation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't modifying the selection context before selecting to be tainted_by_errors also work instead of adding a new field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea so looking back at it I'm not sure I even need this new field I think I actually just needed to move where I set that we were tainted by errors. I'll see if I can change it this evening so we don't need the new selection context field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think what's happen is I wanted a way to distinguish inside the selection context itself whether we were creating a suggestion but forgetting that it derefs down into the function context that spawned it which already has the tainted_by_errors flag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a look at it this evening - it starts ICEing again if I rely on is_tainted_by_errors() check in the overflow check I expect that the function context is set as tainted_by_errors elsewhere in the typechecking stage before reporting fulfilment errors so it reports an incorrect overflow error.

I need to confirm this first however so will do some more digging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can say for sure that without the new field in the selection context struct if Thing is not found during resolution then it is resolved to a Res:Err which causes set_tainted_by_errors() to be called before we go into the fulfilment context. I'm having second thoughts about relying on is_tainted_by_errors() anywhere because it may have even been set before I do it myself and I need to know for sure at the overflow error site whether I should be emitting an error or propagating one back up the call stack.

It may be better to add the field to the infer_ctx struct to set instead of tainting it with errors so that I can be confident that I set it and can handle the overflow errors correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a reasonable avenue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put it back however in my other implementation of this I don't rely on it anymore instead preferring to use a flag in the inferctx, I've linked my other implementation in the PR at the bottom which I think is more robust

}

fn candidate_source(&self, candidate: &Candidate<'tcx>, self_ty: Ty<'tcx>) -> CandidateSource {
Expand Down Expand Up @@ -1521,7 +1522,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
let mut xform_ret_ty = probe.xform_ret_ty;
debug!(?xform_ret_ty);

let selcx = &mut traits::SelectionContext::new(self);
let selcx = &mut traits::SelectionContext::with_suggestion(self, self.is_suggestion.0);
let cause = traits::ObligationCause::misc(self.span, self.body_id);

// If so, impls may carry other conditions (e.g., where
Expand Down
17 changes: 17 additions & 0 deletions src/test/ui/typeck/issue-90319.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
struct Wrapper<T>(T);

trait Trait {
fn method(&self) {}
}

impl<'a, T> Trait for Wrapper<&'a T> where Wrapper<T>: Trait {}

fn get<T>() -> T {
unimplemented!()
}

fn main() {
let thing = get::<Thing>();//~ERROR 14:23: 14:28: cannot find type `Thing` in this scope [E0412]
let wrapper = Wrapper(thing);
Trait::method(&wrapper);//~ERROR 16:5: 16:18: overflow evaluating the requirement `_: Sized` [E0275]
}
25 changes: 25 additions & 0 deletions src/test/ui/typeck/issue-90319.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
error[E0412]: cannot find type `Thing` in this scope
--> $DIR/issue-90319.rs:14:23
|
LL | let thing = get::<Thing>();
| ^^^^^ not found in this scope

error[E0275]: overflow evaluating the requirement `_: Sized`
--> $DIR/issue-90319.rs:16:5
|
LL | Trait::method(&wrapper);
| ^^^^^^^^^^^^^
|
= help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`issue_90319`)
note: required because of the requirements on the impl of `Trait` for `Wrapper<&_>`
--> $DIR/issue-90319.rs:7:13
|
LL | impl<'a, T> Trait for Wrapper<&'a T> where Wrapper<T>: Trait {}
| ^^^^^ ^^^^^^^^^^^^^^
= note: 128 redundant requirements hidden
= note: required because of the requirements on the impl of `Trait` for `Wrapper<&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&&_>`

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0275, E0412.
For more information about an error, try `rustc --explain E0275`.