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

tom7980 · 2021-11-25T23:16:15Z

This fixes #90319 to correctly report overflow errors instead of an ICE while also keeping the functionality of not reporting excess overflow errors during error reporting and method suggestions.

cc: @Mark-Simulacrum @estebank @dtolnay

rust-highfive · 2021-11-25T23:16:18Z

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

bors · 2021-12-02T15:26:11Z

☔ The latest upstream changes (presumably #91354) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2021-12-03T20:04:38Z

☔ The latest upstream changes (presumably #91491) made this pull request unmergeable. Please resolve the merge conflicts.

tom7980 · 2021-12-03T20:39:46Z

r? @Mark-Simulacrum

Not sure if Matt is reviewing PR's but this one keeps hitting merge conflicts so would like to merge it before I have to rebase it again haha

…n not in suggestion context Remove reintrod with_constness() selection context Update to fix regression 90319 and correctly emit overflow errors tidy run Update to fix regression 90319 and correctly emit overflow errors

Mark-Simulacrum · 2021-12-07T14:21:38Z

r? @jackh726 perhaps? I'm not familiar enough with this code I think to review this well.

jackh726 · 2021-12-07T17:15:33Z

I remember seeing #89576, but not thinking too much about it.

Thinking about this a bit, I'm wondering if the fix applied there and here are not the best fix. I'm wondering if a better fix would be to delay omitting all overflow errors. As shown in #91430, there are cases where we don't really need to emit overflow errors — though, we need to be really careful that we don't accidently not emit them when we should.

I'll look a little bit closer at this PR to think about if this really a band-aid fix we want to pursue right now.

tom7980 · 2021-12-07T18:11:39Z

I think you're right that we still want to emit overflow errors when we should, perhaps it should be a delay_span_bug instead so that we still emit overflow if we get to the end of typechecking however I'm not sure if the reason we emit overflow errors unconditionally at the moment is that there are times where if we don't then the compiler hangs.

Currently I know of the two paths to get to this error and it's during a probe_op to suggest conversion methods where all errors are ignored because we're only interested in whether the method does the conversion for an error suggestion.

Then there is the second path here which happens during typechecking trait bounds where honestly we do want to emit the overflow error because the trait is overflowing without a concrete implementation for the trait on the type you're calling the method on so it'll never compile anyway.

Let me know what you think, you could well be right that we're being over eager on overflow errors but I think they are one error that perhaps we should be due to the nature of them making code hit infinite loops

estebank

Great on fixing the ICE! But I have a couple of nitpicks :)

estebank · 2021-12-08T22:55:46Z

compiler/rustc_typeck/src/check/demand.rs

@@ -143,7 +143,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
            Err(e) => e,
        };

-        self.set_tainted_by_errors();


Why is this removed?

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.

Does keeping this call to set_tainted_by_errors cause any undesired effect?

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

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

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

estebank · 2021-12-08T22:57:16Z

compiler/rustc_typeck/src/check/method/probe.rs

@@ -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)


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

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.

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

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.

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

That sounds like a reasonable avenue.

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

tom7980 · 2021-12-09T09:23:31Z

compiler/rustc_typeck/src/check/method/probe.rs

@@ -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();


@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

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.

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()

tom7980 · 2021-12-09T22:25:35Z

Is there a good way to push a commit as a second option for a change? I did some work on this tonight as I don't really like having to rely on is_tainted_by_errors() due to it being set between passes (resolution then type checking) and I would like to push the changes so people can decide which option work better.

estebank · 2021-12-10T03:04:13Z

You can push to another branch and I think there's a way to see a diff between two branches on GH's ui.

tom7980 · 2021-12-10T17:20:46Z

Okay I've created a new branch with the changes I'd prefer to make - I think the changes to the InferCtx are better because set_tainted_by_errors is likely to be set earlier between passes i.e. in this case name resolution and type checking, whereas I want to be able to set a flag when we error in the middle of a pass and need to supress new ones.

Let me know what you think, I personally believe this is more robust & can be easily lifted out if we decide to change how we handle overflow errors in type checking.

Here is the diff:
tom7980/rust@issue-90319-fix...tom7980:issue-90319-fix-2

bors · 2021-12-13T01:25:12Z

☔ The latest upstream changes (presumably #91549) made this pull request unmergeable. Please resolve the merge conflicts.

jackh726 · 2021-12-18T00:40:30Z

Going to go ahead and r? @estebank, since they reviewed the previous PR and have done a partial review here.

I do think changing how we propagate overflow errors is something work experimenting with, but that's probably a bigger task and not worth blocking this.

Dylan-DPC · 2022-02-27T05:35:55Z

closing this based on comment by author #94391 (comment)

rust-highfive assigned matthewjasper Nov 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2021

tom7980 force-pushed the issue-90319-fix branch from 90e3314 to 2f18023 Compare December 2, 2021 17:12

This comment has been minimized.

Sign in to view

tom7980 force-pushed the issue-90319-fix branch 2 times, most recently from 75387e7 to 6322948 Compare December 3, 2021 20:38

rust-highfive assigned Mark-Simulacrum and unassigned matthewjasper Dec 3, 2021

This comment has been minimized.

Sign in to view

tom7980 force-pushed the issue-90319-fix branch from 6322948 to 930df17 Compare December 3, 2021 20:57

rust-highfive assigned jackh726 and unassigned Mark-Simulacrum Dec 7, 2021

estebank reviewed Dec 8, 2021

View reviewed changes

tom7980 commented Dec 9, 2021

View reviewed changes

rust-highfive assigned estebank and unassigned jackh726 Dec 18, 2021

compiler-errors mentioned this pull request Dec 24, 2021

ICE - ErrorReporting Overflow should not reach report_selection_err call #92148

Closed

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2022

estebank added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 17, 2022

This was referenced Feb 26, 2022

regression: ICE ErrorReporting Overflow should not reach report_selection_err call #90319

Closed

Fix ice when error reporting recursion errors #94391

Merged

Dylan-DPC closed this Feb 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tom7980 commented Nov 25, 2021

rust-highfive commented Nov 25, 2021

bors commented Dec 2, 2021

This comment has been minimized.

bors commented Dec 3, 2021

tom7980 commented Dec 3, 2021

This comment has been minimized.

Mark-Simulacrum commented Dec 7, 2021

jackh726 commented Dec 7, 2021

tom7980 commented Dec 7, 2021

estebank left a comment

estebank Dec 8, 2021

tom7980 Dec 9, 2021

estebank Dec 9, 2021

tom7980 Dec 9, 2021

estebank Feb 17, 2022

tom7980 Feb 18, 2022

estebank Dec 8, 2021

tom7980 Dec 9, 2021

tom7980 Dec 9, 2021

tom7980 Dec 9, 2021

tom7980 Dec 9, 2021

estebank Feb 17, 2022

tom7980 Feb 17, 2022

tom7980 Dec 9, 2021

estebank Dec 9, 2021

tom7980 Dec 9, 2021 •

edited

Loading

tom7980 commented Dec 9, 2021

estebank commented Dec 10, 2021

tom7980 commented Dec 10, 2021 •

edited

Loading

bors commented Dec 13, 2021

jackh726 commented Dec 18, 2021

Dylan-DPC commented Feb 27, 2022

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

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

Conversation

tom7980 commented Nov 25, 2021

rust-highfive commented Nov 25, 2021

bors commented Dec 2, 2021

This comment has been minimized.

bors commented Dec 3, 2021

tom7980 commented Dec 3, 2021

This comment has been minimized.

Mark-Simulacrum commented Dec 7, 2021

jackh726 commented Dec 7, 2021

tom7980 commented Dec 7, 2021

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tom7980 Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

tom7980 commented Dec 9, 2021

estebank commented Dec 10, 2021

tom7980 commented Dec 10, 2021 • edited Loading

bors commented Dec 13, 2021

jackh726 commented Dec 18, 2021

Dylan-DPC commented Feb 27, 2022

tom7980 Dec 9, 2021 •

edited

Loading

tom7980 commented Dec 10, 2021 •

edited

Loading