Skip to content

[Sema] Extend callee diagnosis to complex args including generics, e.g. (Void) -> T #1160

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
Feb 4, 2016
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
6 changes: 6 additions & 0 deletions include/swift/AST/TypeMatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,12 @@ class TypeMatcher {
/// FIXME: Split this out into cases?
bool visitAnyFunctionType(CanAnyFunctionType firstFunc, Type secondType) {
if (auto secondFunc = secondType->getAs<AnyFunctionType>()) {
// FIXME: Compare throws()? Both existing subclasses would prefer
// to mismatch on (!firstFunc->throws() && secondFunc->throws()), but
// embedding that non-commutativity in this general matcher is icky.
if (firstFunc->isNoEscape() != secondFunc->isNoEscape())
return mismatch(firstFunc.getPointer(), secondFunc);

return this->visit(firstFunc.getInput(), secondFunc->getInput()) &&
this->visit(firstFunc.getResult(), secondFunc->getResult());
}
Expand Down
112 changes: 94 additions & 18 deletions lib/Sema/CSDiag.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#include "ConstraintSystem.h"
#include "llvm/Support/SaveAndRestore.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/TypeWalker.h"
#include "swift/AST/TypeMatcher.h"

using namespace swift;
using namespace constraints;
Expand Down Expand Up @@ -836,7 +838,7 @@ namespace {
/// obviously mismatching candidates and compute a "closeness" for the
/// resultant set.
std::pair<CandidateCloseness, CalleeCandidateInfo::FailedArgumentInfo>
evaluateCloseness(Type candArgListType, ArrayRef<CallArgParam> actualArgs);
evaluateCloseness(DeclContext *dc, Type candArgListType, ArrayRef<CallArgParam> actualArgs);

void filterList(ArrayRef<CallArgParam> actualArgs);
void filterList(Type actualArgsType) {
Expand Down Expand Up @@ -980,11 +982,71 @@ static bool argumentMismatchIsNearMiss(Type argType, Type paramType) {
return false;
}

/// Given a parameter type that may contain generic type params and an actual
/// argument type, decide whether the param and actual arg have the same shape
/// and equal fixed type portions, and return by reference each archetype and
/// the matching portion of the actual arg type where that archetype appears.
static bool findGenericSubstitutions(DeclContext *dc, Type paramType, Type actualArgType,
SmallVector<ArchetypeType *, 4> &archetypes,
SmallVector<Type, 4> &substitutions) {
class GenericVisitor : public TypeMatcher<GenericVisitor> {
DeclContext *dc;
SmallVector<ArchetypeType *, 4> &archetypes;
SmallVector<Type, 4> &substitutions;

public:
GenericVisitor(DeclContext *dc,
SmallVector<ArchetypeType *, 4> &archetypes,
SmallVector<Type, 4> &substitutions)
: dc(dc), archetypes(archetypes), substitutions(substitutions) {}

bool mismatch(TypeBase *paramType, TypeBase *argType) {
return paramType->isEqual(argType);
}

bool mismatch(SubstitutableType *paramType, TypeBase *argType) {
Type type = paramType;
if (dc && type->isTypeParameter())
type = ArchetypeBuilder::mapTypeIntoContext(dc, paramType);

if (auto archetype = type->getAs<ArchetypeType>()) {
for (unsigned i = 0, c = archetypes.size(); i < c; i++) {
if (archetypes[i]->isEqual(archetype))
return substitutions[i]->isEqual(argType);
}
archetypes.push_back(archetype);
substitutions.push_back(argType);
return true;
}
return false;
}
};

// If paramType contains any substitutions already, find them and add them
// to our list before matching the two types to find more.
paramType.findIf([&](Type type) -> bool {
if (auto substitution = dyn_cast<SubstitutedType>(type.getPointer())) {
Type original = substitution->getOriginal();
if (dc && original->isTypeParameter())
original = ArchetypeBuilder::mapTypeIntoContext(dc, original);

if (auto archetype = original->getAs<ArchetypeType>()) {
archetypes.push_back(archetype);
substitutions.push_back(substitution->getReplacementType());
}
}
return false;
});

GenericVisitor visitor(dc, archetypes, substitutions);
return visitor.match(paramType, actualArgType);
}

/// Determine how close an argument list is to an already decomposed argument
/// list. If the closeness is a miss by a single argument, then this returns
/// information about that failure.
std::pair<CandidateCloseness, CalleeCandidateInfo::FailedArgumentInfo>
CalleeCandidateInfo::evaluateCloseness(Type candArgListType,
CalleeCandidateInfo::evaluateCloseness(DeclContext *dc, Type candArgListType,
ArrayRef<CallArgParam> actualArgs) {
auto candArgs = decomposeArgParamType(candArgListType);

Expand Down Expand Up @@ -1064,16 +1126,28 @@ CalleeCandidateInfo::evaluateCloseness(Type candArgListType,
// more parameters.
// We can still do something more sophisticated with this.
// FIXME: Use TC.isConvertibleTo?
if (rArgType->isEqual(paramType))

SmallVector<ArchetypeType *, 4> archetypes;
SmallVector<Type, 4> substitutions;
bool matched;
if (paramType->is<UnresolvedType>())
matched = false;
else
matched = findGenericSubstitutions(dc, paramType, rArgType,
archetypes, substitutions);

if (matched && archetypes.size() == 0)
continue;
if (auto genericParam = paramType->getAs<GenericTypeParamType>())
paramType = genericParam->getDecl()->getArchetype();
if (paramType->is<ArchetypeType>() && !rArgType->hasTypeVariable()) {
if (matched && archetypes.size() == 1 && !rArgType->hasTypeVariable()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hasTypeVariable() means the type contains a GenericTypeParamType or a DependentMemberType. I don't believe that can happen here since these are contextual types (written in terms of archetypes) and not interface types (written in terms of type parameters).

Maybe try changing that to an assert. If you're seeing both interface and contextual types here, there might be something else to look at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think you mean hasTypeParameter(). hasTypeVariable() is for if the type contains a TypeVariableType that gets inserted by the constraint system. By the time the solver is done there should be none left, but since this is in the couldn't-solve-it path, they can still exist.

This seems rare, but it does happen, like in test/Constraints/diagnostics.swift:644:

func foo() -> [Int] {
  return Array <Int> (count: 1) { // expected-error {{cannot invoke initializer for type 'Array<Int>' with an argument list of type '(count: Int, () -> _)'}}
    // expected-note @-1 {{expected an argument list of type '(count: Int, repeatedValue: Element)'}}
    return 1
  }
}

where rArgType (when checking the trailing closure param) is:

(function_type
  (input=tuple_type num_elements=0)
  (output=type_variable_type id=1))

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I meant hasTypeParameter(), thanks.

auto archetype = archetypes[0];
auto substitution = substitutions[0];

if (singleArchetype) {
if (!paramType->isEqual(singleArchetype))
if (!archetype->isEqual(singleArchetype))
// Multiple archetypes, too complicated.
return { CC_ArgumentMismatch, {}};
if (rArgType->isEqual(matchingArgType)) {

if (substitution->isEqual(matchingArgType)) {
if (nonSubstitutableArgs == 0)
continue;
++nonSubstitutableArgs;
Expand All @@ -1087,20 +1161,18 @@ CalleeCandidateInfo::evaluateCloseness(Type candArgListType,
// If we have only one nonSubstitutableArg so far, then this different
// type might be the one that we should be substituting for instead.
// Note that failureInfo is already set correctly for that case.
auto archetype = paramType->castTo<ArchetypeType>();
if (CS->TC.isSubstitutableFor(rArgType, archetype, CS->DC)) {
mismatchesAreNearMisses = argumentMismatchIsNearMiss(matchingArgType, rArgType);
matchingArgType = rArgType;
if (CS->TC.isSubstitutableFor(substitution, archetype, CS->DC)) {
mismatchesAreNearMisses = argumentMismatchIsNearMiss(matchingArgType, substitution);
matchingArgType = substitution;
continue;
}
}
}
} else {
matchingArgType = rArgType;
singleArchetype = paramType;
matchingArgType = substitution;
singleArchetype = archetype;

auto archetype = paramType->getAs<ArchetypeType>();
if (CS->TC.isSubstitutableFor(rArgType, archetype, CS->DC)) {
if (CS->TC.isSubstitutableFor(substitution, archetype, CS->DC)) {
continue;
}
++nonSubstitutableArgs;
Expand Down Expand Up @@ -1325,7 +1397,9 @@ void CalleeCandidateInfo::filterList(ArrayRef<CallArgParam> actualArgs) {
// If this isn't a function or isn't valid at this uncurry level, treat it
// as a general mismatch.
if (!inputType) return { CC_GeneralMismatch, {}};
return evaluateCloseness(inputType, actualArgs);
Decl *decl = candidate.getDecl();
return evaluateCloseness(decl ? decl->getInnermostDeclContext() : nullptr,
inputType, actualArgs);
});
}

Expand Down Expand Up @@ -3497,8 +3571,10 @@ bool FailureDiagnosis::visitSubscriptExpr(SubscriptExpr *SE) {
}

// Explode out multi-index subscripts to find the best match.
Decl *decl = cand.getDecl();
auto indexResult =
calleeInfo.evaluateCloseness(cand.getArgumentType(), decomposedIndexType);
calleeInfo.evaluateCloseness(decl ? decl->getInnermostDeclContext() : nullptr,
cand.getArgumentType(), decomposedIndexType);
if (selfConstraint > indexResult.first)
return {selfConstraint, {}};
return indexResult;
Expand Down
10 changes: 10 additions & 0 deletions test/Constraints/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ i.wobble() // expected-error{{value of type 'Int' has no member 'wobble'}}
// Generic member does not conform.
extension Int {
func wibble<T: P2>(x: T, _ y: T) -> T { return x }
func wubble<T>(x: Int -> T) -> T { return x(self) }
}
i.wibble(3, 4) // expected-error {{argument type 'Int' does not conform to expected type 'P2'}}

Expand All @@ -82,6 +83,15 @@ let a = A()
for j in i.wibble(a, a) { // expected-error {{type 'A' does not conform to protocol 'SequenceType'}}
}

// Generic as part of function/tuple types
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests involving typealiases and ensure the diagnostics are still useful and we don't fall back to the general path, and that the diagnostic involves the sugared typealias type and not what it points to

func f6<T:P2>(g: Void -> T) -> (c: Int, i: T) {
return (c: 0, i: g())
}
func f7() -> (c: Int, v: A) {
let g: Void -> A = { return A() }
return f6(g) // expected-error {{cannot convert return expression of type '(c: Int, i: A)' to return type '(c: Int, v: A)'}}
}

// <rdar://problem/19658691> QoI: Incorrect diagnostic for calling nonexistent members on literals
1.doesntExist(0) // expected-error {{value of type 'Int' has no member 'doesntExist'}}
[1, 2, 3].doesntExist(0) // expected-error {{value of type '[Int]' has no member 'doesntExist'}}
Expand Down
8 changes: 2 additions & 6 deletions test/Generics/function_defs.swift
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ func doCompare<T : EqualComparable, U : EqualComparable>(t1: T, t2: T, u: U) ->
return true
}

// FIXME: This is not ambiguous, the actual problem is that 'u' has the wrong
// type - "U" is not the same as "T".
return t1.isEqual(u) // expected-error {{type of expression is ambiguous without more context}}
return t1.isEqual(u) // expected-error {{cannot invoke 'isEqual' with an argument list of type '(U)'}} expected-note {{expected an argument list of type '(T)'}}
}

protocol MethodLessComparable {
Expand Down Expand Up @@ -171,9 +169,7 @@ func staticEqCheck<T : StaticEq, U : StaticEq>(t: T, u: U) {
if T.isEqual(t, y: t) { return }
if U.isEqual(u, y: u) { return }

// FIXME: This is not ambiguous, the actual problem is that 'u' has the wrong
// type - "U" is not the same as "T".
T.isEqual(t, y: u) // expected-error{{type of expression is ambiguous without more context}}
T.isEqual(t, y: u) // expected-error{{cannot invoke 'isEqual' with an argument list of type '(T, y: U)'}} expected-note {{expected an argument list of type '(T, y: T)'}}
}

//===----------------------------------------------------------------------===//
Expand Down
4 changes: 2 additions & 2 deletions test/Misc/misc_diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func testIS1() -> Int { return 0 }
let _: String = testIS1() // expected-error {{cannot convert value of type 'Int' to specified type 'String'}}

func insertA<T>(inout array : [T], elt : T) {
array.append(T); // expected-error {{ambiguous reference to member 'append'}}
array.append(T); // expected-error {{cannot invoke 'append' with an argument list of type '((T).Type)'}} expected-note {{expected an argument list of type '(T)'}}
}

// <rdar://problem/17875634> can't append to array of tuples
Expand Down Expand Up @@ -130,6 +130,6 @@ func test17875634() {

// <rdar://problem/20770032> Pattern matching ranges against tuples crashes the compiler
func test20770032() {
if case let 1...10 = (1, 1) { // expected-warning{{'let' pattern has no effect; sub-pattern didn't bind any variables}} {{11-15=}} expected-error{{cannot convert value of type '(Int, Int)' to expected argument type 'Range<Int>'}}
if case let 1...10 = (1, 1) { // expected-warning{{'let' pattern has no effect; sub-pattern didn't bind any variables}} {{11-15=}} expected-error{{expression pattern of type 'Range<Int>' cannot match values of type '(Int, Int)'}}
}
}
3 changes: 1 addition & 2 deletions validation-test/stdlib/StdlibUnittestStaticAssertions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ func test_expectType() {

func test_expectEqualType() {
expectEqualType(S1.self, S1.self)
expectEqualType(S1.self, S2.self) // expected-error {{cannot invoke 'expectEqualType' with an argument list of type '(S1.Type, S2.Type)'}}
// expected-note @-1 {{expected an argument list of type '(T.Type, T.Type)'}}
expectEqualType(S1.self, S2.self) // expected-error {{cannot convert value of type 'S2.Type' to expected argument type 'S1'}}
}