Skip to content

Commit 956d6d2

Browse files
hamishknighthamishknight
authored
Merge pull request #71272 from hamishknight/cleanup-return
[CS] Unify ReturnStmt handling
2 parents 5cbff99 + deaf2dd commit 956d6d2

File tree

8 files changed

+107
-78
lines changed

8 files changed

+107
-78
lines changed

‎include/swift/Sema/ConstraintSystem.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4562,8 +4562,7 @@ class ConstraintSystem {
45624562
///
45634563
/// \returns a possibly-sanitized expression, or null if an error occurred.
45644564
[[nodiscard]]
4565-
Expr *generateConstraints(Expr *E, DeclContext *dc,
4566-
bool isInputExpression = true);
4565+
Expr *generateConstraints(Expr *E, DeclContext *dc);
45674566

45684567
/// Generate constraints for binding the given pattern to the
45694568
/// value of the given expression.

‎include/swift/Sema/SyntacticElementTarget.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,11 @@ class SyntacticElementTarget {
431431
expression.contextualInfo.typeLoc = type;
432432
}
433433

434+
void setExprContextualTypePurpose(ContextualTypePurpose ctp) {
435+
assert(kind == Kind::expression);
436+
expression.contextualInfo.purpose = ctp;
437+
}
438+
434439
/// Whether this target is for an initialization expression and pattern.
435440
bool isForInitialization() const {
436441
return kind == Kind::expression &&

‎lib/Sema/CSApply.cpp‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9595,22 +9595,8 @@ ExprWalker::rewriteTarget(SyntacticElementTarget target) {
95959595

95969596
auto *locator = target.getExprConvertTypeLocator();
95979597
if (!locator) {
9598-
// Bodies of single-expression closures use a special locator
9599-
// for contextual type conversion to make sure that result is
9600-
// convertible to `Void` when `return` is not used explicitly.
9601-
auto *closure = dyn_cast<ClosureExpr>(target.getDeclContext());
9602-
if (closure && closure->hasSingleExpressionBody() &&
9603-
contextualTypePurpose == CTP_ClosureResult) {
9604-
auto *returnStmt =
9605-
castToStmt<ReturnStmt>(closure->getBody()->getLastElement());
9606-
9607-
locator = cs.getConstraintLocator(
9608-
closure, LocatorPathElt::ClosureBody(
9609-
/*hasImpliedReturn*/ returnStmt->isImplied()));
9610-
} else {
9611-
locator = cs.getConstraintLocator(
9612-
expr, LocatorPathElt::ContextualType(contextualTypePurpose));
9613-
}
9598+
locator = cs.getConstraintLocator(
9599+
expr, LocatorPathElt::ContextualType(contextualTypePurpose));
96149600
}
96159601
assert(locator);
96169602

‎lib/Sema/CSGen.cpp‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4959,10 +4959,8 @@ bool ConstraintSystem::generateConstraints(
49594959
}
49604960
}
49614961

4962-
Expr *ConstraintSystem::generateConstraints(
4963-
Expr *expr, DeclContext *dc, bool isInputExpression) {
4964-
if (isInputExpression)
4965-
InputExprs.insert(expr);
4962+
Expr *ConstraintSystem::generateConstraints(Expr *expr, DeclContext *dc) {
4963+
InputExprs.insert(expr);
49664964
return generateConstraintsFor(*this, expr, dc);
49674965
}
49684966

‎lib/Sema/CSSyntacticElement.cpp‎

Lines changed: 28 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -491,7 +491,7 @@ struct SyntacticElementContext
491491
}
492492
}
493493

494-
bool isSingleExpressionClosure(ConstraintSystem &cs) {
494+
bool isSingleExpressionClosure(ConstraintSystem &cs) const {
495495
if (auto ref = getAsAnyFunctionRef()) {
496496
if (cs.getAppliedResultBuilderTransform(*ref))
497497
return false;
@@ -1121,8 +1121,8 @@ class SyntacticElementConstraintGenerator
11211121

11221122
for (auto node : braceStmt->getElements()) {
11231123
if (auto expr = node.dyn_cast<Expr *>()) {
1124-
auto generatedExpr = cs.generateConstraints(
1125-
expr, context.getAsDeclContext(), /*isInputExpression=*/false);
1124+
auto generatedExpr =
1125+
cs.generateConstraints(expr, context.getAsDeclContext());
11261126
if (!generatedExpr) {
11271127
hadError = true;
11281128
}
@@ -1248,33 +1248,7 @@ class SyntacticElementConstraintGenerator
12481248
}
12491249

12501250
void visitReturnStmt(ReturnStmt *returnStmt) {
1251-
// Single-expression closures are effectively a `return` statement,
1252-
// so let's give them a special locator as to indicate that.
1253-
// Return statements might not have a result if we have a closure whose
1254-
// implicit returned value is coerced to Void.
1255-
if (context.isSingleExpressionClosure(cs) && returnStmt->hasResult()) {
1256-
auto *expr = returnStmt->getResult();
1257-
assert(expr && "single expression closure without expression?");
1258-
1259-
expr = cs.generateConstraints(expr, context.getAsDeclContext(),
1260-
/*isInputExpression=*/false);
1261-
if (!expr) {
1262-
hadError = true;
1263-
return;
1264-
}
1265-
1266-
auto contextualResultInfo = getContextualResultInfo();
1267-
cs.addConstraint(ConstraintKind::Conversion, cs.getType(expr),
1268-
contextualResultInfo.getType(),
1269-
cs.getConstraintLocator(
1270-
context.getAsAbstractClosureExpr().get(),
1271-
LocatorPathElt::ClosureBody(
1272-
/*hasImpliedReturn=*/returnStmt->isImplied())));
1273-
return;
1274-
}
1275-
12761251
Expr *resultExpr;
1277-
12781252
if (returnStmt->hasResult()) {
12791253
resultExpr = returnStmt->getResult();
12801254
assert(resultExpr && "non-empty result without expression?");
@@ -1286,10 +1260,10 @@ class SyntacticElementConstraintGenerator
12861260
resultExpr = getVoidExpr(cs.getASTContext(), returnStmt->getEndLoc());
12871261
}
12881262

1289-
auto contextualResultInfo = getContextualResultInfo();
1263+
auto contextualResultInfo = getContextualResultInfoFor(returnStmt);
1264+
12901265
SyntacticElementTarget target(resultExpr, context.getAsDeclContext(),
1291-
contextualResultInfo,
1292-
/*isDiscarded=*/false);
1266+
contextualResultInfo, /*isDiscarded=*/false);
12931267

12941268
if (cs.generateConstraints(target)) {
12951269
hadError = true;
@@ -1334,7 +1308,7 @@ class SyntacticElementConstraintGenerator
13341308
createConjunction({resultElt}, locator);
13351309
}
13361310

1337-
ContextualTypeInfo getContextualResultInfo() const {
1311+
ContextualTypeInfo getContextualResultInfoFor(ReturnStmt *returnStmt) const {
13381312
auto funcRef = AnyFunctionRef::fromDeclContext(context.getAsDeclContext());
13391313
if (!funcRef)
13401314
return {Type(), CTP_Unused};
@@ -1343,8 +1317,18 @@ class SyntacticElementConstraintGenerator
13431317
return {transform->bodyResultType, CTP_ReturnStmt};
13441318

13451319
if (auto *closure =
1346-
getAsExpr<ClosureExpr>(funcRef->getAbstractClosureExpr()))
1347-
return {cs.getClosureType(closure)->getResult(), CTP_ClosureResult};
1320+
getAsExpr<ClosureExpr>(funcRef->getAbstractClosureExpr())) {
1321+
// Single-expression closures need their contextual type locator anchored
1322+
// on the closure itself. Otherwise we use the default contextual type
1323+
// locator, which will be created for us.
1324+
ConstraintLocator *loc = nullptr;
1325+
if (context.isSingleExpressionClosure(cs) && returnStmt->hasResult()) {
1326+
loc = cs.getConstraintLocator(
1327+
closure, {LocatorPathElt::ClosureBody(
1328+
/*hasImpliedReturn=*/returnStmt->isImplied())});
1329+
}
1330+
return {cs.getClosureType(closure)->getResult(), CTP_ClosureResult, loc};
1331+
}
13481332

13491333
return {funcRef->getBodyResultType(), CTP_ReturnStmt};
13501334
}
@@ -2162,22 +2146,15 @@ class SyntacticElementSolutionApplication
21622146
mode = convertToResult;
21632147
}
21642148

2165-
llvm::Optional<SyntacticElementTarget> resultTarget;
2166-
if (auto target = cs.getTargetFor(returnStmt)) {
2167-
resultTarget = *target;
2168-
} else {
2169-
// Single-expression closures have to handle returns in a special
2170-
// way so the target has to be created for them during solution
2171-
// application based on the resolved type.
2172-
assert(context.isSingleExpressionClosure(cs));
2173-
resultTarget = SyntacticElementTarget(
2174-
resultExpr, context.getAsDeclContext(),
2175-
mode == convertToResult ? CTP_ClosureResult : CTP_Unused,
2176-
mode == convertToResult ? resultType : Type(),
2177-
/*isDiscarded=*/false);
2178-
}
2179-
2180-
if (auto newResultTarget = rewriteTarget(*resultTarget)) {
2149+
auto target = *cs.getTargetFor(returnStmt);
2150+
2151+
// If we're not converting to a result, unset the contextual type.
2152+
if (mode != convertToResult) {
2153+
target.setExprConversionType(Type());
2154+
target.setExprContextualTypePurpose(CTP_Unused);
2155+
}
2156+
2157+
if (auto newResultTarget = rewriteTarget(target)) {
21812158
resultExpr = newResultTarget->getAsExpr();
21822159
}
21832160

‎lib/Sema/ConstraintSystem.cpp‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -822,6 +822,12 @@ void ConstraintSystem::addPackEnvironment(PackElementExpr *packElement,
822822
static void extendDepthMap(
823823
Expr *expr,
824824
llvm::DenseMap<Expr *, std::pair<unsigned, Expr *>> &depthMap) {
825+
// If we already have an entry in the map, we don't need to update it. This
826+
// avoids invalidating previous entries when solving a smaller component of a
827+
// larger AST node, e.g during conjunction solving.
828+
if (depthMap.contains(expr))
829+
return;
830+
825831
class RecordingTraversal : public ASTWalker {
826832
SmallVector<ClosureExpr *, 4> Closures;
827833

‎test/IDE/complete_single_expression_return.swift‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,10 +155,27 @@ struct TestExplicitSingleExprClosureBinding {
155155
return self.#^TestExplicitSingleExprClosureBinding^#
156156
}
157157
}
158-
// FIXME: Because we have an explicit return, and no expected type, we shouldn't suggest Void.
158+
// We have an explicit return, and no expected type, so we don't suggest Void.
159159
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
160160
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
161-
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: void()[#Void#];
161+
// TestExplicitSingleExprClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal/TypeRelation[Invalid]: void()[#Void#];
162+
}
163+
164+
struct TestExplicitMultiStmtClosureBinding {
165+
func void() -> Void {}
166+
func str() -> String { return "" }
167+
func int() -> Int { return 0 }
168+
169+
func test() {
170+
let fn = {
171+
()
172+
return self.#^TestExplicitMultiStmtClosureBinding^#
173+
}
174+
}
175+
// We have an explicit return, and no expected type, so we don't suggest Void.
176+
// TestExplicitMultiStmtClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: str()[#String#];
177+
// TestExplicitMultiStmtClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal: int()[#Int#];
178+
// TestExplicitMultiStmtClosureBinding-DAG: Decl[InstanceMethod]/CurrNominal/TypeRelation[Invalid]: void()[#Void#];
162179
}
163180

164181
struct TestExplicitSingleExprClosureBindingWithContext {

‎unittests/Sema/ConstraintGenerationTests.cpp‎

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ TEST_F(SemaTest, TestImplicitForceCastConstraintGeneration) {
3838
auto *castExpr = ForcedCheckedCastExpr::createImplicit(Context, literal,
3939
Context.TheAnyType);
4040

41-
auto *expr = cs.generateConstraints(castExpr, DC, /*isInputExpression=*/true);
41+
auto *expr = cs.generateConstraints(castExpr, DC);
4242

4343
ASSERT_NE(expr, nullptr);
4444

@@ -66,7 +66,7 @@ TEST_F(SemaTest, TestImplicitCoercionConstraintGeneration) {
6666
auto *castExpr = CoerceExpr::createImplicit(Context, literal,
6767
getStdlibType("Double"));
6868

69-
auto *expr = cs.generateConstraints(castExpr, DC, /*isInputExpression=*/true);
69+
auto *expr = cs.generateConstraints(castExpr, DC);
7070

7171
ASSERT_NE(expr, nullptr);
7272

@@ -95,7 +95,7 @@ TEST_F(SemaTest, TestImplicitConditionalCastConstraintGeneration) {
9595
auto *castExpr = ConditionalCheckedCastExpr::createImplicit(
9696
Context, literal, getStdlibType("Double"));
9797

98-
auto *expr = cs.generateConstraints(castExpr, DC, /*isInputExpression=*/true);
98+
auto *expr = cs.generateConstraints(castExpr, DC);
9999

100100
ASSERT_NE(expr, nullptr);
101101

@@ -179,3 +179,44 @@ TEST_F(SemaTest, TestCaptureListIsNotOpenedEarly) {
179179
ASSERT_TRUE(cs.hasType(capture.getVar()));
180180
}
181181
}
182+
183+
TEST_F(SemaTest, TestMultiStmtClosureBodyParentAndDepth) {
184+
// {
185+
// ()
186+
// return ()
187+
// }
188+
DeclAttributes attrs;
189+
auto *closure = new (Context) ClosureExpr(attrs,
190+
/*braceRange=*/SourceRange(),
191+
/*capturedSelfDecl=*/nullptr,
192+
ParameterList::createEmpty(Context),
193+
/*asyncLoc=*/SourceLoc(),
194+
/*throwsLoc=*/SourceLoc(),
195+
/*thrownType*/ nullptr,
196+
/*arrowLoc=*/SourceLoc(),
197+
/*inLoc=*/SourceLoc(),
198+
/*explicitResultType=*/nullptr, DC);
199+
closure->setImplicit();
200+
201+
auto *RS = ReturnStmt::createImplicit(
202+
Context, TupleExpr::createImplicit(Context, {}, {}));
203+
204+
closure->setBody(BraceStmt::createImplicit(Context, {
205+
TupleExpr::createImplicit(Context, {}, {}), RS
206+
}));
207+
208+
SyntacticElementTarget target(closure, DC, ContextualTypeInfo(),
209+
/*isDiscarded*/ true);
210+
211+
ConstraintSystem cs(DC, ConstraintSystemOptions());
212+
cs.solve(target);
213+
214+
ASSERT_EQ(cs.getParentExpr(closure), nullptr);
215+
ASSERT_EQ(cs.getExprDepth(closure), 0);
216+
217+
// We visit the ReturnStmt twice when computing the parent map, ensure we
218+
// don't invalidate its parent on the second walk during the conjunction.
219+
auto *result = RS->getResult();
220+
ASSERT_EQ(cs.getParentExpr(result), closure);
221+
ASSERT_EQ(cs.getExprDepth(result), 1);
222+
}

0 commit comments

Comments
 (0)