[Sema] Diagnostic: improve diagnostics for ApplyExpr. #17093

dingobye · 2018-06-10T07:26:21Z

When diagnosing ApplyExpr, the existing implementation tries to resolve the function subexpression independently in the first place, without considering the argument information. As a result, such resolved function type may not produce the best diagnostic message.

This PR adds consideration of the number of arguments to decide the better function subexpression for diagnostic purpose.

It also improves a primary test case by proving better diagnostic to fix a FIXME.

Resolves SR-7918, SR-7786, SR-7440, SR-7295, SR-5154.

slavapestov · 2018-06-10T08:01:09Z

@xedin Want to take a look?

CodaFi · 2018-06-10T17:27:51Z

lib/Sema/CSDiag.cpp

+  // the first place, the resolved type may not provide the best diagnostic.
+  // We consider the number of arguments to decide whether we'd go with it or
+  // stay with the original one.
+  if (fnExpr != callExpr->getFn()) {


Just counting arguments isn't necessarily going to work for default arguments and variadics.

I agree with @CodaFi here, unfortunately it's not as simple as only matching number of parameters, it needs to account for defaults and variadics as well.

As an alternative approach - just add all of the overloads from original function to existing calleeInfo and filter via evaluateCloseness with the rest?

CodaFi · 2018-06-10T17:32:16Z

test/Sema/diag_ambiguous_overloads.swift

@@ -19,19 +19,17 @@ func fg<T>(_ f: (T) -> T) -> Void {} // expected-note {{in call to function 'fg'
 fg({x in x}) // expected-error {{generic parameter 'T' could not be inferred}}


-// FIXME: Both f & g should complain about ambiguity of arg1, but in both cases the generic member
-// never gets into the list of candidates in the first place.
 struct S {


You can look into why we aren't picking up these callees by examining the overload set passed to the constructor for CalleeCandidateInfo.

This is because the "fnExpr" passed to the constructor of CalleeCandidateInfo is redefined in this line, where the function subexpression is handled INDEPENDENTLY (i.e., without considering the actual argument information). In this case, the CS resolves an easy form of the function (e.g., the less formal parameters it has, the greater it is ranked). It may not be the best choice for diagnostics.

The motivation of this PR is to give it a chance to decide if we want to use the redefined "fnExpr" or the original one, taking into account the number of arguments.

After applying this PR, such FIXME would have been fixed, with the compiler raising better diagnostics.

dingobye · 2018-06-11T03:53:16Z

Thanks for the review. I updated the code to handle parameters with default values and variadics, as you suggested.

xedin · 2018-06-11T18:38:05Z

lib/Sema/CSDiag.cpp

@@ -5599,6 +5599,60 @@ static bool shouldTypeCheckFunctionExpr(TypeChecker &TC, DeclContext *DC,
  return true;
 }

+// Check if any of the candidates can accept a specified number of arguments,
+// regardless of parameter type or label information.
+static bool acceptNumArgs(const CalleeCandidateInfo &CCI, size_t numArgs) {


I think it would be great if logic that we already have in CSRanking could be extracted and re-used here as well, instead - https://github.com/apple/swift/blob/master/lib/Sema/CSRanking.cpp#L598-L701 it also handles trailing closures.

Thanks for leading the way to the relevant code, Pavel! It is not straight forward to reuse, though.

The logic seems to be different. Let's say we have a candidate with variadic parameter foo(_: Int...), and it accepts the argument list (1, 2, 3). The acceptNumArgs function here returns true, while the code snippet in CSRanking.cpp would not due to this line.

In addition, the code snippet is a bit tightly coupled, with some variables defined in outer scope, and some control flow statements, like here and here).

The problem is that do we really have to handle trailing closure? If the the argument number mismatch happens in the closure, this PR does not handle it and compiler's behaviour remains unchanged. I mean, it does not make things worse.

Sorry @dingobye I didn't mean that it has to be quiet literally the same, but I think it makes sense to extract common logic and leave irrelevant checks in their original places. I think the logic could be - given array of parameters can it match given array of arguments, maybe with additional callback to process each accepted argument/parameter pair further, this way it could be used by filter* functions in CalleeCandidateInfo as well.

I am trying to extract common logic, but have some troubles. It seems to produce an opposite result for the following case:

Let's say we have params1 = (_: Int, _: Int) and params2 = (_: Int...). The code in CSRank.cpp concludes params1 is NOT a subtype of params2. Please check this line. However, I would expect the opposite, i.e., a given array of parameters params2 can match a given array of arguments with params1's type (e.g., (0, 0)). Because a variadic Int parameter should match a list of Int arguments.

I get confused here. Did I miss something?

@dingobye I think it's correct behavior in this case because (according to comment) it tries to figure out if parameters from first function could be forwarded to the second, which in case of one having 2 parameters and another having a single variadic can't happen, same goes for the arguments with labels as well, they can't be forwarded to a single variadic parameter e.g.

func foo(_ params: Int...) {} // signature of parameters is (params: Int...) foo(x: 0, y: 1) // fail - signature of arguments is (x: Int, y: Int)

Hi Pavel, I updated the code as per your suggestions. Some logics are extracted, with variadic and defaulted parameters handled in general, and specific matching behaviours (e.g., type checking) are done via callback.

Thank you, @dingobye! I'll try to take a look over the weekend.

Thank you, Pavel! Please take your time.

Do you think it makes sense to rename this function to something more verbose like isViableOverloadSet?

Good idea! I was not so happy with the name.

xedin · 2018-06-11T18:43:47Z

test/Constraints/diagnostics.swift

@@ -376,7 +376,7 @@ CurriedClass.method3(c)(32, b: 1)
 _ = CurriedClass.method3(c)
 _ = CurriedClass.method3(c)(1, 2)        // expected-error {{missing argument label 'b:' in call}} {{32-32=b: }}
 _ = CurriedClass.method3(c)(1, b: 2)(32) // expected-error {{cannot call value of non-function type '()'}}
-_ = CurriedClass.method3(1, 2)           // expected-error {{instance member 'method3' cannot be used on type 'CurriedClass'; did you mean to use a value of this type instead?}}
+_ = CurriedClass.method3(1, 2)           // expected-error {{missing argument label 'b:' in call}}


I'm not actually sure if this is better diagnostic - it's indeed pointing to missing b: but the main problem here is that method3 can't be called on metatype like that...

I agree with your, Pavel. The original error message makes more sense. I updated the code to properly handle the case of instance method as curried member on type. Now the PR does not change this file.

xedin

I think new changes look much better! Thank you for all the work @dingobye! Sorry I've been traveling so I couldn't take a look sooner, I have left some comments and questions, but I think overall everything looks good, I just need to think some more about this. Going to run tests anyway to see if there is anything broken so you are aware.

xedin · 2018-06-19T03:20:06Z

lib/Sema/CSRanking.cpp

+}
+
+InputMatcher::Result InputMatcher::solve(int numInputs,
+    bool ignoreLastPair,


It looks like ignoreLastPair parameter is something specific to ranking it probably shouldn't be specially cased in the matcher itself, because it could be modeled outside by callee itself.

Agree. I removed such ignoreLastPair. In this case, the parameters should be properly prepared from outside by caller. If we intend to ignore trailing closure, we can slice params by dropping the last element like this, before passing it to InputMatcher's constructor.

As a side effect, however, computeDefaultMap does not produce expected results if we pass the sliced params to it. It means that we cannot directly compute defaultMap inside InputMatcher. As a result, defaultMap should be prepared outside by caller as well. So I added the corresponding updates.

xedin · 2018-06-19T03:20:32Z

lib/Sema/CSRanking.cpp

+      funcDecl->getDeclContext()->isTypeContext());
+}
+
+InputMatcher::Result InputMatcher::solve(int numInputs,


Maybe it makes sense to rename this to match instead of solve?

xedin · 2018-06-19T03:23:52Z

lib/Sema/CSRanking.cpp

+
+InputMatcher::Result InputMatcher::solve(int numInputs,
+    bool ignoreLastPair,
+    std::function<bool(unsigned, unsigned)> pairMatcher) {


It would be great if instead of indices callback received references to AnyFunctionType::Parameter, because index itself is inconsequential here.

I was thinking the same, Pavel. However, I found it not easy to check for IUO without indices. I think indices is not a bad idea, since it allows pairMatcher to take more control. What do you think?

xedin · 2018-06-19T03:28:22Z

lib/Sema/ConstraintSystem.h

@@ -3487,6 +3487,52 @@ class OverloadSetCounter : public ASTWalker {
    return { true, expr };
  }
 };
+
+
+/// \brief Matching an array of input to an array of parameter.


Alternative phrasing - Matches array of function parameters to candidate arguments.

xedin · 2018-06-19T03:30:41Z

test/Sema/diag_ambiguous_overloads.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -swift-version 4


Sorry I didn't actually comment on this right away but we can't really switch version to 4 here, we'd have to add additional test/Sema/diag_ambiguous_overloads_swift4.swift and have it specify -swift-version 4 there.

More interesting question is why -swift-version 4 is even required here, which tests change?

This is because SR-7295 compiles in Swift 3 while has error in Swift 4. Anyway, I shouldn't apply -swift-version 4 for all test cases in that file. I created another file to have SR-7295, as you suggested.

xedin · 2018-06-19T03:34:56Z

lib/Sema/CSDiag.cpp

@@ -5599,6 +5599,60 @@ static bool shouldTypeCheckFunctionExpr(TypeChecker &TC, DeclContext *DC,
  return true;
 }

+// Check if any of the candidates can accept a specified number of arguments,
+// regardless of parameter type or label information.
+static bool acceptNumArgs(const CalleeCandidateInfo &CCI, size_t numArgs) {


Do you think it makes sense to rename this function to something more verbose like isViableOverloadSet?

xedin · 2018-06-19T03:37:59Z

@swift-ci please test

dingobye

Thank you so much for your thorough review, Pavel! I updated the code as per your suggestions. swift-ci completed successfully before this update.

dingobye · 2018-06-19T09:22:05Z

lib/Sema/CSDiag.cpp

@@ -5599,6 +5599,60 @@ static bool shouldTypeCheckFunctionExpr(TypeChecker &TC, DeclContext *DC,
  return true;
 }

+// Check if any of the candidates can accept a specified number of arguments,
+// regardless of parameter type or label information.
+static bool acceptNumArgs(const CalleeCandidateInfo &CCI, size_t numArgs) {


Good idea! I was not so happy with the name.

dingobye · 2018-06-19T09:22:42Z

lib/Sema/CSRanking.cpp

+      funcDecl->getDeclContext()->isTypeContext());
+}
+
+InputMatcher::Result InputMatcher::solve(int numInputs,


dingobye · 2018-06-19T09:42:13Z

lib/Sema/CSRanking.cpp

+}
+
+InputMatcher::Result InputMatcher::solve(int numInputs,
+    bool ignoreLastPair,


Agree. I removed such ignoreLastPair. In this case, the parameters should be properly prepared from outside by caller. If we intend to ignore trailing closure, we can slice params by dropping the last element like this, before passing it to InputMatcher's constructor.

As a side effect, however, computeDefaultMap does not produce expected results if we pass the sliced params to it. It means that we cannot directly compute defaultMap inside InputMatcher. As a result, defaultMap should be prepared outside by caller as well. So I added the corresponding updates.

dingobye · 2018-06-19T09:49:11Z

lib/Sema/CSRanking.cpp

+
+InputMatcher::Result InputMatcher::solve(int numInputs,
+    bool ignoreLastPair,
+    std::function<bool(unsigned, unsigned)> pairMatcher) {


I was thinking the same, Pavel. However, I found it not easy to check for IUO without indices. I think indices is not a bad idea, since it allows pairMatcher to take more control. What do you think?

dingobye · 2018-06-19T09:52:36Z

test/Sema/diag_ambiguous_overloads.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -swift-version 4


This is because SR-7295 compiles in Swift 3 while has error in Swift 4. Anyway, I shouldn't apply -swift-version 4 for all test cases in that file. I created another file to have SR-7295, as you suggested.

xedin

@dingobye Sorry it's taking this long, I'll try to take a look again soon, unfortunately I'm currently on vacation...

Regarding paramIsIUO, as an idea, you could change constructor of the InputMatcher to pass original function/subscript declaration, extract parameter list from it, and modify paramIsIUO to accept ParamDecl directly, how does that sound? It's seems unnecessary for that function to always go through parent function/subscript anyway. This way you don't have to pass indices around, the reason why I don't like that - is because the same information has to be kept in two different places.

I think this might be the last request I have, but I will take one more look at it.

Also could you do me a favor and run clang-format on the modified files? You can do it paired with git as well.

dingobye · 2018-06-22T10:32:56Z

Hi Pavel, thank you for the reply. I feel bad if this bothers you during your vocation. Actually, we can continue our discussion after you return to work.

In the case of paramIsIUO, it definitely makes sense if we make it accept ParamDecl directly. Now, the problem is what parameter type to use for pairMatcher if we don't want indices? Should it be ParamDecl& or AnyFunctionType::Param&? Both types are used in pairMatcher, i.e., paramIsIUO would accept ParamDecl&, while maybeAddSubtypeConstraint accepts the other. It seems indices is easier to satisfy both. What's your opinion?

dingobye · 2018-07-09T07:51:18Z

@xedin Hi Pavel, how are you? Sorry for having troubled you during your vocation period. I just had the code rebased to recent master. It would be great if you can help finish this patch. Hope now is an appropriate time to bring you back to this conversation.

As per your latest comments, using indices as callback's parameters is not ideal. However, this callback function uses both AnyFunctionType::Param& and indices (or ParamDecl& if we turn maybeAddSubtypeConstraint to accept ParamDecl& directly). So, passing indices can achieve all these, otherwise we'd have to pass an additional parameter.

What do you think?

xedin · 2018-07-09T17:56:51Z

@dingobye Sorry, I'm back and will take a look ASAP. I think the topic you mentioned is the only outstanding thing which needs improvement, I'll see if I can come up with a good way to solve this, otherwise we can keep on using indices.

xedin · 2018-07-10T01:10:00Z

lib/Sema/CalleeCandidateInfo.cpp

+CalleeCandidateInfo &CalleeCandidateInfo::
+operator=(const CalleeCandidateInfo &CCI) {
+  if (this != &CCI) {
+    // If the reference member (i.e., CS) is identical, just copy the rest


I think the rest could be changed to remaining here.

xedin · 2018-07-10T01:15:17Z

lib/Sema/ConstraintSystem.h

+  };
+
+  InputMatcher(const ArrayRef<AnyFunctionType::Param> params,
+      const llvm::SmallBitVector &defaultValueMap);


Ok, here is an idea - instead of accepting a list of params here, let's give InputMatcher AbstractFunctionDecl where ParamDecls could be extracted via getParameterList (just like what paramisIUO does today), and let's make it so matcher callback uses ParamDecl everywhere, these decls should have all the information needed -label, type and flags. Let's try that, if it doesn't work out we'll go with current approach, but if it does that would be a very nice cleanup!

I tried and it seems not easy to achieve this...

The list of params passed to InputMatcher in CSRanking.cpp is from funcTy2 / openType2, which is not directly computed from decl2 and the computation is non-trivial (involving cs intermediately). If we'd like to compute such list of params locally in InputMatcher, the code for such computation would be bulky and some extra stuffs (e.g., cs) should also be passed to InputMatcher.

In addition, if the list of params is not prepared by the caller, we would have to pass additional information to InputMatcher(like ignoreLastPair in a previous version of this patch), which you dislike as in your previous comment on 19th June.

It seems we do not have much choice here. Shall we just use indices?

Alright, fair enough!

xedin · 2018-07-10T17:57:10Z

@swift-ci please test

xedin · 2018-07-10T18:02:45Z

@dingobye One last thing, could you please run clang-format on the changes before we merge?

dingobye · 2018-07-10T23:43:51Z

I updated code with git-clang-format applied.

Thank you for everything, Pavel! I couldn't have gone this far without your continued help.

xedin · 2018-07-10T23:46:03Z

No problem, thanks for all the work!

xedin · 2018-07-10T23:46:16Z

@swift-ci please test

swift-ci · 2018-07-11T01:15:00Z

Build failed
Swift Test OS X Platform
Git Sha - 00690dd5cc6b48aa67a38a548047cacd5ee16d41

xedin · 2018-07-11T01:26:38Z

I'm not sure what this Dictionary failure is all about so let's wait a bit and I'll re-run.

xedin · 2018-07-11T01:27:11Z

Looks like that should be addressed by #17872

swift-ci · 2018-07-11T01:28:44Z

Build failed
Swift Test Linux Platform
Git Sha - 00690dd5cc6b48aa67a38a548047cacd5ee16d41

When diagnosing ApplyExpr, the existing implementation tries to resolve the function subexpression independently in the first place, without considering the argument information. As a result, such resolved function type may not produce the best diagnostic message. This patch adds condideration of the number of arguments to decide the better function subexpression for diagnostic purpose. Resolves: SR-7918, SR-7786, SR-7440, SR-7295, SR-5154.

instance method as curried member on type.

and reuse it to match parameters and arguments.

split test cases into different files for different swift versions.

dingobye · 2018-07-11T05:30:12Z

Ugh... Thank you for letting me know such information, Pavel! Just had it rebased to include that patch. Would you fire a test again, please?

xedin · 2018-07-11T07:14:39Z

@swift-ci please test

swift-ci · 2018-07-11T07:16:22Z

Build failed
Swift Test Linux Platform
Git Sha - 00690dd5cc6b48aa67a38a548047cacd5ee16d41

swift-ci · 2018-07-11T07:16:25Z

Build failed
Swift Test OS X Platform
Git Sha - 00690dd5cc6b48aa67a38a548047cacd5ee16d41

xedin · 2018-07-11T16:25:42Z

@swift-ci please test source compatibility

xedin · 2018-07-11T18:20:39Z

Ok source compatibility is still busted, let's wait a bit until it's back online, I just want to make sure that everything is green before merging since we touched ranking logic.

xedin · 2018-07-11T22:04:13Z

@swift-ci please test source compatibility

dingobye · 2018-07-11T23:30:44Z

Sure, Pavel! I'd feel bad if this patch became a problem maker.

xedin · 2018-07-12T00:38:27Z

@dingobye Could you please now resolve all of the fixed issues? :)

dingobye · 2018-07-12T01:46:49Z

Issues tickets are resolved :)

xedin · 2018-07-12T01:50:23Z

Awesome! Thanks again for all the work!

slavapestov assigned xedin Jun 10, 2018

CodaFi reviewed Jun 10, 2018

View reviewed changes

xedin reviewed Jun 11, 2018

View reviewed changes

dingobye force-pushed the SR-7918 branch from 9f8ed26 to d7333ca Compare June 15, 2018 06:44

xedin reviewed Jun 19, 2018

View reviewed changes

dingobye commented Jun 19, 2018

View reviewed changes

xedin reviewed Jun 22, 2018

View reviewed changes

dingobye force-pushed the SR-7918 branch from da2b9d3 to 860bcfd Compare July 9, 2018 07:29

xedin reviewed Jul 10, 2018

View reviewed changes

dingobye added 3 commits July 11, 2018 15:26

Handle parameters with default values and variadics.

64274a9

Do not consider number of arguments when it comes to

3916452

instance method as curried member on type.

dingobye added 4 commits July 11, 2018 15:26

Extract some logic of matching parameters from isDeclAsSpecializedAs,

9648371

and reuse it to match parameters and arguments.

Improve interface of InputMatcher with some renaming and rephrasing;

4a05369

split test cases into different files for different swift versions.

Improve comment phrasing

e884126

Apply git-clang-format.

95b91ee

dingobye force-pushed the SR-7918 branch from 00690dd to 95b91ee Compare July 11, 2018 05:27

xedin merged commit 5976980 into swiftlang:master Jul 12, 2018

		@@ -1,4 +1,4 @@
		// RUN: %target-typecheck-verify-swift
		// RUN: %target-typecheck-verify-swift -swift-version 4

[Sema] Diagnostic: improve diagnostics for ApplyExpr. #17093

[Sema] Diagnostic: improve diagnostics for ApplyExpr. #17093

Uh oh!

Conversation

dingobye commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Jun 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye commented Jun 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin commented Jun 19, 2018

Uh oh!

dingobye left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dingobye commented Jun 10, 2018 •

edited

Loading

dingobye Jun 11, 2018 •

edited

Loading

dingobye left a comment •

edited

Loading