Permit Coercions in Type Ascriptions #79730
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
Hey! I left various comments and suggestions -- this is close but not quite there. That said, I had an idea while reviewing for a simpler change that may be preferable which basically sidesteps the motivation for this PR. I pinged on Zulip to see what the lang team thinks about it though (the idea was to forbid type ascription on place expressions, so that x: i32 is just not permitted).
| } | ||
| } | ||
| } | ||
|
|
||
| /// When type-checking an expression, we propagate downward | ||
| /// whatever type hint we are able in the form of an `Expectation`. | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub enum Expectation<'tcx> { |
There was a problem hiding this comment.
Nit: I feel like we should structure this like
struct Expectation<'tcx> {
coercion_site: TypeAscriptionCtxt,
kind: ExpectationKind<'tcx>
}
enum ExpectationKind<"tcx> {
NoExpectation,
....
}| // ExprKind::Type in a sub-expression and TypeAscriptionCtxt is set, we coerce | ||
| // the type ascription | ||
| #[derive(Copy, Clone, Debug)] | ||
| pub enum TypeAscriptionCtxt { |
There was a problem hiding this comment.
Nit: Let's rename this to something like CoercionPermitted
| expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>, | ||
| ctxt: TypeAscriptionCtxt, |
There was a problem hiding this comment.
It seems like we shouldn't need this argument -- why would you invoke check_expr_coercable_to_type except if you were at a coercion site?
| ty::Adt(def, _) if def.is_box() => Expectation::rvalue_hint(self, ty.boxed_ty()), | ||
| _ => NoExpectation, | ||
| }); | ||
| let type_ascr_ctxt = expected.get_coercion_ctxt(); |
There was a problem hiding this comment.
Hmm, I am not sure we want to propagate the coercion context here.
This applies to box foo expressions, and it says that if you have something like
let x: Box<T> = box bar;then bar should be coerced to T. This is probably reasonable, but I think we likely want to just say that this is always a coercion site. It doesn't really depend on the context.
| @@ -320,7 +341,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |||
| let tcx = self.tcx; | |||
| let expected_inner = match unop { | |||
| hir::UnOp::UnNot | hir::UnOp::UnNeg => expected, | |||
| hir::UnOp::UnDeref => NoExpectation, | |||
| hir::UnOp::UnDeref => NoExpectation(expected.get_coercion_ctxt()), | |||
There was a problem hiding this comment.
I don't think we want to consider this a coercion site. This corresponds to an expression like *foo, and it doesn't make sense to "coerce" foo to .. anything.
| let e_ty = self.check_expr_with_hint(e, coerce_to); | ||
| let e_ty = self.check_expr_with_expectation( | ||
| e, | ||
| ExpectHasType(coerce_to, expected.get_coercion_ctxt()), |
There was a problem hiding this comment.
Hmm, so this is interesting... I think this is another case where we would really want to thread down the CoerceMany instance. What's going on here is that we have something like:
let x: [T] = [a, b, c];I think that having the check_expr_coercable_to call occur inside check_expr_with_expectation and then calling coerce.coerce may cause a problem. Hmm.
| let (element_ty, t) = match uty { | ||
| Some(uty) => { | ||
| self.check_expr_coercable_to_type(&element, uty, None); | ||
| self.check_expr_coercable_to_type(&element, uty, None, type_ascr_ctxt); |
There was a problem hiding this comment.
as above, this argument should not be necessary, but this should always be a coercion site
| (uty, uty) | ||
| } | ||
| None => { | ||
| let ty = self.next_ty_var(TypeVariableOrigin { | ||
| kind: TypeVariableOriginKind::MiscVariable, | ||
| span: element.span, | ||
| }); | ||
| let element_ty = self.check_expr_has_type_or_error(&element, ty, |_| {}); | ||
| let element_ty = | ||
| self.check_expr_has_type_or_error(&element, ty, |_| {}, type_ascr_ctxt); |
There was a problem hiding this comment.
Hmm, this is a sort of tricky case. This corresponds to [E; 22] and I guess that we want the expression E to always be considered a coercion site. So this should probably just be a call to check_Expr_coerces_to but using the target type of a fresh variable. But I'm not sure why the code is written this way, there may be a reason, I guess it has to do with extracting as much info about the element type as we can. Still, I would prefer to just simplify to always call coercable_To
| let elt_ts_iter = elts.iter().enumerate().map(|(i, e)| match flds { | ||
| Some(ref fs) if i < fs.len() => { | ||
| let ety = fs[i].expect_ty(); | ||
| self.check_expr_coercable_to_type(&e, ety, None); | ||
| self.check_expr_coercable_to_type(&e, ety, None, type_ascr_ctxt); |
There was a problem hiding this comment.
as usual, can remove this parameter
| ety | ||
| } | ||
| _ => self.check_expr_with_expectation(&e, NoExpectation), | ||
| _ => self.check_expr_with_expectation(&e, NoExpectation(type_ascr_ctxt)), |
There was a problem hiding this comment.
this should always be considered a coercion site, but we may not have a useful hint
camelid
added
A-coercions
|
☔ The latest upstream changes (presumably #80105) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels: |
|
Ping from triage |
JohnCSimon
added
S-waiting-on-author
|
@JohnCSimon We agreed on trying another approach for this issue (#80788). This PR will be closed if the other PR is accepted. |
Dylan-DPC-zz
added
S-blocked
|
Closing in favor of #80788 |
We want to permit coercions of type ascriptions inside coercion sites. To implement this we introduce a
TypeAscriptionCtxt, which we pass inside anExpectationdown to sub-expressions. Once we encounter a coercion site, we set TypeAscriptionCtxt::Coercion in the Expectation and propagate this down intocheck_expr_with_expectation. Whenever we encounter a type ascription and TypeAscriptionCtxt::Coercion is set, we coerce the type ascription.Since not all sites, in which
check_expr_coercable_to_typecalls originate, are coercion sites (e.g. ExprKind::Assign or some methods in op.rs usecheck_expr_coercable_to_type), we currently also pass a TypeAscriptionCtxt tocheck_expr_coercable_to_type. Exposing TypeAscriptionCtxt throughout typeck::check is probably bad, I'm not sure what's the best way to hide the Ctxt, maybe we could call different variants of 'check_expr_coercable_to_type' based on whether the site at which these calls originate is an actual coercion site or not?Closes #78248
r? @nikomatsakis