Skip to content

[ConstraintSystem] Infer empty closures as returning () more eagerly. #19282

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 1 commit into from
Sep 13, 2018
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
3 changes: 3 additions & 0 deletions include/swift/AST/Expr.h
Original file line number Diff line number Diff line change
Expand Up @@ -3653,6 +3653,9 @@ class ClosureExpr : public AbstractClosureExpr {
/// its body; it can only update that expression.
void setSingleExpressionBody(Expr *NewBody);

/// \brief Is this a completely empty closure?
bool hasEmptyBody() const;

static bool classof(const Expr *E) {
return E->getKind() == ExprKind::Closure;
}
Expand Down
4 changes: 4 additions & 0 deletions lib/AST/Expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1855,6 +1855,10 @@ void ClosureExpr::setSingleExpressionBody(Expr *NewBody) {
getBody()->setElement(0, NewBody);
}

bool ClosureExpr::hasEmptyBody() const {
return getBody()->getNumElements() == 0;
}

FORWARD_SOURCE_LOCS_TO(AutoClosureExpr, Body)

void AutoClosureExpr::setBody(Expr *E) {
Expand Down
32 changes: 20 additions & 12 deletions lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2400,22 +2400,30 @@ namespace {
expr->getExplicitResultTypeLoc().getType()) {
resultTy = expr->getExplicitResultTypeLoc().getType();
CS.setFavoredType(expr, resultTy.getPointer());
} else if (!crt.isNull()) {
resultTy = crt;
} else{
} else {
auto locator =
CS.getConstraintLocator(expr, ConstraintLocator::ClosureResult);

// If no return type was specified, create a fresh type
// variable for it.
resultTy = CS.createTypeVariable(locator);
if (expr->hasEmptyBody()) {
resultTy = CS.createTypeVariable(locator);

// Allow it to default to () if there are no return statements.
if (closureHasNoResult(expr)) {
CS.addConstraint(ConstraintKind::Defaultable,
resultTy,
TupleType::getEmpty(CS.getASTContext()),
locator);
// Closures with empty bodies should be inferred to return
// ().
CS.addConstraint(ConstraintKind::Bind, resultTy,
TupleType::getEmpty(CS.getASTContext()), locator);
} else if (crt) {
// Otherwise, use the contextual type if present.
resultTy = crt;
} else {
// If no return type was specified, create a fresh type
// variable for it.
resultTy = CS.createTypeVariable(locator);

if (closureHasNoResult(expr)) {
// Allow it to default to () if there are no return statements.
CS.addConstraint(ConstraintKind::Defaultable, resultTy,
TupleType::getEmpty(CS.getASTContext()), locator);
}
}
}

Expand Down
48 changes: 48 additions & 0 deletions test/Constraints/closures.swift
Original file line number Diff line number Diff line change
Expand Up @@ -749,3 +749,51 @@ func f20371273() {
let y: UInt = 4
_ = x.filter { ($0 + y) > 42 } // expected-error {{'+' is unavailable}}
}

// rdar://problem/42337247

func overloaded(_ handler: () -> Int) {} // expected-note {{found this candidate}}
func overloaded(_ handler: () -> Void) {} // expected-note {{found this candidate}}

overloaded { } // empty body => inferred as returning ()

overloaded { print("hi") } // single-expression closure => typechecked with body

overloaded { print("hi"); print("bye") } // multiple expression closure without explicit returns; can default to any return type
// expected-error@-1 {{ambiguous use of 'overloaded'}}

func not_overloaded(_ handler: () -> Int) {}

not_overloaded { } // empty body
// expected-error@-1 {{cannot convert value of type '() -> ()' to expected argument type '() -> Int'}}

not_overloaded { print("hi") } // single-expression closure
// expected-error@-1 {{cannot convert value of type '()' to closure result type 'Int'}}

// no error in -typecheck, but dataflow diagnostics will complain about missing return
not_overloaded { print("hi"); print("bye") } // multiple expression closure

func apply(_ fn: (Int) throws -> Int) rethrows -> Int {
return try fn(0)
}

enum E : Error {
case E
}

func test() -> Int? {
return try? apply({ _ in throw E.E })
}

var fn: () -> [Int] = {}
// expected-error@-1 {{cannot convert value of type '() -> ()' to specified type '() -> [Int]'}}

fn = {}
// expected-error@-1 {{cannot assign value of type '() -> ()' to type '() -> [Int]'}}

func test<Instances : Collection>(
_ instances: Instances,
_ fn: (Instances.Index, Instances.Index) -> Bool
) { fatalError() }

test([1]) { _, _ in fatalError(); () }
2 changes: 1 addition & 1 deletion test/SILGen/objc_blocks_bridging.swift
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ class Test: NSObject {
// CHECK: return [[RESULT]]

func clearDraggingItemImageComponentsProvider(_ x: NSDraggingItem) {
x.imageComponentsProvider = {}
x.imageComponentsProvider = { [] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a language change? Does it need to be guarded under -swift-version 5?

Copy link
Contributor Author

@rudkx rudkx Sep 12, 2018

Choose a reason for hiding this comment

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

I rewrote the commit message just before pushing and accidentally deleted the sentence about the updated test cases being ones where data flow diagnostics emit an error currently.

It looks like after reworking the approach to fixing this I also left in at least one test update that isn’t strictly necessary with my changes but isn’t harmful either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't comment about it because I tried second modified test-case and it did produce an error without return 0

}
// CHECK-LABEL: sil shared [transparent] [serializable] [reabstraction_thunk] @$SSayypGIego_So7NSArrayCSgIeyBa_TR
// CHECK: [[CONVERT:%.*]] = function_ref @$SSa10FoundationE19_bridgeToObjectiveCSo7NSArrayCyF
Expand Down
1 change: 1 addition & 0 deletions test/decl/func/default-values-swift4.swift
Original file line number Diff line number Diff line change
Expand Up @@ -109,4 +109,5 @@ public func evilCode(
versionedFunction()
// expected-error@-1 {{global function 'versionedFunction()' is internal and cannot be referenced from a default argument value}}
}
return 0
}()) {}
1 change: 1 addition & 0 deletions test/expr/closure/closures.swift
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ func rdar19179412() -> (Int) -> Int {
class A {
let d : Int = 0
}
return 0
}
}

Expand Down