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 1 commit
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
2 changes: 2 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,8 @@ impl<'a, 'tcx> Promoter<'a, 'tcx> {
let operand = Operand::Copy(promoted_place(ty, span));
mem::replace(&mut args[index], operand)
}
// already promoted out
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explain how we know the candidate is already promoted out here?

TerminatorKind::Goto { .. } => return,
_ => bug!()
}
}
Expand Down
18 changes: 14 additions & 4 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,9 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
if !constant_arguments.contains(&i) {
return
}
if this.qualif.is_empty() {
// if the argument requires a constant, we care about constness, not
Copy link
Contributor

Choose a reason for hiding this comment

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

This could do with elaboration. What do we mean by "requires a constant"? Where is this checked for?

// promotability
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 All @@ -985,7 +992,11 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
}

// non-const fn calls.
if !is_const_fn {
if is_const_fn {
if !is_promotable_const_fn && self.mode == Mode::Fn {
self.qualif = Qualif::NOT_PROMOTABLE;
}
} else {
self.qualif = Qualif::NOT_CONST;
if self.mode != Mode::Fn {
self.tcx.sess.delay_span_bug(
Expand All @@ -1003,7 +1014,6 @@ 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.

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));
}