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.

Sign up for GitHub

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

Do not assert in op_to_const. #117441

Merged
merged 6 commits into from
Nov 2, 2023
Merged

Do not assert in op_to_const. #117441

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c2f49e9
Do not assert in op_to_const.
cjgillot Oct 31, 2023
a2e151c
Make ui into mir-opt test.
cjgillot Oct 31, 2023
3a55c28
Rename hook.
cjgillot Oct 31, 2023
0f8f77f
Rebase fallout.
cjgillot Nov 1, 2023
224e290
Specify diagnostic path.
cjgillot Nov 1, 2023
f512f91
Comment `for_diagnostics` flag.
cjgillot Nov 1, 2023
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
13 changes: 10 additions & 3 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ pub(crate) fn mk_eval_cx<'mir, 'tcx>(
pub(super) fn op_to_const<'tcx>(
Copy link
Member

Choose a reason for hiding this comment

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

Please extend the doc comment (sadly github doesn't let me put a suggestion there...)

/// The `for_diagnostics` flag turns the usual rules for returning `ConstValue::Scalar` into a best-effort
/// attempt. This is not okay for use in const-eval sine it breaks invariants rustc relies on, but
/// it is okay for diagnostics which will just give up gracefully when they encounter an `Indirect` they
/// cannot handle.

ecx: &CompileTimeEvalContext<'_, 'tcx>,
op: &OpTy<'tcx>,
for_diagnostics: bool,
) -> ConstValue<'tcx> {
// Handle ZST consistently and early.
if op.layout.is_zst() {
Expand All @@ -132,8 +133,14 @@ pub(super) fn op_to_const<'tcx>(
// functionality.)
_ => false,
};
let immediate = if force_as_immediate && let Ok(imm) = ecx.read_immediate(op) {
Right(imm)
let immediate = if force_as_immediate {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder, would it work to just say if !force_as_immediate && force_as_immediate here? Then we could avoid "catching" entirely.

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 tried it. This would degrade diagnostics significantly, as we wont be able to output an Option<u32> as a Some(99_usize), just Some(Indirect { stuff }), which IMO is much harder to use.

Copy link
Member

@RalfJung RalfJung Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This could be alleviated by equipping the diagnostic code with support for printing integers that are Indirect. Not sure if that's worth it though.

match ecx.read_immediate(op) {
Ok(imm) => Right(imm),
Err(err) if !for_diagnostics => {
panic!("normalization works on validated constants: {err:?}")
}
_ => op.as_mplace_or_imm(),
}
} else {
op.as_mplace_or_imm()
};
Expand Down Expand Up @@ -205,7 +212,7 @@ pub(crate) fn turn_into_const_value<'tcx>(
);

// Turn this into a proper constant.
op_to_const(&ecx, &mplace.into())
op_to_const(&ecx, &mplace.into(), /* for diagnostics */ false)
}

#[instrument(skip(tcx), level = "debug")]
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/const_eval/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ pub(crate) fn try_destructure_mir_constant_for_user_output<'tcx>(
let fields_iter = (0..field_count)
.map(|i| {
let field_op = ecx.project_field(&down, i).ok()?;
let val = op_to_const(&ecx, &field_op);
let val = op_to_const(&ecx, &field_op, /* for diagnostics */ true);
Some((val, field_op.layout.ty))
})
.collect::<Option<Vec<_>>>()?;
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/const_eval/valtrees.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ pub fn valtree_to_const_value<'tcx>(
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, CanAccessStatics::No);
let imm = valtree_to_ref(&mut ecx, valtree, *inner_ty);
let imm = ImmTy::from_immediate(imm, tcx.layout_of(param_env_ty).unwrap());
op_to_const(&ecx, &imm.into())
op_to_const(&ecx, &imm.into(), /* for diagnostics */ false)
}
ty::Tuple(_) | ty::Array(_, _) | ty::Adt(..) => {
let layout = tcx.layout_of(param_env_ty).unwrap();
Expand Down Expand Up @@ -265,7 +265,7 @@ pub fn valtree_to_const_value<'tcx>(
dump_place(&ecx, &place);
intern_const_alloc_recursive(&mut ecx, InternKind::Constant, &place).unwrap();

op_to_const(&ecx, &place.into())
op_to_const(&ecx, &place.into(), /* for diagnostics */ false)
}
ty::Never
| ty::Error(_)
Expand Down
Loading