mgca: Propagate errors in const-to-valtree lowering#158401
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
r? @camelid rustbot has assigned @camelid. Use Why was this reviewer chosen?The reviewer was selected based on:
|
camelid
added
the
A-const-generics
There was a problem hiding this comment.
The case reported in the original issue goes through lower_const_arg_struct(). I found that a similar ICE can also occur through lower_const_arg_tuple_call(), and this serves as a regression test for that case.
| let valtree = ty::ValTree::from_branches(tcx, elems); | ||
|
|
||
| ty::Const::new_value(tcx, valtree, ty) | ||
| ty::Const::new_value_from_branches(tcx, elems, ty) |
There was a problem hiding this comment.
I was not able to find a case that reproduces the similar ICE through lower_const_arg_{array, tuple}() (unlike struct/tuple_call cases I mentioned above, I think there are few situations in which these would cause an ICE under the current evaluation order 🤔 ). However, I don't think we can rule out the possibility that these paths may receive branches containing error constants in the future. Therefore, I think it's reasonable to apply the same fix to these paths as well.
There was a problem hiding this comment.
I'm not sure if this is the right place to be fixing the problem. Given that ValTrees can have arbitrary ty::Const, it feels like they should be able to have error constants in them.
@BoxyUwU do you have thoughts about the best way to handle this?
| branches: impl IntoIterator<Item = ty::Const<'tcx>>, | ||
| ty: Ty<'tcx>, | ||
| ) -> Const<'tcx> { | ||
| let branches: Vec<_> = branches.into_iter().collect(); |
There was a problem hiding this comment.
Hmm I don't love that this requires collecting the iterator, looking through all the branches, and then passing it on.
There was a problem hiding this comment.
I just saw this discussion. Please let me think about it for a while... https://rust-lang.zulipchat.com/#narrow/channel/260443-project-const-generics/topic/const.20generics.20talkies.2013.20jul.202026.201600pm/with/609875464
There was a problem hiding this comment.
No worries. The TLDR is that we should probably instead make other parts of the compiler handle errors inside valtrees gracefully, rather than avoiding creating valtrees that contain errors in an ad hoc way like this PR does.
camelid
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #159246) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
3 participants
Fixes #154632
When lowering const expressions,
ValTree::from_branches()was called without checking whether any of the branches or the type contained prior errors. This produced aConstKind::Valuewrapping an error constant, which later caused an ICE invaltree_to_const_value(). Therefore this PR introducesConst::new_value_from_branches, which checks forerror_reported()on both the branches and the type before constructing theValTree, and returnsConst::new_errorif any error is found.