-
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
Don't try to promote already promoted out temporaries #54816
Changes from all commits
8b097c4
f7629ef
4dc0287
f4fe9b0
ee7f4a2
fd77500
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() { | ||
|
@@ -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 | ||
|
@@ -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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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." There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you remove this line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise we'd be erasing the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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); | ||
|
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)); | ||
} |
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.
Thanks for adding this. Sorry to nitpick, but this 2nd sentence doesn't parse grammatically.