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.

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

Don't try to promote already promoted out temporaries #54816

Merged
merged 6 commits into from
Oct 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions src/librustc_mir/transform/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,14 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
let operand = Operand::Copy(promoted_place(ty, span));
mem::replace(&mut args[index], operand)
}
// We expected a `TerminatorKind::Call` for which we'd like to promote an
// argument. `qualify_consts` saw a `TerminatorKind::Call` here, but
// we are seeing a `Goto`. That means that the `promote_temps` method
// already promoted this call away entirely. This case occurs when calling
// a function requiring a constant argument and as that constant value
// providing a value whose computation contains another call to a function
// requiring a constant argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this. Sorry to nitpick, but this 2nd sentence doesn't parse grammatically.

TerminatorKind::Goto { .. } => return,
_ => bug!()
}
}
Expand Down
25 changes: 22 additions & 3 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -812,7 +812,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {

let fn_ty = func.ty(self.mir, self.tcx);
let mut callee_def_id = None;
let (mut is_shuffle, mut is_const_fn) = (false, false);
let mut is_shuffle = false;
let mut is_const_fn = false;
let mut is_promotable_const_fn = false;
if let ty::FnDef(def_id, _) = fn_ty.sty {
callee_def_id = Some(def_id);
match self.tcx.fn_sig(def_id).abi() {
Expand Down Expand Up @@ -873,6 +875,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
// functions without #[rustc_promotable]
if self.tcx.is_promotable_const_fn(def_id) {
is_const_fn = true;
is_promotable_const_fn = true;
} else if self.tcx.is_const_fn(def_id) {
is_const_fn = true;
}
} else {
// stable const fn or unstable const fns with their feature gate
Expand Down Expand Up @@ -974,7 +979,17 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
if !constant_arguments.contains(&i) {
return
}
if this.qualif.is_empty() {
// Since the argument is required to be constant,
// we care about constness, not promotability.
// If we checked for promotability, we'd miss out on
// the results of function calls (which are never promoted
// in runtime code)
// This is not a problem, because the argument explicitly
// requests constness, in contrast to regular promotion
// which happens even without the user requesting it.
// We can error out with a hard error if the argument is not
// constant here.
if (this.qualif - Qualif::NOT_PROMOTABLE).is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for expanding on this. I still think the terminology is a bit confusing: it seems to suggest "return values are never promotable but we're going to promote one here anyway, because it's const."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... yes. The rules for automatic promotion don't include function calls. The rules for constants do include function calls. Since the current function requires a constant here, if we did something like 0usize - 1 we'd get an error. If you just do &(0usize - 1) in normal runtime code, you get a warning and a panic at runtime (in debug mode).

Copy link
Contributor

@alexreg alexreg Oct 25, 2018

Choose a reason for hiding this comment

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

Yeah sure, I agree this makes sense, but could we just change the comment to "never promoted for runtime-evaluated/non-const/whatever functions" or such, just to be super-clear and make me happy? :-)

this.promotion_candidates.push(candidate);
} else {
this.tcx.sess.span_err(this.span,
Expand Down Expand Up @@ -1003,7 +1018,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
// Be conservative about the returned value of a const fn.
let tcx = self.tcx;
let ty = dest.ty(self.mir, tcx).to_ty(tcx);
self.qualif = Qualif::empty();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise we'd be erasing the NOT_PROMOTABLE qualification we added above.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but I think you want to reset all fields apart from that one, no? Alternatively, just make sure that's set after self.qualif is reset.

if is_const_fn && !is_promotable_const_fn && self.mode == Mode::Fn {
self.qualif = Qualif::NOT_PROMOTABLE;
} else {
self.qualif = Qualif::empty();
}
self.add_type(ty);
}
self.assign(dest, location);
Expand Down
17 changes: 17 additions & 0 deletions src/test/ui/consts/const-eval/double_promotion.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// compile-pass

#![feature(const_fn, rustc_attrs)]

#[rustc_args_required_const(0)]
pub const fn a(value: u8) -> u8 {
value
}

#[rustc_args_required_const(0)]
pub fn b(_: u8) {
unimplemented!()
}

fn main() {
let _ = b(a(0));
}