[Typed throws] Infer thrown error type for multi-statement closures #70478
Conversation
DougGregor
requested review from
hborla,
slavapestov and
xedin
as code owners
|
@swift-ci please smoke test |
| !CS.getAppliedResultBuilderTransform(closure) && | ||
| !closure->hasSingleExpressionBody()) { | ||
| return Type( | ||
| CS.createTypeVariable(thrownErrorLocator, TVO_CanBindToHole)); |
There was a problem hiding this comment.
If this is allowed to bind to a hole we should handle it in TypeVariableBinding::fixForHole by producing an appropriate fix otherwise we'd end up crashing during solution verification when the type cannot be inferred.
There was a problem hiding this comment.
I suspect I don't need it to bind to a hole at all, thank you for noticing this.
| referencedVars.push_back(thrownErrorTypeVar); | ||
| constraints.push_back( | ||
| Constraint::createCaughtError(cs, Type(thrownErrorTypeVar), closure, | ||
| locator, referencedVars)); |
There was a problem hiding this comment.
A couple of points here:
-
This constraint needs a custom locator, maybe use
ClosureThrownErrorjust like the type variable but it cannot be directly on the closure. -
I don't think
referencedVarsneeds to be passed here because the constraint itself doesn't actually reference all of these variables and there is no risk of it being disconnected from the conjunction.
| @@ -1092,10 +1105,11 @@ class SyntacticElementConstraintGenerator | |||
| void visitBraceStmt(BraceStmt *braceStmt) { | |||
| auto &ctx = cs.getASTContext(); | |||
|
|
|||
| ClosureExpr *closure = nullptr; | |||
There was a problem hiding this comment.
Nit: Doesn't seem to be necessary since it's not used anywhere else?
| CatchNode catchNode, | ||
| TypeMatchOptions flags, | ||
| ConstraintLocatorBuilder locator) { | ||
| Type caughtErrorType = inferCaughtErrorType(catchNode); |
There was a problem hiding this comment.
Since it's only used in multi-statement closures at the moment it's guaranteed to have all the throw sites resolved by the time it comes to this constraint (since it's the last in the body). The situation is more interesting for single-expression closures because they support bi-directional inference and the constraint needs to be re-activated every time something in one of its throw sites gets resolved.
I think what needs to happen to support single-expression closures is:
CaughtErrorconstraint needs to reference all of the relevant (relevance should be determined by thePotentialThrowSite) type variables from all of its potential throw sites; This way it would never run into the risk of being disconnected from its context;- Referencing these variables also triggers re-activation which is the most important.
- Simplification logic needs to go over all of the referenced variables and check whether they are fixed before attempting to call
inferCaughtErrorTypeand re-introduce the constraint otherwise; CaughtErrorconstraint for single-expression closures could be introduced @ https://github.com/apple/swift/blob/main/lib/Sema/CSSyntacticElement.cpp#L1132 when all of the potential throw sites in the body have been identified.
| } | ||
|
|
||
| Type ConstraintSystem::inferCaughtErrorType(CatchNode catchNode) { | ||
| ASTContext &ctx = getASTContext(); |
No description provided.