Skip to content

Allow return to be omitted from single expression functions. #23251

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

Merged
merged 33 commits into from
Apr 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
74b462b
WIP: Allow return to be omitted from single expression functions.
nate-chandler Mar 8, 2019
64d4e30
Used a constraint system, not typeCheckExpression.
nate-chandler Mar 13, 2019
018ba70
Just revert return insertion if inappropriate.
nate-chandler Mar 14, 2019
2276e2a
Just reuse old body when reverting return insertion.
nate-chandler Mar 15, 2019
08bbce3
Addressed issues from PR review.
nate-chandler Mar 15, 2019
c7f502e
Tested implicit return of most exprs from free functions.
nate-chandler Mar 16, 2019
ac7a76c
Don't insert return during code completion.
nate-chandler Mar 16, 2019
aa10bfa
Placed prechecked expressions back in body.
nate-chandler Mar 18, 2019
72c6651
Only remove return for a unique viable solution.
nate-chandler Mar 18, 2019
16cfada
Expected errors around implicitly returning #line.
nate-chandler Mar 18, 2019
5262e19
Avoided double typechecking when possible.
nate-chandler Mar 19, 2019
1bb7f29
Added comments explaining approach.
nate-chandler Mar 21, 2019
25bbef1
Visited single expression of AbstractFunctionDecl.
nate-chandler Mar 21, 2019
6aa8bfd
Corrected autoclosure handling.
nate-chandler Mar 22, 2019
4f269fc
Added diagnostic for dangling expression at end.
nate-chandler Mar 22, 2019
2cc8985
Added tests to cover autoclosure discriminators.
nate-chandler Mar 22, 2019
859ffdc
Used expected-error to test fixit.
nate-chandler Mar 22, 2019
717e5e4
Added PlaygroundTransform tests on implicit return.
nate-chandler Mar 25, 2019
92998f7
Don't attempt to apply solution from salvage.
nate-chandler Mar 26, 2019
0143ce2
Implicitly convert single exprs from uninhabited.
nate-chandler Mar 26, 2019
5c1537f
Tested implicit non/conversion from uninhabited.
nate-chandler Mar 27, 2019
0b528ae
Don't add an implicit () after return for initializers.
nate-chandler Mar 27, 2019
c21678d
Corrected tests by removing implicit returns.
nate-chandler Mar 27, 2019
1071d97
Replaced superseded test.
nate-chandler Mar 27, 2019
3139d3e
Tweaked remaining failing tests.
nate-chandler Mar 27, 2019
cca301a
Fixed up return omission test cases for Linux.
nate-chandler Mar 27, 2019
448e04e
[ConstraintLocator] Add a special locator element to denote contextua…
xedin Apr 8, 2019
ca6cf0c
[ConstraintSystem] Use special locator for expr representing return o…
xedin Apr 8, 2019
b2ad562
Eliminated conversion from uninhabited.
nate-chandler Apr 8, 2019
a09a964
Don't implicitly return assignments.
nate-chandler Apr 9, 2019
39bbce1
Reverted unneeded test change.
nate-chandler Apr 9, 2019
843ac0b
Removed old comment.
nate-chandler Apr 9, 2019
f8ef213
Corrected off-by-one error.
nate-chandler Apr 10, 2019
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
20 changes: 17 additions & 3 deletions include/swift/AST/Decl.h
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,7 @@ class alignas(1 << DeclAlignInBits) Decl {
SWIFT_INLINE_BITFIELD(SubscriptDecl, VarDecl, 2,
StaticSpelling : 2
);

SWIFT_INLINE_BITFIELD(AbstractFunctionDecl, ValueDecl, 3+8+1+1+1+1+1+1+1,
SWIFT_INLINE_BITFIELD(AbstractFunctionDecl, ValueDecl, 3+8+1+1+1+1+1+1+1+1,
/// \see AbstractFunctionDecl::BodyKind
BodyKind : 3,

Expand All @@ -426,7 +425,10 @@ class alignas(1 << DeclAlignInBits) Decl {

/// Whether this member was synthesized as part of a derived
/// protocol conformance.
Synthesized : 1
Synthesized : 1,

/// Whether this member's body consists of a single expression.
HasSingleExpressionBody : 1
);

SWIFT_INLINE_BITFIELD(FuncDecl, AbstractFunctionDecl, 1+2+1+1+2,
Expand Down Expand Up @@ -5433,13 +5435,25 @@ class AbstractFunctionDecl : public GenericContext, public ValueDecl {
Bits.AbstractFunctionDecl.NeedsNewVTableEntry = false;
Bits.AbstractFunctionDecl.HasComputedNeedsNewVTableEntry = false;
Bits.AbstractFunctionDecl.Synthesized = false;
Bits.AbstractFunctionDecl.HasSingleExpressionBody = false;
}

void setBodyKind(BodyKind K) {
Bits.AbstractFunctionDecl.BodyKind = unsigned(K);
}

public:
void setHasSingleExpressionBody(bool Has = true) {
Bits.AbstractFunctionDecl.HasSingleExpressionBody = Has;
}

bool hasSingleExpressionBody() const {
return Bits.AbstractFunctionDecl.HasSingleExpressionBody;
}

Expr *getSingleExpressionBody() const;
void setSingleExpressionBody(Expr *NewBody);

/// Returns the string for the base name, or "_" if this is unnamed.
StringRef getNameStr() const {
assert(!getFullName().isSpecial() && "Cannot get string for special names");
Expand Down
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsSIL.def
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ NOTE(designated_init_c_struct_fix,none,
ERROR(missing_return,none,
"missing return in a %select{function|closure}1 expected to return %0",
(Type, unsigned))
ERROR(missing_return_last_expr,none,
"missing return in a %select{function|closure}1 expected to return %0; "
"did you mean to return the last expression?",
(Type, unsigned))
ERROR(missing_never_call,none,
"%select{function|closure}1 with uninhabited return type %0 is missing "
"call to another never-returning function on all paths",
Expand Down
5 changes: 4 additions & 1 deletion lib/AST/ASTDumper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1070,7 +1070,10 @@ namespace {
Indent -= 2;
}
}
if (auto Body = D->getBody(/*canSynthesize=*/false)) {
if (D->hasSingleExpressionBody()) {
OS << '\n';
printRec(D->getSingleExpressionBody());
} else if (auto Body = D->getBody(/*canSynthesize=*/false)) {
OS << '\n';
printRec(Body, D->getASTContext());
}
Expand Down
40 changes: 40 additions & 0 deletions lib/AST/Decl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,46 @@ case DeclKind::ID: return cast<ID##Decl>(this)->getLoc();
llvm_unreachable("Unknown decl kind");
}

Expr *AbstractFunctionDecl::getSingleExpressionBody() const {
assert(hasSingleExpressionBody() && "Not a single-expression body");
auto braceStmt = getBody();
assert(braceStmt != nullptr && "No body currently available.");
auto body = getBody()->getElement(0);
if (auto *stmt = body.dyn_cast<Stmt *>()) {
if (auto *returnStmt = dyn_cast<ReturnStmt>(stmt)) {
return returnStmt->getResult();
} else if (auto *failStmt = dyn_cast<FailStmt>(stmt)) {
// We can only get to this point if we're a type-checked ConstructorDecl
// which was originally spelled init?(...) { nil }.
//
// There no longer is a single-expression to return, so ignore null.
return nullptr;
}
}
return body.get<Expr *>();
}

void AbstractFunctionDecl::setSingleExpressionBody(Expr *NewBody) {
assert(hasSingleExpressionBody() && "Not a single-expression body");
auto body = getBody()->getElement(0);
if (auto *stmt = body.dyn_cast<Stmt *>()) {
if (auto *returnStmt = dyn_cast<ReturnStmt>(stmt)) {
returnStmt->setResult(NewBody);
return;
} else if (auto *failStmt = dyn_cast<FailStmt>(stmt)) {
// We can only get to this point if we're a type-checked ConstructorDecl
// which was originally spelled init?(...) { nil }.
//
// We can no longer write the single-expression which is being set on us
// into anything because a FailStmt does not have such a child. As a
// result we need to demand that the NewBody is null.
assert(NewBody == nullptr);
return;
}
}
getBody()->setElement(0, NewBody);
}

bool AbstractStorageDecl::isTransparent() const {
return getAttrs().hasAttribute<TransparentAttr>();
}
Expand Down
56 changes: 54 additions & 2 deletions lib/Parse/ParseDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5655,8 +5655,60 @@ void Parser::parseAbstractFunctionBody(AbstractFunctionDecl *AFD) {
Context.Stats->getFrontendCounters().NumFunctionsParsed++;

ParserResult<BraceStmt> Body = parseBraceItemList(diag::invalid_diagnostic);
if (!Body.isNull())
AFD->setBody(Body.get());
if (!Body.isNull()) {
BraceStmt * BS = Body.get();
AFD->setBody(BS);

// If the body consists of a single expression, turn it into a return
// statement.
//
// But don't do this transformation during code completion, as the source
// may be incomplete and the type mismatch in return statement will just
// confuse the type checker.
if (!Body.hasCodeCompletion() && BS->getNumElements() == 1) {
auto Element = BS->getElement(0);
if (auto *stmt = Element.dyn_cast<Stmt *>()) {
auto kind = AFD->getKind();
if (kind == DeclKind::Var || kind == DeclKind::Subscript ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFD is an AbstractFunctionDecl so it will never have a kind that is Var or Subscript. In general we prefer to use isa<> instead of checking the kind directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR with a fix at #24546 .

kind == DeclKind::Func ) {
if (auto *returnStmt = dyn_cast<ReturnStmt>(stmt)) {
if (!returnStmt->hasResult()) {
auto returnExpr = TupleExpr::createEmpty(Context,
SourceLoc(),
SourceLoc(),
/*implicit*/true);
returnStmt->setResult(returnExpr);
AFD->setHasSingleExpressionBody();
AFD->setSingleExpressionBody(returnExpr);
}
}
}
} else if (auto *E = Element.dyn_cast<Expr *>()) {
if (auto SE = dyn_cast<SequenceExpr>(E->getSemanticsProvidingExpr())) {
if (SE->getNumElements() > 1 && isa<AssignExpr>(SE->getElement(1))) {
// This is an assignment. We don't want to implicitly return
// it.
return;
}
}
if (auto F = dyn_cast<FuncDecl>(AFD)) {
auto RS = new (Context) ReturnStmt(SourceLoc(), E);
BS->setElement(0, RS);
AFD->setHasSingleExpressionBody();
AFD->setSingleExpressionBody(E);
} else if (auto *F = dyn_cast<ConstructorDecl>(AFD)) {
if (F->getFailability() != OTK_None && isa<NilLiteralExpr>(E)) {
// If it's a nil literal, just insert return. This is the only
// legal thing to return.
auto RS = new (Context) ReturnStmt(E->getStartLoc(), E);
BS->setElement(0, RS);
AFD->setHasSingleExpressionBody();
AFD->setSingleExpressionBody(E);
}
}
}
}
}
}

bool Parser::parseAbstractFunctionBodyDelayed(AbstractFunctionDecl *AFD) {
Expand Down
18 changes: 18 additions & 0 deletions lib/SILOptimizer/Mandatory/DataflowDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,37 @@ static void diagnoseMissingReturn(const UnreachableInst *UI,
SILLocation FLoc = F->getLocation();

Type ResTy;
BraceStmt *BS;

if (auto *FD = FLoc.getAsASTNode<FuncDecl>()) {
ResTy = FD->getResultInterfaceType();
BS = FD->getBody(/*canSynthesize=*/false);
} else if (auto *CD = FLoc.getAsASTNode<ConstructorDecl>()) {
ResTy = CD->getResultInterfaceType();
BS = FD->getBody();
} else if (auto *CE = FLoc.getAsASTNode<ClosureExpr>()) {
ResTy = CE->getResultType();
BS = CE->getBody();
} else {
llvm_unreachable("unhandled case in MissingReturn");
}

SILLocation L = UI->getLoc();
assert(L && ResTy);
auto numElements = BS->getNumElements();
if (numElements > 0) {
auto element = BS->getElement(numElements - 1);
if (auto expr = element.dyn_cast<Expr *>()) {
if (expr->getType()->getCanonicalType() == ResTy->getCanonicalType()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeBase::isEqual() is a shortcut for comparing getCanonicalType()s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR with a fix at #24546 .

Context.Diags.diagnose(
expr->getStartLoc(),
diag::missing_return_last_expr, ResTy,
FLoc.isASTNode<ClosureExpr>() ? 1 : 0)
.fixItInsert(expr->getStartLoc(), "return ");
return;
}
}
}
auto diagID = F->isNoReturnFunction() ? diag::missing_never_call
: diag::missing_return;
diagnose(Context,
Expand Down
12 changes: 10 additions & 2 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7684,10 +7684,18 @@ Expr *ConstraintSystem::applySolution(Solution &solution, Expr *expr,
if (hadError)
return nullptr;
}


// We are supposed to use contextual type only if it is present and
// this expression doesn't represent the implicit return of the single
// expression function which got deduced to be `Never`.
auto shouldCoerceToContextualType = [&]() {
return convertType && !(getType(result)->isUninhabited() &&
getContextualTypePurpose() == CTP_ReturnSingleExpr);
};

// If we're supposed to convert the expression to some particular type,
// do so now.
if (convertType) {
if (shouldCoerceToContextualType()) {
result = rewriter.coerceToType(result, convertType,
getConstraintLocator(expr));
if (!result)
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/CSDiag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2102,6 +2102,7 @@ bool FailureDiagnosis::diagnoseContextualConversionError(
}
};
break;
case CTP_ReturnSingleExpr:
case CTP_ReturnStmt:
// Special case the "conversion to void" case.
if (contextualType->isVoid()) {
Expand Down
6 changes: 4 additions & 2 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2771,9 +2771,11 @@ ConstraintSystem::matchTypes(Type type1, Type type2, ConstraintKind kind,
}

// Allow '() -> T' to '() -> ()' and '() -> Never' to '() -> T' for closure
// literals.
// literals and expressions representing an implicit return type of the single
// expression functions.
if (auto elt = locator.last()) {
if (elt->getKind() == ConstraintLocator::ClosureResult) {
if (elt->getKind() == ConstraintLocator::ClosureResult ||
elt->getKind() == ConstraintLocator::SingleExprFuncResultType) {
if (kind >= ConstraintKind::Subtype &&
(type1->isUninhabited() || type2->isVoid())) {
increaseScore(SK_FunctionConversion);
Expand Down
4 changes: 3 additions & 1 deletion lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1169,7 +1169,9 @@ ConstraintSystem::solveImpl(Expr *&expr,
constraintKind = ConstraintKind::Bind;

auto *convertTypeLocator = getConstraintLocator(
getConstraintLocator(expr), ConstraintLocator::ContextualType);
expr, getContextualTypePurpose() == CTP_ReturnSingleExpr
? ConstraintLocator::SingleExprFuncResultType
: ConstraintLocator::ContextualType);

if (allowFreeTypeVariables == FreeTypeVariableBinding::UnresolvedType) {
convertType = convertType.transform([&](Type type) -> Type {
Expand Down
5 changes: 4 additions & 1 deletion lib/Sema/ConstraintLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ void ConstraintLocator::Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
case KeyPathRoot:
case KeyPathValue:
case KeyPathComponentResult:
case SingleExprFuncResultType:
if (unsigned numValues = numNumericValuesInPathElement(elt.getKind())) {
id.AddInteger(elt.getValue());
if (numValues > 1)
Expand Down Expand Up @@ -351,8 +352,10 @@ void ConstraintLocator::dump(SourceManager *sm, raw_ostream &out) {
case KeyPathComponentResult:
out << "key path component result";
break;
case SingleExprFuncResultType:
out << " expected result type of the function with a single expression";
break;
}
}

out << ']';
}
4 changes: 4 additions & 0 deletions lib/Sema/ConstraintLocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ class ConstraintLocator : public llvm::FoldingSetNode {
KeyPathValue,
/// The result type of a key path component. Not used for subscripts.
KeyPathComponentResult,
/// The expected type of the function with a single expression body.
SingleExprFuncResultType,
};

/// Determine the number of numeric values used for the given path
Expand Down Expand Up @@ -168,6 +170,7 @@ class ConstraintLocator : public llvm::FoldingSetNode {
case KeyPathRoot:
case KeyPathValue:
case KeyPathComponentResult:
case SingleExprFuncResultType:
return 0;

case OpenedGeneric:
Expand Down Expand Up @@ -237,6 +240,7 @@ class ConstraintLocator : public llvm::FoldingSetNode {
case KeyPathRoot:
case KeyPathValue:
case KeyPathComponentResult:
case SingleExprFuncResultType:
return 0;

case FunctionArgument:
Expand Down
39 changes: 37 additions & 2 deletions lib/Sema/TypeCheckStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "TypeChecker.h"
#include "TypeCheckType.h"
#include "MiscDiagnostics.h"
#include "ConstraintSystem.h"
#include "swift/Subsystems.h"
#include "swift/AST/ASTPrinter.h"
#include "swift/AST/ASTWalker.h"
Expand Down Expand Up @@ -491,10 +492,17 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
}
}

ContextualTypePurpose ctp = CTP_ReturnStmt;
if (auto func =
dyn_cast_or_null<FuncDecl>(TheFunc->getAbstractFunctionDecl())) {
if (func->hasSingleExpressionBody()) {
ctp = CTP_ReturnSingleExpr;
}
}

auto exprTy = TC.typeCheckExpression(E, DC, TypeLoc::withoutLoc(ResultTy),
CTP_ReturnStmt,
ctp,
options);

RS->setResult(E);

if (!exprTy) {
Expand Down Expand Up @@ -1905,10 +1913,37 @@ bool TypeChecker::typeCheckFunctionBodyUntil(FuncDecl *FD,
BraceStmt *BS = FD->getBody();
assert(BS && "Should have a body");

if (FD->hasSingleExpressionBody()) {
auto resultTypeLoc = FD->getBodyResultTypeLoc();
auto E = FD->getSingleExpressionBody();

if (resultTypeLoc.isNull() || resultTypeLoc.getType()->isVoid()) {
// The function returns void. We don't need an explicit return, no matter
// what the type of the expression is. Take the inserted return back out.
BS->setElement(0, E);
}
}

StmtChecker SC(*this, static_cast<AbstractFunctionDecl *>(FD));
SC.EndTypeCheckLoc = EndTypeCheckLoc;
bool HadError = SC.typeCheckBody(BS);

// If this was a function with a single expression body, let's see
// if implicit return statement came out to be `Never` which means
// that we have eagerly converted something like `{ fatalError() }`
// into `{ return fatalError() }` that has to be corrected here.
if (FD->hasSingleExpressionBody()) {
if (auto *stmt = BS->getElement(0).dyn_cast<Stmt *>()) {
if (auto *RS = dyn_cast<ReturnStmt>(stmt)) {
if (RS->isImplicit() && RS->hasResult()) {
auto returnType = RS->getResult()->getType();
if (returnType && returnType->isUninhabited())
BS->setElement(0, RS->getResult());
}
}
}
}

FD->setBody(BS);
return HadError;
}
Expand Down
1 change: 1 addition & 0 deletions lib/Sema/TypeChecker.h
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ enum ContextualTypePurpose {
CTP_Unused, ///< No contextual type is specified.
CTP_Initialization, ///< Pattern binding initialization.
CTP_ReturnStmt, ///< Value specified to a 'return' statement.
CTP_ReturnSingleExpr, ///< Value implicitly returned from a function.
CTP_YieldByValue, ///< By-value yield operand.
CTP_YieldByReference, ///< By-reference yield operand.
CTP_ThrowStmt, ///< Value specified to a 'throw' statement.
Expand Down
Loading