Ensure no type errors when calling Closure/Generator upvars_ty #78392
Conversation
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
So this looks a little bit different than I expected. I expected to have tupled_upvars_ty just return a Ty (which may be a Ty::Err) and upvar_tys return a Result. That said, having tupled_upvars_ty return a Result seems ok -- but I think that for consistency upvar_tys should also do so, rather than silently ICEing if there is a Ty::Err. It seems like there are a number of calls to upvar_tys that are first guarded by a call to tupled_upvars_ty.is_ok(), which seems like a suboptimal pattern (it would be easy to forget).
| @@ -670,7 +673,7 @@ impl<'tcx> UpvarSubsts<'tcx> { | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub fn tupled_upvars_ty(self) -> Ty<'tcx> { | |||
| pub fn tupled_upvars_ty(self) -> Result<Ty<'tcx>, ErrorReported> { | |||
There was a problem hiding this comment.
Pre-existing to some extent, but I think this function merits a comment. Something like:
Returns a tuple type whose elements are the types of the upvars captured by this closure/generator. Returns Err(ErrorReported) if a type error prevented us from figuring out the types of the upvars for this closure.
| @@ -517,13 +519,14 @@ impl<'tcx> GeneratorSubsts<'tcx> { | |||
|
|
|||
| #[inline] | |||
| pub fn upvar_tys(self) -> impl Iterator<Item = Ty<'tcx>> + 'tcx { | |||
There was a problem hiding this comment.
Nit: I think this function merits a comment too. Something like
Returns an iterator over the upvar types for this closure/generator. This should only be invoked after type check is complete; before that, the upvar types may still be being inferred or may be an inference variable. To access the upvar types before type check is known to have completed, use
tupled_upvar_tys.
That said, this will still ICE in the event of a Ty::Err, right? I'm not sure if that's a great idea, as it means that every use of this function has to be guarded by a call to tupled_upvars_ty. Why not make this function return a Result as well (or perhaps an empty vector)?
| // Not yet resolved. | ||
| Ambiguous | ||
|
|
||
| if let Ok(tupled_tys) = substs.as_closure().tupled_upvars_ty() { |
There was a problem hiding this comment.
Nit: it feels like we may want a helper for this pattern of shallow resolving the tupled_upvars_ty, but maybe it doesn't occur that often actually.
There was a problem hiding this comment.
I don't see too much benefit here, especially since each case Tuple/Infer/Error will need to be handled at each call site. Also, we only do this in 3 spots.
Fixes rust-lang#77993 Co-authored-by: Roxane Fruytier <roxane.fruytier@hotmail.com>
arora-aman
force-pushed
the
fix-77993-aman
branch
from
c903524 to
d991d6e
Compare
|
Closing this in favor of #78432 |
Fixes #77993
Places with ungaurded calls to
upvars_tywould already throw a bug in case of Ty::Error, or we were working with MIR / building MIR exprs.r? @nikomatsakis