-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[Typed throws] Infer thrown error type for multi-statement closures #70478
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -2304,6 +2304,7 @@ ConstraintSystem::matchTupleTypes(TupleType *tuple1, TupleType *tuple2, | |
case ConstraintKind::ExplicitGenericArguments: | ||
case ConstraintKind::SameShape: | ||
case ConstraintKind::MaterializePackExpansion: | ||
case ConstraintKind::CaughtError: | ||
llvm_unreachable("Bad constraint kind in matchTupleTypes()"); | ||
} | ||
|
||
|
@@ -2665,6 +2666,7 @@ static bool matchFunctionRepresentations(FunctionType::ExtInfo einfo1, | |
case ConstraintKind::ExplicitGenericArguments: | ||
case ConstraintKind::SameShape: | ||
case ConstraintKind::MaterializePackExpansion: | ||
case ConstraintKind::CaughtError: | ||
return true; | ||
} | ||
|
||
|
@@ -3186,6 +3188,7 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2, | |
case ConstraintKind::ExplicitGenericArguments: | ||
case ConstraintKind::SameShape: | ||
case ConstraintKind::MaterializePackExpansion: | ||
case ConstraintKind::CaughtError: | ||
llvm_unreachable("Not a relational constraint"); | ||
} | ||
|
||
|
@@ -6965,6 +6968,7 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind, | |
case ConstraintKind::ExplicitGenericArguments: | ||
case ConstraintKind::SameShape: | ||
case ConstraintKind::MaterializePackExpansion: | ||
case ConstraintKind::CaughtError: | ||
llvm_unreachable("Not a relational constraint"); | ||
} | ||
} | ||
|
@@ -13647,6 +13651,17 @@ ConstraintSystem::simplifyMaterializePackExpansionConstraint( | |
return SolutionKind::Error; | ||
} | ||
|
||
ConstraintSystem::SolutionKind | ||
ConstraintSystem::simplifyCaughtErrorConstraint( | ||
Type type, | ||
CatchNode catchNode, | ||
TypeMatchOptions flags, | ||
ConstraintLocatorBuilder locator) { | ||
Type caughtErrorType = inferCaughtErrorType(catchNode); | ||
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. 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:
|
||
addConstraint(ConstraintKind::Bind, type, caughtErrorType, locator); | ||
return SolutionKind::Solved; | ||
} | ||
|
||
ConstraintSystem::SolutionKind | ||
ConstraintSystem::simplifyExplicitGenericArgumentsConstraint( | ||
Type type1, Type type2, TypeMatchOptions flags, | ||
|
@@ -15293,6 +15308,7 @@ ConstraintSystem::addConstraintImpl(ConstraintKind kind, Type first, | |
case ConstraintKind::KeyPathApplication: | ||
case ConstraintKind::FallbackType: | ||
case ConstraintKind::SyntacticElement: | ||
case ConstraintKind::CaughtError: | ||
llvm_unreachable("Use the correct addConstraint()"); | ||
} | ||
|
||
|
@@ -15879,6 +15895,11 @@ ConstraintSystem::simplifyConstraint(const Constraint &constraint) { | |
return simplifyMaterializePackExpansionConstraint( | ||
constraint.getFirstType(), constraint.getSecondType(), | ||
/*flags*/ llvm::None, constraint.getLocator()); | ||
|
||
case ConstraintKind::CaughtError: | ||
return simplifyCaughtErrorConstraint( | ||
constraint.getFirstType(), constraint.getCatchNode(), | ||
/*flags*/ llvm::None, constraint.getLocator()); | ||
} | ||
|
||
llvm_unreachable("Unhandled ConstraintKind in switch."); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -361,16 +361,29 @@ static void createConjunction(ConstraintSystem &cs, DeclContext *dc, | |
cs, element, context, elementLoc, isDiscarded)); | ||
} | ||
|
||
for (auto *externalVar : paramCollector.getTypeVars()) | ||
referencedVars.push_back(externalVar); | ||
|
||
// If the body of the closure is being used to infer the thrown error type | ||
// of that closure, introduce a constraint to do so. | ||
if (locator->directlyAt<ClosureExpr>()) { | ||
auto *closure = castToExpr<ClosureExpr>(locator->getAnchor()); | ||
if (auto thrownErrorTypeVar = cs.getInferredThrownError(closure)) { | ||
referencedVars.push_back(thrownErrorTypeVar); | ||
constraints.push_back( | ||
Constraint::createCaughtError(cs, Type(thrownErrorTypeVar), closure, | ||
locator, referencedVars)); | ||
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. A couple of points here:
|
||
referencedVars.pop_back(); | ||
DougGregor marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
// It's possible that there are no viable elements in the body, | ||
// because e.g. whole body is an `#if` statement or it only has | ||
// declarations that are checked during solution application. | ||
// In such cases, let's avoid creating a conjunction. | ||
if (constraints.empty()) | ||
return; | ||
|
||
for (auto *externalVar : paramCollector.getTypeVars()) | ||
referencedVars.push_back(externalVar); | ||
|
||
cs.addUnsolvedConstraint(Constraint::createConjunction( | ||
cs, constraints, isIsolated, locator, referencedVars)); | ||
} | ||
|
@@ -1092,10 +1105,11 @@ class SyntacticElementConstraintGenerator | |
void visitBraceStmt(BraceStmt *braceStmt) { | ||
auto &ctx = cs.getASTContext(); | ||
|
||
ClosureExpr *closure = nullptr; | ||
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. Nit: Doesn't seem to be necessary since it's not used anywhere else? |
||
CaptureListExpr *captureList = nullptr; | ||
{ | ||
if (locator->directlyAt<ClosureExpr>()) { | ||
auto *closure = castToExpr<ClosureExpr>(locator->getAnchor()); | ||
closure = castToExpr<ClosureExpr>(locator->getAnchor()); | ||
captureList = getAsExpr<CaptureListExpr>(cs.getParentExpr(closure)); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -433,6 +433,11 @@ Type ConstraintSystem::getCaughtErrorType(CatchNode catchNode) { | |
} | ||
|
||
// Handle inference of caught error types. | ||
return inferCaughtErrorType(catchNode); | ||
} | ||
|
||
Type ConstraintSystem::inferCaughtErrorType(CatchNode catchNode) { | ||
ASTContext &ctx = getASTContext(); | ||
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. Nit: appears to be unused. 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. Right, thanks! |
||
|
||
// Collect all of the potential throw sites for this catch node. | ||
SmallVector<PotentialThrowSite, 2> throwSites; | ||
|
@@ -477,6 +482,15 @@ Type ConstraintSystem::getCaughtErrorType(CatchNode catchNode) { | |
return caughtErrorType; | ||
} | ||
|
||
TypeVariableType * | ||
ConstraintSystem::getInferredThrownError(ClosureExpr *closure) { | ||
auto closureType = getClosureType(closure); | ||
if (Type thrownError = closureType->getThrownError()) | ||
return thrownError->getAs<TypeVariableType>(); | ||
|
||
return nullptr; | ||
} | ||
|
||
ConstraintLocator *ConstraintSystem::getConstraintLocator( | ||
ASTNode anchor, ArrayRef<ConstraintLocator::PathElement> path) { | ||
auto summaryFlags = ConstraintLocator::getSummaryFlagsForPath(path); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1680,7 +1680,6 @@ void ConstraintSystem::print(raw_ostream &out) const { | |
out << "\n"; | ||
}); | ||
out << "\n"; | ||
|
||
} | ||
} | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect I don't need it to bind to a hole at all, thank you for noticing this.