Skip to content

[Constraint solver] Reject trailing closures matching non-closure-parameters #24403

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
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/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,9 @@ ERROR(extra_argument_to_nullary_call,none,
"argument passed to call that takes no arguments", ())
ERROR(extra_trailing_closure_in_call,none,
"extra trailing closure passed in call", ())
ERROR(trailing_closure_bad_param,none,
"trailing closure passed to parameter of type %0 that does not "
"accept a closure", (Type))
ERROR(no_accessible_initializers,none,
"%0 cannot be constructed because it has no accessible initializers",
(Type))
Expand Down
22 changes: 22 additions & 0 deletions lib/Sema/CSDiag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3517,6 +3517,28 @@ class ArgumentMatcher : public MatchCallArgumentListener {
return true;
}

bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx) override {
Expr *arg = ArgExpr;

auto tuple = dyn_cast<TupleExpr>(ArgExpr);
if (tuple)
arg = tuple->getElement(argIdx);

auto &param = Parameters[paramIdx];
TC.diagnose(arg->getLoc(), diag::trailing_closure_bad_param,
param.getPlainType())
.highlight(arg->getSourceRange());

auto candidate = CandidateInfo[0];
if (candidate.getDecl())
TC.diagnose(candidate.getDecl(), diag::decl_declared_here,
candidate.getDecl()->getFullName());

Diagnosed = true;

return true;
}

bool diagnose() {
// Use matchCallArguments to determine how close the argument list is (in
// shape) to the specified candidates parameters. This ignores the
Expand Down
28 changes: 28 additions & 0 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ bool MatchCallArgumentListener::relabelArguments(ArrayRef<Identifier> newNames){
return true;
}

bool MatchCallArgumentListener::trailingClosureMismatch(
unsigned paramIdx, unsigned argIdx) {
return true;
}

/// Produce a score (smaller is better) comparing a parameter name and
/// potentially-typo'd argument name.
///
Expand Down Expand Up @@ -205,6 +210,21 @@ static ConstraintSystem::TypeMatchOptions getDefaultDecompositionOptions(
return flags | ConstraintSystem::TMF_GenerateConstraints;
}

/// Determine whether the given parameter can accept a trailing closure.
static bool acceptsTrailingClosure(const AnyFunctionType::Param &param) {
Type paramTy = param.getPlainType();
if (!paramTy)
return true;

paramTy = paramTy->lookThroughAllOptionalTypes();
return paramTy->isTypeParameter() ||
paramTy->is<ArchetypeType>() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

How precise do you want to get here? If it has any constraints at all, it can't accept a function value (just like non-Any existentials)

paramTy->is<AnyFunctionType>() ||
paramTy->isTypeVariableOrMember() ||
paramTy->is<UnresolvedType>() ||
paramTy->isAny();
}

// FIXME: This should return ConstraintSystem::TypeMatchResult instead
// to give more information to the solver about the failure.
bool constraints::
Expand Down Expand Up @@ -425,6 +445,14 @@ matchCallArguments(ArrayRef<AnyFunctionType::Param> args,

// If we have a trailing closure, it maps to the last parameter.
if (hasTrailingClosure && numParams > 0) {
// If there is no suitable last parameter to accept the trailing closure,
// notify the listener and bail if we need to.
if (!acceptsTrailingClosure(params[numParams - 1])) {
if (listener.trailingClosureMismatch(numParams - 1, numArgs - 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

In the spirit of matchCallArguments used only to match parameters to arguments based on labels and positions, it seems that better place for this logic could be in other overload of matchCallArguments where types are matched, the problem with that though is that we currently don't diagnose missing parameters via new diagnostic framework, that would have made diagnosing trailing closures much easier...

return true;
}

// Claim the parameter/argument pair.
claimedArgs[numArgs-1] = true;
++numClaimedArgs;
parameterBindings[numParams-1].push_back(numArgs-1);
Expand Down
4 changes: 4 additions & 0 deletions lib/Sema/CalleeCandidateInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,10 @@ CalleeCandidateInfo::ClosenessResultTy CalleeCandidateInfo::evaluateCloseness(
result = CC_ArgumentLabelMismatch;
return true;
}
bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx) override {
result = CC_ArgumentMismatch;
return true;
}
} listener;

// Use matchCallArguments to determine how close the argument list is (in
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -3700,6 +3700,13 @@ class MatchCallArgumentListener {
/// \returns true to indicate that this should cause a failure, false
/// otherwise.
virtual bool relabelArguments(ArrayRef<Identifier> newNames);

/// Indicates that the trailing closure argument at the given \c argIdx
/// cannot be passed to the last parameter at \c paramIdx.
///
/// \returns true to indicate that this should cause a failure, false
/// otherwise.
virtual bool trailingClosureMismatch(unsigned paramIdx, unsigned argIdx);
};

/// Match the call arguments (as described by the given argument type) to
Expand Down
11 changes: 6 additions & 5 deletions test/Constraints/diag_missing_arg.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,20 @@ trailingClosureSingle1() { 1 } // expected-error {{missing argument for paramete

func trailingClosureSingle2(x: () -> Int, y: Int) {} // expected-note * {{here}}
// FIXME: Bad diagnostics.
trailingClosureSingle2 { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{23-23=(x: <#() -> Int#>)}}
trailingClosureSingle2() { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{24-24=x: <#() -> Int#>}}
trailingClosureSingle2 { 1 } // expected-error {{cannot convert value of type '() -> Int' to expected argument type '(x: () -> Int, y: Int)'}}
trailingClosureSingle2() { 1 } // expected-error {{cannot convert value of type '() -> Int' to expected argument type '(x: () -> Int, y: Int)'}}

func trailingClosureMulti1(x: Int, y: Int, z: () -> Int) {} // expected-note * {{here}}
trailingClosureMulti1(y: 1) { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{23-23=x: <#Int#>, }}
trailingClosureMulti1(x: 1) { 1 } // expected-error {{missing argument for parameter 'y' in call}} {{27-27=, y: <#Int#>}}
trailingClosureMulti1(x: 1, y: 1) // expected-error {{missing argument for parameter 'z' in call}} {{33-33=, z: <#() -> Int#>}}

func trailingClosureMulti2(x: Int, y: () -> Int, z: Int) {} // expected-note * {{here}}
trailingClosureMulti2 { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{22-22=(x: <#Int#>)}}
trailingClosureMulti2 { 1 } // expected-error {{cannot convert value of type '() -> Int' to expected argument type '(x: Int, y: () -> Int, z: Int)'}}
// FIXME: Bad diagnostics.
trailingClosureMulti2() { 1 } // expected-error {{missing argument for parameter 'x' in call}} {{23-23=x: <#Int#>}}
trailingClosureMulti2(x: 1) { 1 } // expected-error {{missing argument for parameter 'y' in call}} {{27-27=, y: <#() -> Int#>}}
trailingClosureMulti2() { 1 } // expected-error {{cannot convert value of type '() -> Int' to expected argument type '(x: Int, y: () -> Int, z: Int)'}}
trailingClosureMulti2(x: 1) { 1 } // expected-error {{cannot invoke 'trailingClosureMulti2' with an argument list of type '(x: Int, @escaping () -> Int)'}}
// expected-note@-1{{expected an argument list of type '(x: Int, y: () -> Int, z: Int)'}}

func param2Func(x: Int, y: Int) {} // expected-note * {{here}}
param2Func(x: 1) // expected-error {{missing argument for parameter 'y' in call}} {{16-16=, y: <#Int#>}}
Expand Down
11 changes: 11 additions & 0 deletions test/Constraints/overload_filtering.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,14 @@ func testUnresolvedMember(i: Int) -> X {
// CHECK-NEXT: introducing single enabled disjunction term {{.*}} bound to decl overload_filtering.(file).X.init(_:_:)
return .init(i, i)
}

func trailing(x: Int = 0, y: () -> Void) { }
func trailing(x: Int = 0, z: Float) { }

func testTrailing() {
// CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
trailing() { }

// CHECK: disabled disjunction term {{.*}} bound to decl overload_filtering.(file).trailing(x:z:)
trailing(x: 5) { }
}
4 changes: 2 additions & 2 deletions test/expr/closure/trailing.swift
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ func rdar17965209_test() {
func limitXY(_ xy:Int, toGamut gamut: [Int]) {}
let someInt = 0
let intArray = [someInt]
limitXY(someInt, toGamut: intArray) {} // expected-error {{extra argument 'toGamut' in call}}

limitXY(someInt, toGamut: intArray) {} // expected-error{{cannot invoke 'limitXY' with an argument list of type '(Int, toGamut: [Int], @escaping () -> ())'}}
// expected-note@-1{{expected an argument list of type '(Int, toGamut: [Int])'}}

// <rdar://problem/23036383> QoI: Invalid trailing closures in stmt-conditions produce lowsy diagnostics
func retBool(x: () -> Int) -> Bool {}
Expand Down