-
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
Lower type ascriptions to HAIR and MIR #54447
Conversation
This comment has been minimized.
This comment has been minimized.
src/librustc_mir/build/expr/stmt.rs
Outdated
@@ -161,6 +162,41 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { | |||
); | |||
block.unit() | |||
} | |||
ExprKind::PlaceTypeAscription { source, user_ty } => { |
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.
This code is not in the right place, I don't think. It should go into expr_as_place
:
rust/src/librustc_mir/build/expr/as_place.rs
Lines 45 to 50 in 1002e40
fn expr_as_place( | |
&mut self, | |
mut block: BasicBlock, | |
expr: Expr<'tcx>, | |
mutability: Mutability, | |
) -> BlockAnd<Place<'tcx>> { |
The idea is that each expr kind is implemented in exactly one place, whereever is the "best fit". This particular spot is for "statements" -- control-flow like things.
src/librustc_mir/build/expr/stmt.rs
Outdated
); | ||
block.unit() | ||
} | ||
ExprKind::ValueTypeAscription { source, user_ty } => { |
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.
Similarly this should go into as_rvalue
, I think?
rust/src/librustc_mir/build/expr/as_rvalue.rs
Lines 49 to 54 in 1002e40
fn expr_as_rvalue( | |
&mut self, | |
mut block: BasicBlock, | |
scope: Option<region::Scope>, | |
expr: Expr<'tcx>, | |
) -> BlockAnd<Rvalue<'tcx>> { |
Or maybe, actually, it too should go into as_place
, and it should just evaluate its argument into a temp
and yield the temp as a place (sort of like it does today).
src/librustc_mir/build/expr/stmt.rs
Outdated
let source_span = source.span; | ||
let source_ty = source.ty.clone(); | ||
let temp = this.temp(source_ty.clone(), source_span); | ||
unpack!(block = this.into(&temp, block, source)); |
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.
Here I think you would call as_temp
instead of calling into
yourself:
rust/src/librustc_mir/build/expr/as_temp.rs
Lines 19 to 27 in 1002e40
/// Compile `expr` into a fresh temporary. This is used when building | |
/// up rvalues so as to freeze the value that will be consumed. | |
pub fn as_temp<M>( | |
&mut self, | |
block: BasicBlock, | |
temp_lifetime: Option<region::Scope>, | |
expr: M, | |
mutability: Mutability, | |
) -> BlockAnd<Local> |
ExprKind::Scope { .. } => None, | ||
ExprKind::Scope { .. } | ||
| ExprKind::PlaceTypeAscription { .. } | ||
| ExprKind::ValueTypeAscription { .. } => None, |
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.
These should not return None
-- rather I think they should both return Some(Category::Place)
.
0e94459
to
ca114d6
Compare
This comment has been minimized.
This comment has been minimized.
So, something is wrong with type ascriptions in const contexts... this simple program would overflow its stack: #![feature(type_ascription)]
const C1: u8 = 10: u8;
fn main() {} I'm not exactly sure how to debug this. |
}, | ||
ValueTypeAscription { | ||
source: ExprRef<'tcx>, | ||
/// Type that the user gave to this expression |
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.
This is on only one variant.
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.
Fixed.
ca114d6
to
1c731df
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
Looks like we need some tests! Do you need help to write up a few tests?
@KiChjang when you get a chance, can you add some tests into Did you plan to support casts, too? |
@nikomatsakis Just added a UI test for user ascribed types, going to be working on casts next. |
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.
Looks good so far! will wait for the cast stuff
src/librustc_mir/hair/cx/expr.rs
Outdated
@@ -718,7 +718,23 @@ fn make_mirror_unadjusted<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, | |||
ExprKind::Cast { source } | |||
} | |||
} | |||
hir::ExprKind::Type(ref source, _) => return source.make_mirror(cx), | |||
hir::ExprKind::Type(ref source, ref ty) => { | |||
if let Some(user_ty) = cx.tables.user_provided_tys().get(ty.hir_id) { |
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.
I think I should use expect
here instead of silently returning the mirror of the source expression when we can't find the user_ty in the table.
7a94ed2
to
8380b25
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@bors r+ |
📌 Commit 8380b25 has been approved by |
Lower type ascriptions to HAIR and MIR Fixes #54331. r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
Fixes #54331.
r? @nikomatsakis