Skip to content

[Diagnostics] Introduce extraneous call fix #27976

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
Oct 31, 2019
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
38 changes: 38 additions & 0 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5453,3 +5453,41 @@ bool ExpandArrayIntoVarargsFailure::diagnoseAsNote() {
}
return false;
}

bool ExtraneousCallFailure::diagnoseAsError() {
auto &cs = getConstraintSystem();

auto *anchor = getAnchor();
auto *locator = getLocator();

// If this is something like `foo()` where `foo` is a variable
// or a property, let's suggest dropping `()`.
auto removeParensFixIt = [&](InFlightDiagnostic &diagnostic) {
auto *argLoc = cs.getConstraintLocator(getRawAnchor(),
ConstraintLocator::ApplyArgument);

if (auto *TE =
dyn_cast_or_null<TupleExpr>(simplifyLocatorToAnchor(argLoc))) {
if (TE->getNumElements() == 0) {
diagnostic.fixItRemove(TE->getSourceRange());
}
}
};

if (auto overload = getChoiceFor(cs.getCalleeLocator(locator))) {
if (auto *decl = overload->choice.getDeclOrNull()) {
if (auto *enumCase = dyn_cast<EnumElementDecl>(decl)) {
auto diagnostic = emitDiagnostic(
anchor->getLoc(), diag::unexpected_arguments_in_enum_case,
enumCase->getName());
removeParensFixIt(diagnostic);
return true;
}
}
}

auto diagnostic = emitDiagnostic(
anchor->getLoc(), diag::cannot_call_non_function_value, getType(anchor));
removeParensFixIt(diagnostic);
return true;
}
9 changes: 9 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -1912,6 +1912,15 @@ class MissingForcedDowncastFailure final : public ContextualFailure {
bool diagnoseAsError() override;
};

class ExtraneousCallFailure final : public FailureDiagnostic {
public:
ExtraneousCallFailure(Expr *expr, ConstraintSystem &cs,
ConstraintLocator *locator)
: FailureDiagnostic(expr, cs, locator) {}

bool diagnoseAsError() override;
};

} // end namespace constraints
} // end namespace swift

Expand Down
10 changes: 10 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -984,3 +984,13 @@ AllowArgumentMismatch::create(ConstraintSystem &cs, Type argType,
return new (cs.getAllocator())
AllowArgumentMismatch(cs, argType, paramType, locator);
}

bool RemoveInvalidCall::diagnose(Expr *root, bool asNote) const {
ExtraneousCallFailure failure(root, getConstraintSystem(), getLocator());
return failure.diagnose(asNote);
}

RemoveInvalidCall *RemoveInvalidCall::create(ConstraintSystem &cs,
ConstraintLocator *locator) {
return new (cs.getAllocator()) RemoveInvalidCall(cs, locator);
}
19 changes: 19 additions & 0 deletions lib/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,10 @@ enum class FixKind : uint8_t {
/// If an array was passed to a variadic argument, give a specific diagnostic
/// and offer to drop the brackets if it's a literal.
ExpandArrayIntoVarargs,

/// Remove extraneous call to something which can't be invoked e.g.
/// a variable, a property etc.
RemoveCall,
};

class ConstraintFix {
Expand Down Expand Up @@ -1469,6 +1473,21 @@ class CoerceToCheckedCast final : public ContextualMismatch {
Type toType, ConstraintLocator *locator);
};

class RemoveInvalidCall final : public ConstraintFix {
RemoveInvalidCall(ConstraintSystem &cs, ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::RemoveCall, locator) {}

public:
std::string getName() const {
return "remove extraneous call from value of non-function type";
}

bool diagnose(Expr *root, bool asNote = false) const;

static RemoveInvalidCall *create(ConstraintSystem &cs,
ConstraintLocator *locator);
};

} // end namespace constraints
} // end namespace swift

Expand Down
34 changes: 32 additions & 2 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7097,8 +7097,37 @@ ConstraintSystem::simplifyApplicableFnConstraint(
}

// Handle applications of @dynamicCallable types.
return simplifyDynamicCallableApplicableFnConstraint(type1, origType2,
subflags, locator);
auto result = simplifyDynamicCallableApplicableFnConstraint(
type1, origType2, subflags, locator);

if (shouldAttemptFixes() && result == SolutionKind::Error) {
// Skip this fix if the type is not yet resolved or
// it's a function type/metatype which points to argument mismatches.
if (desugar2->is<TypeVariableType>() || desugar2->is<FunctionType>() ||
desugar2->is<AnyMetatypeType>())
return SolutionKind::Error;

if (auto objectTy = desugar2->lookThroughAllOptionalTypes()) {
if (objectTy->isAny() || objectTy->isAnyObject())
return SolutionKind::Error;
}

// If there are any type variables associated with arguments/result
// they have to be marked as "holes".
type1.visit([&](Type subType) {
if (auto *typeVar = subType->getAs<TypeVariableType>())
recordHole(typeVar);
});

auto *fix = RemoveInvalidCall::create(*this, getConstraintLocator(locator));
// Let's make this fix as high impact so if there is a function or member
// overload with e.g. argument-to-parameter type mismatches it would take
// a higher priority.
return recordFix(fix, /*impact=*/10) ? SolutionKind::Error
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we're developing the need for more a principled priority mechanism among fixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's something we need to think about for the long term, but logic involved there would be even more complicated than CSRanking, so I'm postponing that as much as possible. So far "weight" such as what impact really is seems to be doing the job well enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't specific to this change or something that needs to be addressed now, but what do you think about making impact a method on ConstraintFix that subclasses could override as needed? I think it would make it easier to determine the relative importance of fixes by just scanning the CSFix header.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, didn't see @hborla's comment before posting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about doing that as well, but some far most of the impact changes where contextual in nature. Although maybe that's a nice long-hanging fruit to tackle for clarity...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My hope is once we finish porting majority of diagnostics we would become more clear whether we can stick to weight or need something more comprehensive than that...

: SolutionKind::Solved;
}

return result;
}

/// Looks up and returns the @dynamicCallable required methods (if they exist)
Expand Down Expand Up @@ -8012,6 +8041,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::UseSubscriptOperator:
case FixKind::ExplicitlyEscaping:
case FixKind::RelabelArguments:
case FixKind::RemoveCall:
case FixKind::RemoveUnwrap:
case FixKind::DefineMemberBasedOnUse:
case FixKind::AllowMemberRefOnExistential:
Expand Down
49 changes: 25 additions & 24 deletions test/Constraints/closures.swift
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ func r21544303() {
inSubcall = false

var v2 : Bool = false
v2 = inSubcall
{ // expected-error {{cannot call value of non-function type 'Bool'}} expected-note {{did you mean to use a 'do' statement?}} {{3-3=do }}
v2 = inSubcall // expected-error {{cannot call value of non-function type 'Bool'}}
{
}
}

Expand All @@ -114,56 +114,56 @@ func SR3671() {
{ consume($0) }(42)
;

({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
({ $0(42) } { consume($0) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ print(42) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-note {{did you mean to use a 'do' statement?}} {{3-3=do }} expected-error {{cannot call value of non-function type '()'}}
{ print(42) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-note {{did you mean to use a 'do' statement?}} {{3-3=do }}
;

({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
({ $0(42) } { consume($0) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ print($0) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
{ print($0) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}}
;

({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
({ $0(42) } { consume($0) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ [n] in print(42) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
{ [n] in print(42) } // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}}
;

({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
({ $0(42) } { consume($0) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ consume($0) }(42) // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
{ consume($0) }(42) // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}}
;

({ $0(42) } { consume($0) }) // expected-note {{callee is here}}
({ $0(42) } { consume($0) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ (x: Int) in consume(x) }(42) // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}} expected-error {{cannot call value of non-function type '()'}}
{ (x: Int) in consume(x) }(42) // expected-warning {{braces here form a trailing closure separated from its callee by multiple newlines}}
;

// This is technically a valid call, so nothing goes wrong until (42)

{ $0(3) }
{ consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
{ $0(3) } // expected-error {{cannot call value of non-function type '()'}}
{ consume($0) }(42)
;
({ $0(42) })
{ consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
({ $0(42) }) // expected-error {{cannot call value of non-function type '()'}}
{ consume($0) }(42)
;
{ $0(3) }
{ [n] in consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
{ $0(3) } // expected-error {{cannot call value of non-function type '()'}}
{ [n] in consume($0) }(42)
;
({ $0(42) })
{ [n] in consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
({ $0(42) }) // expected-error {{cannot call value of non-function type '()'}}
{ [n] in consume($0) }(42)
;

// Equivalent but more obviously unintended.

{ $0(3) } // expected-note {{callee is here}}
{ $0(3) } // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
{ consume($0) }(42)
// expected-warning@-1 {{braces here form a trailing closure separated from its callee by multiple newlines}}

({ $0(3) }) // expected-note {{callee is here}}
({ $0(3) }) // expected-error {{cannot call value of non-function type '()'}} expected-note {{callee is here}}

{ consume($0) }(42) // expected-error {{cannot call value of non-function type '()'}}
{ consume($0) }(42)
// expected-warning@-1 {{braces here form a trailing closure separated from its callee by multiple newlines}}
;

Expand Down Expand Up @@ -407,6 +407,7 @@ func r20789423() {

let p: C
print(p.f(p)()) // expected-error {{cannot convert value of type 'C' to expected argument type 'Int'}}
// expected-error@-1:11 {{cannot call value of non-function type '()'}}

let _f = { (v: Int) in // expected-error {{unable to infer complex closure return type; add explicit type to disambiguate}} {{23-23=-> String }}
print("a")
Expand Down
6 changes: 3 additions & 3 deletions test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func rdar20142523() {
func rdar21080030() {
var s = "Hello"
// SR-7599: This should be `cannot_call_non_function_value`
if s.count() == 0 {} // expected-error{{cannot invoke 'count' with no arguments}}
if s.count() == 0 {} // expected-error{{cannot call value of non-function type 'Int'}} {{13-15=}}
}

// <rdar://problem/21248136> QoI: problem with return type inference mis-diagnosed as invalid arguments
Expand Down Expand Up @@ -530,8 +530,8 @@ let _: Color = .frob(1, &d) // expected-error {{missing argument label 'b:' in c
let _: Color = .frob(1, b: &d) // expected-error {{cannot convert value of type 'Double' to expected argument type 'Int'}}
var someColor : Color = .red // expected-error {{enum type 'Color' has no case 'red'; did you mean 'Red'}}
someColor = .red // expected-error {{enum type 'Color' has no case 'red'; did you mean 'Red'}}
someColor = .svar() // expected-error {{static property 'svar' is not a function}}
someColor = .svar(1) // expected-error {{static property 'svar' is not a function}}
someColor = .svar() // expected-error {{cannot call value of non-function type 'Color'}}
someColor = .svar(1) // expected-error {{cannot call value of non-function type 'Color'}}

func testTypeSugar(_ a : Int) {
typealias Stride = Int
Expand Down
4 changes: 2 additions & 2 deletions test/Index/invalid_code.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class CrashTest {
init() { }
}
// CHECK: [[@LINE+1]]:13 | instance-method/Swift | returnSelf
CrashTest().returnSelf(["": 0]).something()
CrashTest().returnSelf(["": 0]).something

class CrashTest2 {
// CHECK: [[@LINE+1]]:8 | instance-method/Swift | bar
Expand Down Expand Up @@ -48,4 +48,4 @@ extension Protector where T: RangeReplaceableCollection {
_ = newElement
}
}
}
}