Skip to content
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

[AST] Clean up handling of single-expression closures #32178

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -3774,7 +3774,7 @@ class ClosureExpr : public AbstractClosureExpr {
/// The bit indicates whether this closure has had a function builder
/// applied to it.
llvm::PointerIntPair<VarDecl *, 1, bool> CapturedSelfDeclAndAppliedBuilder;

/// The location of the "throws", if present.
SourceLoc ThrowsLoc;

Expand All @@ -3786,7 +3786,8 @@ class ClosureExpr : public AbstractClosureExpr {
SourceLoc InLoc;

/// The explicitly-specified result type.
TypeExpr *ExplicitResultType;
llvm::PointerIntPair<TypeExpr *, 1, bool>
ExplicitResultTypeAndEnclosingChecked;

/// The body of the closure, along with a bit indicating whether it
/// was originally just a single expression.
Expand All @@ -3801,7 +3802,8 @@ class ClosureExpr : public AbstractClosureExpr {
BracketRange(bracketRange),
CapturedSelfDeclAndAppliedBuilder(capturedSelfDecl, false),
ThrowsLoc(throwsLoc), ArrowLoc(arrowLoc), InLoc(inLoc),
ExplicitResultType(explicitResultType), Body(nullptr) {
ExplicitResultTypeAndEnclosingChecked(explicitResultType, false),
Body(nullptr) {
setParameterList(params);
Bits.ClosureExpr.HasAnonymousClosureVars = false;
}
Expand Down Expand Up @@ -3855,13 +3857,15 @@ class ClosureExpr : public AbstractClosureExpr {

Type getExplicitResultType() const {
assert(hasExplicitResultType() && "No explicit result type");
return ExplicitResultType->getInstanceType();
return ExplicitResultTypeAndEnclosingChecked.getPointer()
->getInstanceType();
}
void setExplicitResultType(Type ty);

TypeRepr *getExplicitResultTypeRepr() const {
assert(hasExplicitResultType() && "No explicit result type");
return ExplicitResultType->getTypeRepr();
return ExplicitResultTypeAndEnclosingChecked.getPointer()
->getTypeRepr();
}

/// Determine whether the closure has a single expression for its
Expand Down Expand Up @@ -3918,6 +3922,16 @@ class ClosureExpr : public AbstractClosureExpr {
CapturedSelfDeclAndAppliedBuilder.setInt(flag);
}

/// Whether this closure's body was type checked within the enclosing
/// context.
bool wasTypeCheckedInEnclosingContext() const {
return ExplicitResultTypeAndEnclosingChecked.getInt();
}

void setTypeCheckedInEnclosingContext(bool flag = true) {
ExplicitResultTypeAndEnclosingChecked.setInt(flag);
}

static bool classof(const Expr *E) {
return E->getKind() == ExprKind::Closure;
}
Expand Down
6 changes: 1 addition & 5 deletions lib/AST/ASTDumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2496,11 +2496,7 @@ class PrintExpr : public ExprVisitor<PrintExpr> {
}

OS << '\n';
if (E->hasSingleExpressionBody()) {
printRec(E->getSingleExpressionBody());
} else {
printRec(E->getBody(), E->getASTContext());
}
printRec(E->getBody(), E->getASTContext());
PrintWithColorRAII(OS, ParenthesisColor) << ')';
}
void visitAutoClosureExpr(AutoClosureExpr *E) {
Expand Down
9 changes: 3 additions & 6 deletions lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1923,7 +1923,8 @@ bool ClosureExpr::capturesSelfEnablingImplictSelf() const {

void ClosureExpr::setExplicitResultType(Type ty) {
assert(ty && !ty->hasTypeVariable());
ExplicitResultType->setType(MetatypeType::get(ty));
ExplicitResultTypeAndEnclosingChecked.getPointer()
->setType(MetatypeType::get(ty));
}

FORWARD_SOURCE_LOCS_TO(AutoClosureExpr, Body)
Expand Down Expand Up @@ -2364,11 +2365,7 @@ void swift::simple_display(llvm::raw_ostream &out, const ClosureExpr *CE) {
return;
}

if (CE->hasSingleExpressionBody()) {
out << "single expression closure";
} else {
out << "closure";
}
out << "closure";
}

void swift::simple_display(llvm::raw_ostream &out,
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/BuilderTransform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1701,6 +1701,13 @@ class PreCheckFunctionBuilderApplication : public ASTWalker {
FunctionBuilderBodyPreCheck
PreCheckFunctionBuilderRequest::evaluate(Evaluator &eval,
AnyFunctionRef fn) const {
// We don't want to do the precheck if it will already have happened in
// the enclosing expression.
bool skipPrecheck = false;
if (auto closure = dyn_cast_or_null<ClosureExpr>(
fn.getAbstractClosureExpr()))
skipPrecheck = shouldTypeCheckInEnclosingExpression(closure);

return PreCheckFunctionBuilderApplication(fn, false).run();
}

Expand Down
6 changes: 3 additions & 3 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5787,9 +5787,9 @@ static bool applyTypeToClosureExpr(ConstraintSystem &cs,
if (auto CE = dyn_cast<ClosureExpr>(expr)) {
cs.setType(CE, toType);

// If this is not a single-expression closure, write the type into the
// ClosureExpr directly here, since the visitor won't.
if (!CE->hasSingleExpressionBody())
// If this closure isn't type-checked in its enclosing expression, write
// theg type into the ClosureExpr directly here, since the visitor won't.
if (!shouldTypeCheckInEnclosingExpression(CE))
CE->setType(toType);

return true;
Expand Down
9 changes: 7 additions & 2 deletions lib/Sema/CSClosure.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -331,18 +331,23 @@ SolutionApplicationToFunctionResult ConstraintSystem::applySolution(
fn.setBody(newBody, /*isSingleExpression=*/false);
if (closure) {
closure->setAppliedFunctionBuilder();
closure->setTypeCheckedInEnclosingContext();
solution.setExprTypes(closure);
}

return SolutionApplicationToFunctionResult::Success;

}
assert(closure && "Can only get here with a closure at the moment");

// If there is a single-expression body, transform that body now.
if (fn.hasSingleExpressionBody()) {
// If this closure is checked as part of the enclosing expression, handle
// that now.
if (shouldTypeCheckInEnclosingExpression(closure)) {
ClosureConstraintApplication application(
solution, closure, closureFnType->getResult(), rewriteTarget);
application.visit(fn.getBody());

closure->setTypeCheckedInEnclosingContext();
return SolutionApplicationToFunctionResult::Success;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2248,7 +2248,8 @@ namespace {
// as potential hole right away.
resultTy = CS.createTypeVariable(
resultLoc,
closure->hasSingleExpressionBody() ? 0 : TVO_CanBindToHole);
shouldTypeCheckInEnclosingExpression(closure)
? 0 : TVO_CanBindToHole);
}
}

Expand Down
9 changes: 4 additions & 5 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7157,14 +7157,13 @@ bool ConstraintSystem::resolveClosure(TypeVariableType *typeVar,
}
}

bool hasReturn = hasExplicitResult(closure);
// If this closure should be type-checked as part of this expression,
// generate constraints for it now.
auto &ctx = getASTContext();
// If this is a multi-statement closure its body doesn't participate
// in type-checking.
if (closure->hasSingleExpressionBody()) {
if (shouldTypeCheckInEnclosingExpression(closure)) {
if (generateConstraints(closure, closureType->getResult()))
return false;
} else if (!hasReturn) {
} else if (!hasExplicitResult(closure)) {
// If this closure has an empty body and no explicit result type
// let's bind result type to `Void` since that's the only type empty body
// can produce. Otherwise, if (multi-statement) closure doesn't have
Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -5487,6 +5487,12 @@ BraceStmt *applyFunctionBuilderTransform(
constraints::SolutionApplicationTarget)>
rewriteTarget);

/// Determine whether the given closure expression should be type-checked
/// within the context of its enclosing expression. Otherwise, it will be
/// separately type-checked once its enclosing expression has determined all
/// of the parameter and result types without looking at the body.
bool shouldTypeCheckInEnclosingExpression(ClosureExpr *expr);

} // end namespace swift

#endif // LLVM_SWIFT_SEMA_CONSTRAINT_SYSTEM_H
4 changes: 2 additions & 2 deletions lib/Sema/MiscDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,

bool walkToDeclPre(Decl *D) override {
if (auto *closure = dyn_cast<ClosureExpr>(D->getDeclContext()))
return closure->hasAppliedFunctionBuilder();
return closure->wasTypeCheckedInEnclosingContext();
return false;
}

Expand Down Expand Up @@ -1459,7 +1459,7 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
// Don't walk into nested decls.
bool walkToDeclPre(Decl *D) override {
if (auto *closure = dyn_cast<ClosureExpr>(D->getDeclContext()))
return closure->hasAppliedFunctionBuilder();
return closure->wasTypeCheckedInEnclosingContext();
return false;
}

Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/TypeCheckAvailability.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,8 @@ static void findAvailabilityFixItNodes(SourceRange ReferenceRange,
[](ASTNode Node, ASTWalker::ParentTy Parent) {
if (Expr *ParentExpr = Parent.getAsExpr()) {
auto *ParentClosure = dyn_cast<ClosureExpr>(ParentExpr);
if (!ParentClosure || !ParentClosure->hasSingleExpressionBody()) {
if (!ParentClosure ||
!ParentClosure->wasTypeCheckedInEnclosingContext()) {
return false;
}
} else if (auto *ParentStmt = Parent.getAsStmt()) {
Expand Down
9 changes: 6 additions & 3 deletions lib/Sema/TypeCheckConstraints.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1320,9 +1320,8 @@ bool PreCheckExpression::walkToClosureExprPre(ClosureExpr *closure) {
if (hadParameterError)
return false;

// If the closure has a multi-statement body, we don't walk into it
// here.
if (!closure->hasSingleExpressionBody())
// If we won't be checking the body of the closure, don't walk into it here.
if (!shouldTypeCheckInEnclosingExpression(closure))
return false;

// Update the current DeclContext to be the closure we're about to
Expand Down Expand Up @@ -4058,3 +4057,7 @@ HasDynamicCallableAttributeRequest::evaluate(Evaluator &evaluator,
return type->hasDynamicCallableAttribute();
});
}

bool swift::shouldTypeCheckInEnclosingExpression(ClosureExpr *expr) {
return expr->hasSingleExpressionBody();
}
6 changes: 3 additions & 3 deletions lib/Sema/TypeCheckStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,10 @@ namespace {
}
}

// If the closure has a single expression body or has had a function
// builder applied to it, we need to walk into it with a new sequence.
// If the closure was type checked within its enclosing context,
// we need to walk into it with a new sequence.
// Otherwise, it'll have been separately type-checked.
if (CE->hasSingleExpressionBody() || CE->hasAppliedFunctionBuilder())
if (CE->wasTypeCheckedInEnclosingContext())
CE->getBody()->walk(ContextualizeClosures(CE));

TypeChecker::computeCaptures(CE);
Expand Down