Add ConstKind::Error and convert ErrorHandled::Reported to it. by eddyb · Pull Request #71049 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation49 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
By replicating the ty::Error approach to encoding "an error has occurred", all of the mechanisms that skip redundant/downstream errors are engaged and help out (see the reduction in test output).
This PR also adds ErrorHandled::Linted for the lint case because using ErrorHandled::Reported without having emitted an error that is guaranteed to stop compilation, is incorrect now.
r? @oli-obk cc @rust-lang/wg-const-eval @varkor @yodaldevoid
This comment has been minimized.
This PR also adds ErrorHandled::Linted for the lint case because using ErrorHandled::Reported without having emitted an error that is guaranteed to stop compilation, is incorrect now.
Should there be a strongly worded comment somewhere to ensure this? What is the failure mode if we get this wrong (i.e., do we have a safety net that makes this ICE instead of silently doing the wrong thing)?
Should there be a strongly worded comment somewhere to ensure this? What is the failure mode if we get this wrong (i.e., do we have a safety net that makes this ICE instead of silently doing the wrong thing)?
@mark-i-m has a series of PRs where using tcx.types.err is replaced with a wrapper that uses delay_span_bug and they're finding all sorts of places in the compiler that abused error types.
And if we do #69426, we can have ErrorHandled::Reported hold an ErrorReported token.
Actually, let me do that now, so it's not forgotten.
@RalfJung Implemented the more explicit route and added a comment, let me know if it's better.
@varkor I won't remove it but F-const_generics may be misleading, this is useful with regular array lengths too (in fact, most of the test that have improved error output are like that).
@eddyb: that label is simply used to track what PRs have made progress on any issue related to const generics, even if the primary purpose is not necessarily for const generics. At least, that's how I've been using it :)
| if !x.val.needs_infer() { |
|---|
| return x.eval(tcx, relation.param_env()).val; |
| } |
| x.val |
| }; |
| // FIXME(eddyb) doesn't look like everything below checks that `a.ty == b.ty`. |
| // We could probably always assert it early, as `const` generic parameters |
| // are not allowed to depend on other generic parameters, i.e. are concrete. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this temporary and won't always remain so, so let's not bake that assumption too deeply into the compiler.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have already, this is nothing compared to everything else. It's only "temporary" in the sense that the first thing we stabilize won't have generic parameters in const parameter types.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the problem is we'd need to stabilize something like StructuralMatch (some deep trait, unlike StructuralPartialEq/StructuralEq), because we can't just allow unrestricted types.
But even with that also it's a PITA to implement correctly, far more so than just concrete types.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure I understand all of this; I'm just saying we should avoid too deep of an architectural dependence on the fact that const params can't depend on type params so that it doesn't take a year to dig out of.
| err_inval!(TypeckError) => return Err(ErrorHandled::Reported), |
|---|
| err_inval!(TypeckError(error_reported)) => { |
| return Err(ErrorHandled::Reported(error_reported)); |
| } |
| // We must *always* hard error on these, even if the caller wants just a lint. |
| err_inval!(Layout(LayoutError::SizeOverflow(_))) => true, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this case doesn't early return so it results in Ok(_) and then report_as_lint will turn it into ErrorReported::Linted, which seems wrong?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function unfortunately became a mess over time. :/ I get the feeling we are trying to shove too many different things into one code path here.
I also do not have the slightest clue what that replace_span_with business in report_as_lint is even doing.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But SizeOverflow will be a hard error even when report_as_lint is called, so always returning Linted there seems wrong, superficially.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function unfortunately became a mess over time. :/ I get the feeling we are trying to shove too many different things into one code path here.
My thoughts exactly.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, so the must_error variable should be eliminated in favour of early returning here by calling report_as_error
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept must_error but replaced Result<(), ErrorHandled> in the return type with ErrorHandled, let me know if that's enough.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that works, too, lgtm
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoiding that boolean could have made the code cleaner... but maybe not, and anyway we can leave that to later.
| @@ -90,7 +81,7 @@ impl<'tcx> ConstEvalErr<'tcx> { |
|---|
| pub fn report_as_error(&self, tcx: TyCtxtAt<'tcx>, message: &str) -> ErrorHandled { |
| match self.struct_error(tcx, message, |mut e |
| Ok(_) => ErrorHandled::Reported, |
| Ok(_) => ErrorHandled::Reported(ErrorReported), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is still seems rather easy to just say ErrorReported when the compiler says that that is the type you need. Maybe the only way to construct this should be something like ErrorReported::assert_error_reported_to_user or so? Could that constructor do delay_bug just to be extra safe or does that not make sense?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung We're working on that but it's not ready yet because of some abuse in the compiler (of ty::Error which we wanted to add an ErrorReported to).
I tried to make this PR not blocked on that, but write ErrorReported or error_reported everywhere relevant so e.g. @mark-i-m can find it later.
This is mostly because I only did this PR as a prerequisite for #70970.
I am fine with blocking this PR on @mark-i-m's work if fixing #70942 is not a priority.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eddyb sure, if improving this is on your list then that's fine for me. I was just pointing out that as the final solution this still seems suboptimal.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just pointing out that as the final solution this still seems suboptimal.
Indeed. Also, I should've pointed at #69426.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this PR will definitely make my next PR easier.
☔ The latest upstream changes (presumably #70452) made this pull request unmergeable. Please resolve the merge conflicts.
This (partially) reverts commit f47c4ff.
@bors r=oli-obk rollup=never
📌 Commit 77f38dc has been approved by oli-obk
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
This was referenced
Apr 17, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request
…-obk
Detect mistyped associated consts in Instance::resolve.
Based on rust-lang#71049 to prevent redundant/misleading downstream errors.
Fixes rust-lang#70942 by refusing to resolve an associated const if it doesn't have the same type in the impl that it does in the trait (which we assume had errored, and delay_span_bug guards against bugs).