-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Convert delayed_bug
s to bug
s.
#121208
Convert delayed_bug
s to bug
s.
#121208
Conversation
Some changes might have occurred in exhaustiveness checking cc @Nadrieril Some changes occurred in cc @BoxyUwU Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Type relation code was changed |
delayed_bug
s to bug
s.delayed_bug
s to bug
s.
This comment has been minimized.
This comment has been minimized.
8f01448
to
7a9d32e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should land this with at least a few of these converted to bugs. I think doing so is fine because either:
- I am fairly confident that this can be a
bug!
, the code is familiar and I believe it to be unreachable. - anything triggering that ICE should be self-contained and easy to minimize, meaning that even if it's reachable, we should be able to get a new ui test fairly quickly, which is worth it.
Went through all changes rn and I would like to push this through and merge it after handling all review comments. we still have nearly a month until the next beta cutoff, so we could merge this now.
self.dcx() | ||
.span_delayed_bug(lifetime.ident.span, "no def-id for fresh lifetime"); | ||
continue; | ||
self.dcx().span_bug(lifetime.ident.span, "no def-id for fresh lifetime"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2
@@ -628,7 +628,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> { | |||
) => { | |||
// HIR lowering sometimes doesn't catch this in erroneous | |||
// programs, so we need to use span_delayed_bug here. See #82126. | |||
self.dcx().span_delayed_bug( | |||
self.dcx().span_bug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original UI test for #82126 does not trigger this anymore, so we should get a new test if it's still reachable. Please update the comment before merge
2
@@ -316,8 +316,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> { | |||
.and(type_op::normalize::Normalize::new(ty)) | |||
.fully_perform(self.infcx, span) | |||
else { | |||
tcx.dcx().span_delayed_bug(span, format!("failed to normalize {ty:?}")); | |||
continue; | |||
tcx.dcx().span_bug(span, format!("failed to normalize {ty:?}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like this should be reachable and potentially hard to minimize, please keep this a span_delayed_bug
but maybe add a comment "this is currently not reached in any test, so it may be worth it to minimize an example triggering this".
@@ -154,8 +154,7 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |||
if argument_index + 1 >= body.local_decls.len() { | |||
self.tcx() | |||
.dcx() | |||
.span_delayed_bug(body.span, "found more normalized_input_ty than local_decls"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1
trace!("finalized opaque type {:?} to {:#?}", opaque_type_key, hidden_type.ty.kind()); | ||
if hidden_type.has_non_region_infer() { | ||
let reported = infcx.dcx().span_delayed_bug( | ||
infcx.dcx().span_bug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1
not as confident as I maybe should be, but I would really like a test for cases where this is triggered if reachable.
@@ -2016,11 +2016,10 @@ fn assemble_candidates_from_impls<'cx, 'tcx>( | |||
ImplSource::Builtin(BuiltinImplSource::TraitUpcasting { .. }, _) | |||
| ImplSource::Builtin(BuiltinImplSource::TupleUnsizing, _) => { | |||
// These traits have no associated types. | |||
selcx.tcx().dcx().span_delayed_bug( | |||
selcx.tcx().dcx().span_bug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably theoretically reachable when manually redefining their lang item 🤷 so we should keep it as a delay span bug
@@ -82,7 +82,7 @@ where | |||
let value = infcx.commit_if_ok(|_| { | |||
let ocx = ObligationCtxt::new(infcx); | |||
let value = op(&ocx).map_err(|_| { | |||
infcx.dcx().span_delayed_bug(span, format!("error performing operation: {name}")) | |||
infcx.dcx().span_bug(span, format!("error performing operation: {name}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
theoretically reachable I think, may make sense to still keep this as is to get a test 🤷 easier to just keep a span_delay_bug
here
)); | ||
infcx | ||
.dcx() | ||
.span_bug(span, format!("ambiguity processing {obligations:?} from {self:?}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's currently unreachable, but that may change going forward. Still Making it a bug seems alright to me
@@ -90,8 +90,7 @@ fn univariant_uninterned<'tcx>( | |||
let dl = cx.data_layout(); | |||
let pack = repr.pack; | |||
if pack.is_some() && repr.align.is_some() { | |||
cx.tcx.dcx().delayed_bug("struct cannot be packed and aligned"); | |||
return Err(cx.tcx.arena.alloc(LayoutError::Unknown(ty))); | |||
cx.tcx.dcx().bug("struct cannot be packed and aligned"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 2
@@ -337,7 +337,7 @@ impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for ImplTraitInAssocTypeCollector<'tcx> { | |||
.instantiate(self.0.tcx, impl_args) | |||
.visit_with(self); | |||
} else { | |||
self.0.tcx.dcx().span_delayed_bug( | |||
self.0.tcx.dcx().span_bug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷 2
r? @lcnr |
7a9d32e
to
5f00f10
Compare
Thanks for the amazingly thorough review! I think I have made all the changes you requested. @matthiaskrgr: would you mind fuzzing this for a bit before we merge? It would be good to get some extra coverage. @bors try |
[Experiment] Convert `delayed_bug`s to `bug`s. I have a suspicion that quite a few delayed bug paths are impossible to reach, so I did an experiment. I converted every `delayed_bug` to a `bug`, ran the full test suite, then converted back every `bug` that was hit. A surprising number were never hit. This is too dangerous to merge. Increased coverage (fuzzing or a crater run) would likely hit more cases. But it might be useful for people to look at and think about which paths are genuinely unreachable. r? `@ghost`
delayed_bug
s to bug
s.delayed_bug
s to bug
s.
☀️ Try build successful - checks-actions |
I've run this through roughly 850000 files x 6 different sets of rustcflags on the dev desktop server and didn't see any crashes that would strike me as new 👍 |
@@ -48,8 +48,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
let to = normalize(to); | |||
trace!(?from, ?to); | |||
if from.has_non_region_infer() || to.has_non_region_infer() { | |||
// Note: this path is currently not reached in any test, so any | |||
// example that triggers this would be worth minimizing and | |||
// converting into a test. | |||
tcx.dcx().span_delayed_bug(span, "argument to transmute has inference variables"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not be a span_bug
? After removing the return
we now use the below code even if there are infer vars
} else { | ||
ty.needs_drop(tcx, tcx.param_env(item.owner_id)) | ||
} | ||
assert!(!ty.has_infer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert!(!ty.has_infer()); |
@@ -172,6 +172,7 @@ impl<'cx, 'tcx> VerifyBoundCx<'cx, 'tcx> { | |||
self.bound_from_components(components, visited) | |||
} | |||
Component::UnresolvedInferenceVariable(v) => { | |||
// njn: comment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment?
format!("unexpected inference var {b:?}",), | ||
); | ||
Ok(a) | ||
self.infcx.dcx().bug(format!("unexpected inference var {b:?}")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bug
vs span_bug
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want me to use span_bug
? What span would I use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.delegate.span()
? the same we previously used in the span_delayed_bug
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I removed that because of this comment. Maybe I misinterpreted what you were asking for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah xd, yeah 😁 i messed that up, didn't look at the context in my second review 😅 sorry
@@ -42,7 +42,7 @@ fn opt_span_bug_fmt<S: Into<MultiSpan>>( | |||
/// delayed bug, so what is the point of this? It exists to help us test the interaction of delayed | |||
/// bugs with the query system and incremental. | |||
pub fn trigger_delayed_bug(tcx: TyCtxt<'_>, key: rustc_hir::def_id::DefId) { | |||
tcx.dcx().span_delayed_bug( | |||
tcx.dcx().span_bug( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs to get converted back
.size; | ||
let width = tcx | ||
.layout_of(param_ty) | ||
.expect(&format!("couldn't compute width of literal: {:?}", lit_input.lit)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now eagerly formats even in the success case, which is unnecessary work and may negatively impact perf
.size; | ||
let width = tcx | ||
.layout_of(param_ty) | ||
.expect(&format!("couldn't compute width of literal: {:?}", lit_input.lit)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
compiler/rustc_resolve/src/late.rs
Outdated
@@ -3682,13 +3682,13 @@ impl<'a: 'ast, 'b, 'ast, 'tcx> LateResolutionVisitor<'a, 'b, 'ast, 'tcx> { | |||
None | |||
} | |||
Res::SelfCtor(_) => { | |||
// njn: remove comment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still relevant
// njn: ? | ||
// FIXME(generic_const_exprs): we have a `ConstKind::Expr` which is fully concrete, | ||
// but currently it is not possible to evaluate `ConstKind::Expr` so we are unable | ||
// to tell if it is evaluatable or not. For now we just ICE until this is | ||
// implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// njn: ? | |
// FIXME(generic_const_exprs): we have a `ConstKind::Expr` which is fully concrete, | |
// but currently it is not possible to evaluate `ConstKind::Expr` so we are unable | |
// to tell if it is evaluatable or not. For now we just ICE until this is | |
// implemented. | |
// FIXME(generic_const_exprs): we have a fully concrete `ConstKind::Expr`, | |
// but haven't implemented evaluating `ConstKind::Expr` yet, so we | |
// are unable to tell if it is evaluatable or not. As this is | |
// unreachable for now, we can simple ICE here. |
5f00f10
to
83fa4f6
Compare
@lcnr: Ok, I think I've addressed all the new comments, except for this one. @matthiaskrgr: thank you for the fuzzing! That's very helpful. |
r=me after addressing the last comment |
I have a suspicion that quite a few delayed bug paths are impossible to reach, so I did an experiment. I converted every `delayed_bug` to a `bug`, ran the full test suite, then converted back every `bug` that was hit. A surprising number were never hit. The next commit will convert some more back, based on human judgment.
This commit undoes some of the previous commit's mechanical changes, based on human judgment.
83fa4f6
to
2903bbb
Compare
@bors r=lcnr |
Rollup merge of rust-lang#121480 - nnethercote:fix-more-121208-fallout, r=lcnr Fix more rust-lang#121208 fallout rust-lang#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing. r? `@lcnr`
Rollup merge of rust-lang#121434 - nnethercote:fix-121208-fallout, r=lcnr Fix rust-lang#121208 fallout rust-lang#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing. r? `@lcnr`
…allout, r=lcnr Fix more rust-lang#121208 fallout (round 3) rust-lang#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing. r? `@lcnr`
…llaumeGomez Rollup of 5 pull requests Successful merges: - rust-lang#120656 (Allow tests to specify a `//@ filecheck-flags:` header) - rust-lang#120840 (Split Diagnostics for Uncommon Codepoints: Add Individual Identifier Types) - rust-lang#121554 (Don't unnecessarily change `SIGPIPE` disposition in `unix_sigpipe` tests) - rust-lang#121590 (Correctly handle if rustdoc JS script hash changed) - rust-lang#121620 (Fix more rust-lang#121208 fallout (round 3)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121620 - nnethercote:fix-even-more-121208-fallout, r=lcnr Fix more rust-lang#121208 fallout (round 3) rust-lang#121208 converted lots of delayed bugs to bugs. Unsurprisingly, there were a few invalid conversion found via fuzzing. r? `@lcnr`
PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test.
PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test. Fixes rust-lang#126385.
PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test. Fixes rust-lang#126385.
PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test. Fixes rust-lang#126385.
Convert a `span_bug` to a `span_delayed_bug`. PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test. Fixes rust-lang#126385. r? `@lcnr`
Rollup merge of rust-lang#126570 - nnethercote:fix-126385, r=lcnr Convert a `span_bug` to a `span_delayed_bug`. PR rust-lang#121208 converted this from a `span_delayed_bug` to a `span_bug` because nothing in the test suite caused execution to hit this path. But now fuzzing has found a test case that does hit it. So this commit converts it back to `span_delayed_bug` and adds the relevant test. Fixes rust-lang#126385. r? `@lcnr`
For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a target tested case.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc `@nnethercote` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc ``@nnethercote`` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc ```@nnethercote``` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc ````@nnethercote```` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc `````@nnethercote````` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc `@nnethercote` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
…-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc ``@nnethercote`` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
Rollup merge of rust-lang#133286 - jieyouxu:bug-ourselves, r=compiler-errors Re-delay a resolve `bug` related to `Self`-ctor in patterns For the code pattern reported in <rust-lang#133272>, ```rs impl Foo { fn fun() { let S { ref Self } = todo!(); } } ``` <rust-lang#121208> converted this to a `span_bug` from a `span_delayed_bug` because this specific self-ctor code pattern lacked test coverage. It turns out this can be hit but we just lacked test coverage, so change it back to a `span_delayed_bug` and add a targeted test case. Follow-up to rust-lang#121208, cc ``@nnethercote`` (very good exercise to expose our test coverage gaps). Fixes rust-lang#133272.
I have a suspicion that quite a few delayed bug paths are impossible to reach, so I did an experiment.
I converted every
delayed_bug
to abug
, ran the full test suite, then converted back everybug
that was hit. A surprising number were never hit.This is too dangerous to merge. Increased coverage (fuzzing or a crater run) would likely hit more cases. But it might be useful for people to look at and think about which paths are genuinely unreachable.
r? @ghost