[ConstraintSystem] Diagnose label issues individually #30444

omochi · 2020-03-17T14:57:58Z

Summary

Diagnose label mismatch based on argument mapping which is used by type check in after repair stage
Diagnose all out of order labels
Diagnose label mismatch and out of order same time
Split diagnostic into for each individual issues.

@xedin Hi again.
I retried many things and got betters.
Please review this.

Change Log

Add about out of order at 2020/03/18 11:34 JST.

Discussion

After label issues (mismatch or out of order) happen in matchCallArgument, type checking is continued for diagnostics.
Such type checking uses mapping between arguments and parameters which is built by matchCallArgument with many consideration.
For example, typo correction is used to map them.
When both label and type check issues happen, their behavior should be consistent.
And mapping may contains multiple out of ordered labels and mismatches.
To diagnose all what happened here is useful for user.
Because it tells user to how mapping is used by compiler.

But currently some of them are not achieved.

Label mismatch diagnostics are built independently without referring to mapping (parameterBindings variable) for type checking.
It causes inconsistent error message between labels and types in corner case.

Reordering diagnostics is consistent with type checking.
But it works only limited condition and just one.

Reordering and mismatch diagnostics are exclusive.
And relabeling diagnostic have own calculation of out of order impacts.

For example,

func f(aa: Int, bb: Int, _ cc: Int, dd: Int, ee: Int..., ff: Int = 0, gg: Int) {}

f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)

This call has multiple label mismatch and out of order.
To make it easy for users to understand, each issues should be diagnosed individually.
Because, unified mismatch message with multiple labels which is twisted reordering
is hard to understand which parameter is mapped by each argument.

With this patch, diagnostics will be following.

$ swift -frontend -typecheck a.swift
a.swift:3:19: error: incorrect argument label in call (have 'aax:', expected 'aa:')
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
                  ^~~
                  aa
a.swift:3:12: error: extraneous argument label 'cc:' in call
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
           ^~~~
           
a.swift:3:27: error: missing argument label 'dd:' in call
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
                          ^
                          dd: 
a.swift:3:19: error: argument 'aax' must precede argument 'bb'
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
    ~~~~~       ~~^~~~~~
    aax: 1,     
a.swift:3:37: error: argument 'ee' must precede argument 'gg'
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
                             ~~~~~~~^~~~~~~~~~~~~~
                             ee: 55, 56, 57,

Current is:

$ xcrun --toolchain org.swift.50202003131a swift -frontend -typecheck a.swift
a.swift:3:4: error: incorrect argument labels in call (have 'bb:cc:aax:_:gg:ee:_:_:', expected 'aa:bb:_:dd:ee:ff:gg:')
  f(bb: 2, cc: 3, aax: 1, 4, gg: 6, ee: 55, 56, 57)
   ^~~     ~~     ~~~~~      ~~     ~~
    aa     bb             dd:  ee   ff      gg:

Design

Remove MatchCallArgumentListener::relabelArguments
matchCallArgument does not use diagnoseArgumentLabelFailure anymore
Add RelabelSingleArgument fix
Add SingleLabelingFailure diagnostic
Add note message for extra label and missing label.
New reordering algorithm

About out of order

What I mean by all out of order,
which arguments are moved from right to left so that all arguments are in correct order.
If argument needs to move, it is out of order.
In this rule, argument index number and parameter index number is not related.
Only the relative order in the arguments is relevant.
The correct order is determined based on parameters mapped before.

This change may increase out of order in some situation.

Examples

This patch:

func f(aa: Int, bb: Int, cc: Int, dd: Int) {}
f(bb: 1, cc: 2, dd: 3, aa: 0)
// [1] only aa is out of order

f(dd: 3, cc: 2, bb: 1, aa: 0)
// [3] cc, bb, aa are out of order

f(dd: 3, bb: 1, cc: 2, aa: 0)
// [3] bb, cc, aa are out of order

Current:

func f(aa: Int, bb: Int, cc: Int, dd: Int) {}
f(bb: 1, cc: 2, dd: 3, aa: 0)
// [4] all are out of order

f(dd: 3, cc: 2, bb: 1, aa: 0)
// [4] all are out of order

f(dd: 3, bb: 1, cc: 2, aa: 0)
// [2] dd, aa are out of order

Concern about impact value

This patch removes MatchCallArgumentListener::relabelArguments and (extraneous|missing|incorrect)Labels emits individual constraint fixes.
So I removed shared base impact point of RelabelArgument.

Base is 1 in

swift/lib/Sema/CSSimplify.cpp

Line 925 in 77af77f

auto impact = 1 + numOutOfOrder + numExtraneous * 2 + numRenames * 3 +

New impacts are:

extraneousLabel	missingLabel	incorrectLabel
3	1	4

They includes base 1.
So if more than one issues, accumulated impact will be greater than current.

But this 1 is important.

See this:

func f() {}
func f(aa: Int) {}

f(bb: "x")

Currently choice f().

$ xcrun --toolchain org.swift.50202003131a swift -frontend -typecheck a.swift
a.swift:4:9: error: argument passed to call that takes no arguments
  f(bb: "x")
        ^~~

Because,

for f(), fix impact is 5 (missing argument).
for f(aa:), fix impact is 2 (type error) + 1 (relabel base) + 3 (incorrect label) = 6.

If I choice 3 for incorrect label,
this overload will be ambiguous.
I considered that this is bad regression.

Of course, it is all under the incorrect source code.
It could be changeable.

Contrarily, one test case r27212391 in diagnostics.swift changed to ambiguous error.

xedin

Overall looks great, changes are going in the direction I had in mind. I've left some comments inline, some of which we need to address before merging.

xedin · 2020-03-18T22:58:59Z

lib/Sema/CSFix.h

@@ -165,6 +165,10 @@ enum class FixKind : uint8_t {
  /// Allow single tuple closure parameter destructuring into N arguments.
  AllowClosureParameterDestructuring,

+  /// A single argument have labeling failure - missing/extraneous or incorrect
+  /// label attached to the, fix it by suggesting proper label.


Let's adjust wording a bit:

Labeling failure associated with a particular argument, could be a missing, extraneous or incorrect label. Let fix the problem by suggesting a new label aligned with parameter at the same position.

OK, thanks 🙂

xedin · 2020-03-18T23:04:02Z

lib/Sema/CSFix.h

+
+  bool coalesceAndDiagnose(const Solution &solution,
+                           ArrayRef<ConstraintFix *> secondaryFixes,
+                           bool asNote = false) const override;


Please see my comment for incorrectLabel, neither RelabelSingleArgument nor MoveOutOfOrderArgument should implement coalesceAndDiagnose since this is supposed to represent a labeling failure with a single argument.

xedin · 2020-03-18T23:05:21Z

lib/Sema/CSFix.h

@@ -1184,6 +1188,33 @@ class RemoveExtraneousArguments final
  }
 };

+class RelabelSingleArgument final : public ConstraintFix {


Nit: I think we can simplify the class and case names down to RelabelArgument since having singular form is indicative enough...

xedin · 2020-03-18T23:05:45Z

lib/Sema/CSFix.h

  std::string getName() const override {
    return "move out-of-order argument to correct position";
  }

  bool diagnose(const Solution &solution, bool asNote = false) const override;

+  bool coalesceAndDiagnose(const Solution &solution,
+                           ArrayRef<ConstraintFix *> secondaryFixes,
+                           bool asNote = false) const override;


Same comment here as with RelabelSingleArgument.

xedin · 2020-03-18T23:10:52Z

lib/Sema/CSSimplify.cpp

+
+    auto *fix = RelabelSingleArgument::create(CS, Bindings[paramIndex].front(),
+                                              Parameters[paramIndex].getLabel(),
+                                              CS.getConstraintLocator(Locator));


I think we should use a more specific locator here because Locator points to a call itself in this case, it should be extended the same way as in case of synthesized arguments.

OK, I change approach so.

Actually I have tried to locate this fix at CallExpr -> apply argument -> arg to param.
It could print multiple diagnostics without using coalesceAndDiagnose.
But another problem happened on it.

When both type error and label error exists,

e.g.

func f(aa: Int, bb: Int) {} f(aax: "0", bb: 1)

This logic detects labeling failure as other argument mismatch.

swift/lib/Sema/CSSimplify.cpp

Lines 3422 to 3433 in f4fee7a

if (llvm::any_of(getFixes(), [&](const ConstraintFix *fix) {

auto *locator = fix->getLocator();

// Since arguments to @dynamicCallable form either an array

// or a dictionary and all have to match the same element type,

// let's allow multiple invalid arguments.

if (locator->findFirst<LocatorPathElt::DynamicCallable>())

return false;

return locator->findLast<LocatorPathElt::ApplyArgToParam>()

? locator->getAnchor() == anchor

: false;

}))

And solver stopped with ambiguous expression.

I will try to avoid this problem and make them works.

I'd suggest you to try and use a new locator element Label specifically for re-labeling failures.

I thought that we could handle different kinds of fixes with the same locator but apparently that is not so.

I will try to add new locator element.

xedin · 2020-03-18T23:35:25Z

test/APINotes/basic.swift

@@ -24,7 +24,9 @@ func testSelectors(a: AnyObject) {

 func testSwiftName() {
  moveTo(x: 0, y: 0, z: 0)
-  moveTo(0, 0, 0) // expected-error{{missing argument labels 'x:y:z:' in call}}
+  moveTo(0, 0, 0) // expected-error {{missing argument label 'x:' in call}}


If all of the arguments have labeling failures maybe it makes sense to have a single diagnostic instead of N?

Oh I see.
This case is clearly better to aggregate to one diagnostic.

I will implement aggregation
when all labeling issues are same category.

xedin · 2020-03-18T23:43:39Z

lib/Sema/CSSimplify.cpp

-      }
-    }
+    const auto bindings = ArgumentBindingHelper::fromParameterBindings(
+        parameterBindings, numArgs);


It seems like all of the logic associated with ArgumentBindingHelper could be inlined here instead.

Another question - are we only aggregating data into ArgumentBindingHelper in order to discount variadic arguments?

Purpose of ArgumentBindingHelper is multiple.

(1) Skip to parameter which has default and not mapped to any argument.

(2) Skip to parameter which hasn't default and not mapped to any argument.
It's missing argument.

(3) Skip to argument which is at variadic tails.

(4) Skip to argument which isn't mapped to any parameter.
It's extra argument.

In other words, to focus reordering,
it picks established pairs of argument and parameters.

Answer is yes.
I need argument indices which are bounded to some parameter and not variadic tail here.

for (unsigned leftArgIdx = 0; leftArgIdx < binding.argIdx; leftArgIdx++) {

(1), (2) can be got from parameterBindings efficiently.
(4) can be got from claimedArgs efficiently.

Okay, that what I thought, the question is whether it could be all done in a single loop? It seems to be that it could be done?

OK, I remove ArgumentBindingHelper and make as simple as possible here.

xedin · 2020-03-18T23:46:36Z

lib/Sema/CSSimplify.cpp

+
+    const auto argIdx = parameterBinding[0];
+    // extra argument
+    if (numArgs <= argIdx)


Should it be strictly less here?

Oh, comment extra argument is wrong. sorry.
Here is expected to skip synthesized argument from missing argument.

If parameter is unbound,
ArgumentFailureTracker generates dummy argument and bind it.

Generation:

swift/lib/Sema/CSSimplify.cpp

Line 813 in f4fee7a

unsigned newArgIdx = Arguments.size();

Binding:

swift/lib/Sema/CSSimplify.cpp

Lines 638 to 641 in f4fee7a

if (auto newArgIdx = listener.missingArgument(paramIdx)) {

parameterBindings[paramIdx].push_back(*newArgIdx);

continue;

}

numArgs are not updated when it happen.
So this logic can detect synthesized argument.

Ah I see, please update that comment to mention that.

xedin · 2020-03-18T23:52:09Z

lib/Sema/CSDiagnostics.cpp

 #include "swift/Parse/Lexer.h"
+#include "swift/Sema/IDETypeChecking.h"


Let's avoid using functions from swift/Sema/IDETypeChecking.h and simplify record more information into the fix itself.

OK, I will fix it.

It is included to use getOriginalArgumentList helper function at SingleLabelingFailure::diagnoseAsError.
It integrates ParenExpr, TupleExpr, and other cases.

I don't know well what AST may comes here.
I will be looking into it.

I'd suggest to avoid dealing with AST at all and record all necessary information in the fix/diagnostic. We are hoping that we'd be able to model argument in calls differently in the future so adjusting diagnostics proactively means less refactoring in the future.

Oh I never thought about not touching AST.
OK!
Thank for telling me future direction.

xedin · 2020-03-18T23:55:36Z

lib/Sema/CSDiagnostics.cpp

+  paramStr += paramLabel.str();
+  paramStr += ':';
+
+  if (auto selectedOverload = getChoiceFor(getLocator())) {


Once locator points to an individual argument you can use getCalleeLocator here to refer to the overload choice.

Thanks.
Do you tell me code after I will change locator of RelabelSingleArgument following above review?

Yes, I'm just pointing out how to fix a future problem once locator would be changed :)

omochi · 2020-03-21T13:05:01Z

My work will finish soon.
I wil report detail when finish.

omochi · 2020-03-21T17:04:34Z

@xedin I updated implementation.
Most of problems you pointed out are resolved.
Please review again.

I explain details below.

I added diagnostics aggregation in two cases.

All labeling issues have same category (wrong/missing/extra) and no out of order.
In this case, aggregated diagnostics looks clear.
Labels which is contained by reordering fix has labeling issue.
In this case, reordering fix has label string which isn't seen in correct parameters.
So it is difficult to diagnose what happen.
This is fallback mode.

Aggregated labeling constructs another mapping between arguments and parameters.

https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L716-L760

It is different from mapping for type checking.
It has own impact score for labeling, and same impact score for reordering with individual case.
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L1018

I removed RelabelSingleArgument.
Instead, RelabelArguments supports single argument.
If one argument specified, it points column of label for diagnostics.
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSDiagnostics.cpp#L760-L771

I added ArgumentRelabeling as parameter of RelabelArguments.
It contains what need for labeling diagnostics.
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSFix.h#L350-L358
After that, LabelingFailure use AST only few.

But constructing RelabelArguments are complex.

https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L885-L891
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L980-L986

I didn't understand how to use getCalleeLocator what you told.
getChoiceFor which calls getCalleeLocator inside it looks enough.
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSDiagnostics.cpp#L810

There is another use of RelabelArguments.
I had trouble rewriting here.

https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L1986-L1999

I found only two source code which is processed here from test cases.

// (1)
struct S {}
let _: (S) -> S.Type = type(of:)

// (2)
let _: ((Int) -> Int, (@escaping (Int) -> Int) -> ()) -> ()
  = withoutActuallyEscaping(_:do:)

But both cases didn't work correctly.
They have locator [DeclRef -> contextual type -> function argument].
LabelingFailure::getArgumentListExprFor failed with it and didn't diagnose.
For the moment, I build empty relabeling to keep constraint solver same behavior.

I added LocatorPathElt::ArgumentLabel.
I stopped using coalesceAndDiagnose.
Using it here.
https://github.com/omochi/swift/blob/25973233491269126eacd2333c80a7cc57bc9421/lib/Sema/CSSimplify.cpp#L898

omochi · 2020-03-23T07:43:09Z

I pushed one commit to fix bug.

Original

func f(aa: Int, bb: Int, cc: () -> Void) {}
f(aax: 1, bbx: 1) {}
// incorrect argument labels in call (have 'aax:bbx:_:', expected 'aa:bb:cc:')

Fixed

func f(aa: Int, bb: Int, cc: () -> Void) {}
f(aax: 1, bbx: 1) {}
// incorrect argument labels in call (have 'aax:bbx:', expected 'aa:bb:')

xedin · 2020-03-24T23:22:32Z

lib/Sema/CSDiagnostics.h

@@ -465,15 +465,20 @@ class SuperclassRequirementFailure final : public RequirementFailure {
 /// Call to `foo` is going to be diagnosed as missing `q:`
 /// and having extraneous `a:` labels, with appropriate fix-its added.
 class LabelingFailure final : public FailureDiagnostic {
-  ArrayRef<Identifier> CorrectLabels;
+  ArgumentRelabeling Relabeling;


I went through this a couple of times and I can say that I like previous approach better. I think we should record individual RelabelArgument fixes for each incorrect position so that content of ArgumentRelabelingItem could be inlined into the fix itself and then have a RelabelArgument::coaleseAndDiagnose method which would group labeling fixes if necessary e.g., if majority of labels are incorrect, and produce a single diagnostic. WDYT?

I thought about that today.

Basically, I think it's good.

Certainly, it makes sense to generate fixes for each label that has an problem, and then aggregate them at the time of the coalesceAndDaignose where the fixes are translated into diagnostics.

As a constraint system, it is easier to understand the presence of fixes for each label when looking at the intermediate state with -debug-constraints, and the impact value of each fix is constant.

The implementation of matchCallArguments is also simplified around the following.

https://github.com/apple/swift/blob/e26763d56be595f2a19bf9022e890696def118f8/lib/Sema/CSSimplify.cpp#L688-L713

However, some changes in diagnostics are inevitable because other information about reordering fix, extraneous or missing arguments are not detectable in RelabelArgument::coalesceAndDiagnose.

(1). It can't control not aggregating label errors when there is a reorder fix.

For example, it would be:

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {} f(aax: 0, bb: 1, dd: 3, cc: 2, eex: 5) // incorrect argument labels in call (have 'aax:eex:', expected 'aa:ee:') // fix-its: aax => aa, eex => ee // argument 'cc:' must precede argument 'dd:'

but in my current implementation:

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {} f(aax: 0, bb: 1, dd: 3, cc: 2, eex: 5) // incorrect argument label in call (have 'aax:', expected 'aa:') // fix-its: aax => aa // incorrect argument label in call (have 'eex:', expected 'ee:') // fix-its: eex => ee // argument 'cc:' must precede argument 'dd:'

(2). It shows the given label and the expected label when it produces an aggregated label error. At this time, the given and expected labels cannot be reconstructed because the information of the extra argument and missing argument is not passed to the RelabelArgument::coalesceAndDiagnose. These are the cases represented as None in the ArgumentRelabelingItem.

For a given label, It can get the AST information from the Locator. So the extra argument is fine. To begin with, currently it doesn't diagnose label issues when there is an extra argument, but I would like to do this at the same time in the future.

However, the label of the parameter corresponding to the missing argument can't be helped.

For example, it would be:

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {} f(aa: 0, cc: 2, ddx: 3, eex: 3) // incorrect argument labels in call (have 'ddx:eex:', expected 'dd:ee:') // fix-its: ddx => dd, eex => ee // missing argument for parameter 'bb' in call

but in my current implementation:

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {} f(aa: 0, cc: 2, ddx: 3, eex: 3) // incorrect argument labels in call (have 'aa:cc:ddx:eex:', expected 'aa:bb:cc:dd:ee:') // fix-its: ddx => dd, eex => ee // missing argument for parameter 'bb' in call

I can accept both cases.
It's rather simple and may be better.

With this policy, I can drop the Optional in the ArgumentRelabelingItem.

I will try to implement it.

I think we can do it this way - aggregate only if there are more than two labeling failures and no other failures associated with arguments of the same call?

Also please note that "aggregate" diagnostic has to produce all of the labels because it's about what call as a whole expects.

If I let RelabelArgument have only contents of the ArgumentRelabelingItem as you said at start of this review thread, it can't produce an aggregated diagnosis that includes all the labels because RelabelArgument fix is not generated for the missing argument, extra argument or label where there is no problem.

I considered that and made one previous reply.

Am I misreading something?

It would be easy enough to retrieve parameter labels from an overload associated with the call via getOverloadChoice(getCalleeLocator(getLocator())) (from FailureDiagnostic), maybe worth a separate fix/diagnostic even.

I got it! Thanks for the advice.

omochi · 2020-03-27T12:10:36Z

I updated implementation based on @xedin's idea.
#30444 (comment)

Behavior of diagnostics is almost same with my previous version.

Expected labels are reconstructed from OverloadChoice.
#30444 (comment)

(1) But that logic looks too complex.
3c03c44#diff-34de70db160d5d09726050ab292a15acR832-R899

(2) LabelingFailure and RelabelArgument needs Bindings.
3c03c44#diff-94de6f31b3e55bfbbab78d47e14760e0R487

They are needed to support these cases.
3c03c44#diff-fbf713f8bd7baed28b31eabcd6837d65R1432-R1434

it skips variadic tails in argument labels
it skips optional parameter labels which has default or is variadic and without mapped argument.
it skips argument and parameter labels for trailing closure.

(3) There is an another critical problem.
With #colorLiteral(...), getOverloadChoiceIfAvailable returns null.
3c03c44#diff-34de70db160d5d09726050ab292a15acR812
So it can't reconstruct parameter labels with this.

#colorLiteral(red: 1, bleen: 0, grue: 0, alpha: 1)

Test case is this.
https://github.com/apple/swift/blob/3c03c4404f5ebf8fef645af637fafa0599edf825/test/Sema/object_literals_osx.swift#L10
This is only one test failure of this commit.

Should I try to fix getOverload ?
But #colorLiteral is compiler directive.
Does it have any Decl object?

Or should I use another approach to reconstruct?

xedin · 2020-03-30T22:14:47Z

@omochi Sorry I didn't have much time to look into the changes again but I can tell you now regarding (3) - I think that's a bug in getCalleeLocator which should be easily fixable.

omochi · 2020-03-31T12:19:09Z

I will investigate getCalleeLocator for #colorLiteral.

I'm fine about the delay. This project is more complicated than I thought at start time. I'm happy to progress it a little at a time.

omochi · 2020-04-01T09:53:29Z

I updated implementation to support ObjectLiteralExpr.
https://github.com/apple/swift/blob/158d14fd13d8bba494b4056bd3cd3b75b5a2c46d/lib/Sema/CSDiagnostics.cpp#L835-L843

CSGen for ObjectLiteralExpr calls matchCallArgument directly and does not call resolveOverload.

https://github.com/apple/swift/blob/158d14fd13d8bba494b4056bd3cd3b75b5a2c46d/lib/Sema/CSGen.cpp#L1357

After solver has determined the type corresponding to _ExpressibleByColorLiteral, the actual call of init is generated at CSApply.

https://github.com/apple/swift/blob/158d14fd13d8bba494b4056bd3cd3b75b5a2c46d/lib/Sema/CSApply.cpp#L2629-L2634

getOverloadChoiceIfAvailable cannot retrieve SelectedOverload because nothing is recorded in the Solution.ResolvedOverloads.

https://github.com/apple/swift/blob/158d14fd13d8bba494b4056bd3cd3b75b5a2c46d/lib/Sema/ConstraintSystem.h#L964-L970

This contains special handling such as rewriting labels from _colorLiteralRed: to red:.

https://github.com/apple/swift/blob/158d14fd13d8bba494b4056bd3cd3b75b5a2c46d/lib/Sema/CSDiagnostics.cpp#L945-L959

omochi · 2020-04-01T12:49:31Z

@swift-ci please smoke test

omochi · 2020-04-01T13:02:38Z

At least it's passed the test, but very fragile code.
I will explain details tomorrow.

omochi · 2020-04-02T06:18:13Z

I have a very long post below.
However, this is inevitably to share my thoughts.
Please read it when you get some time....

premise

First of all, here's what I want to do with this patch.

Goal 1

Outputs all multiple label errors individually.

1-1: Reorderings and wrong labels

My patch

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {}
f(bb: 1, aa: 0, ccx: 2, ee: 4, dd: 3)

a.swift:2:10: error: argument 'aa' must precede argument 'bb'
f(bb: 1, aa: 0, ccx: 2, ee: 4, dd: 3)
  ~~~~~~~^~~~~
  aa: 0,  
a.swift:2:17: error: incorrect argument label in call (have 'ccx:', expected 'cc:')
f(bb: 1, aa: 0, ccx: 2, ee: 4, dd: 3)
                ^~~
                cc
a.swift:2:32: error: argument 'dd' must precede argument 'ee'
f(bb: 1, aa: 0, ccx: 2, ee: 4, dd: 3)
                        ~~~~~~~^~~~~
                        dd: 3,

Xcode11.4

a.swift:2:10: error: argument 'aa' must precede argument 'bb'
f(bb: 1, aa: 0, ccx: 2, ee: 4, dd: 3)
  ~~~~~~~^~~~~
  aa: 0,

1-2: wrong, missing and extraneous labels

My patch

func f(aa: Int, bb: Int, _ cc: Int, dd: Int, ee: Int, _ ff: Int) {}
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)

a.swift:2:3: error: incorrect argument label in call (have 'aax:', expected 'aa:')
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
  ^~~
  aa
a.swift:2:11: error: missing argument label 'bb:' in call
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
          ^
          bb: 
a.swift:2:14: error: extraneous argument label 'cc:' in call
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
             ^~~~
             
a.swift:2:21: error: incorrect argument label in call (have 'ddx:', expected 'dd:')
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
                    ^~~
                    dd
a.swift:2:29: error: missing argument label 'ee:' in call
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
                            ^
                            ee: 
a.swift:2:32: error: extraneous argument label 'ff:' in call
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
                               ^~~~

Xcode11.4

a.swift:2:2: error: incorrect argument labels in call (have 'aax:_:cc:ddx:_:ff:', expected 'aa:bb:_:dd:ee:_:')
f(aax: 0, 1, cc: 2, ddx: 3, 4, ff: 5)
 ^~~~        ~~~~   ~~~        ~~~~
  aa      bb:       dd      ee:

Goal 2

In certain cases, I want to produce an aggregated label error.

2-1: If the arguments involved in the reordering message have label problems.

In this case, the reordering message is difficult to understand, so instead of using a normal label mapping, use a positional mapping.

My patch

func f(aa: Int, bb: Int, cc: Int, dd: Int, ee: Int) {}
f(bb: 1, aax: 0, dd: 3, ee: 4, cc: 2)

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:dd:ee:cc:', expected 'aa:bb:cc:dd:ee:')
f(bb: 1, aax: 0, dd: 3, ee: 4, cc: 2)
 ^~~     ~~~     ~~     ~~     ~~
  aa     bb      cc     dd     ee

In this input, arguments aa: and cc: are misplaced.
And bb:, dd:, ee: have no problem.

However, the argument aa: is incorrectly labeled aax:, so it will stop reordering and use a positional mapping.

As a result, bb:, dd:, and ee: are also considered to be label errors.

Xcode 11.4: Same result.

2-1-1: When using positional mappings, maintain the given variadic arguments and omitted defaulted parameters detected at the time of the normal mapping.

My patch

func f(aa: Int, bb: Int, cc: Int, dd: Int..., ee: Int, ff: Int = 0, gg: Int = 0, hh: Int) {}
f(bb: 1, aax: 0, dd: 31, 32, 33, ee: 4, hh: 7, cc: 2)

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:dd:ee:hh:cc:', expected 'aa:bb:cc:dd:ee:hh:')
f(bb: 1, aax: 0, dd: 31, 32, 33, ee: 4, hh: 7, cc: 2)
 ^~~     ~~~     ~~              ~~     ~~     ~~
  aa     bb      cc              dd     ee     hh

Since dd: is a variable arguments, there are no label errors at 32 and 33.
ff: and gg: are omitted, so they don't show up as candidates for fixing the label error.

Xcode 11.4

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:dd:_:_:ee:hh:cc:', expected 'aa:bb:cc:dd:ee:ff:gg:hh:')
f(bb: 1, aax: 0, dd: 31, 32, 33, ee: 4, hh: 7, cc: 2)
 ^~~     ~~~     ~~              ~~     ~~     ~~
  aa     bb      cc      dd: ee: ff     gg     hh

Label insertion appear at 32 and 33.
ff: and gg:, which were omitted, are candidates for fixing.

2-2: If there is more than one label error and only wrongs, all labels will be displayed in the message.

My patch

func f(aa: Int, bb: Int, cc: Int, dd: Int) {}
f(aax: 0, bbx: 1, cc: 2, dd: 3)

a.swift:2:2: error: incorrect argument labels in call (have 'aax:bbx:cc:dd:', expected 'aa:bb:cc:dd:')
f(aax: 0, bbx: 1, cc: 2, dd: 3)
 ^~~~     ~~~
  aa      bb

Xcode11.4: Same result.

2-3: If there is more than one label error and only extraneous, group it.

My patch

func f(_ aa: Int, _ bb: Int, cc: Int, dd: Int) {}
f(aa: 0, bb: 1, cc: 2, dd: 3)

a.swift:2:2: error: extraneous argument labels 'aa:bb:' in call
f(aa: 0, bb: 1, cc: 2, dd: 3)
 ^~~~~   ~~~~

Xcode11.4: Same result.

2-4: If there is more than one label error and only missing, group it.

My patch

func f(aa: Int, bb: Int, cc: Int, dd: Int) {}
f(0, 1, cc: 2, dd: 3)

a.swift:2:2: error: missing argument labels 'aa:bb:' in call
f(0, 1, cc: 2, dd: 3)
 ^
  aa:  bb:

Xcode11.4: Same result.

Goal 3

I want to improve have and expect when making aggregated label errors.

If all labels appear in the message (2-1, 2-2), I want to keep the given variadic arguments and the omitted defaulted parameters.

3-1: Maintaining given variadic arguments

My patch

func f(aa: Int, bb: Int, cc: Int..., dd: Int) {}
f(aax: 0, bbx: 1, cc: 21, 22, 23, dd: 3)

a.swift:2:2: error: incorrect argument labels in call (have 'aax:bbx:cc:dd:', expected 'aa:bb:cc:dd:')
f(aax: 0, bbx: 1, cc: 21, 22, 23, dd: 3)
 ^~~~     ~~~
  aa      bb

Xcode11.4:

a.swift:2:2: error: incorrect argument labels in call (have 'aax:bbx:cc:_:_:dd:', expected 'aa:bb:cc:_:_:dd:')
f(aax: 0, bbx: 1, cc: 21, 22, 23, dd: 3)
 ^~~~     ~~~
  aa      bb

It is supposed to have given 6 arguments, but as a user, cc: has given it as a variadics argument, and the compiler recognizes it as such, so the label in the message should be 4.

It is supposed to expect 6 parameters, but by the definition of f there should be 4.

3-2: Maintaining omitted defaulted parameters

My patch

func f(aa: Int, bb: Int, cc: Int = 0, dd: Int = 0, ee: Int, ff: Int = 0) {}
f(bb: 1, aax: 0, cc: 2, ee: 4)

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:cc:ee:', expected 'aa:bb:cc:ee:')
f(bb: 1, aax: 0, cc: 2, ee: 4)
 ^~~     ~~~
  aa     bb

It's a positional mapping of (2-1) because aax: is misplaced.
Parameters cc:, dd:, and ff: all have defaults, but dd: and ff:, for which no arguments are given, are not shown in have and expect.

Xcode11.4:

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:cc:ee:', expected 'aa:bb:cc:dd:ee:ff:')
f(bb: 1, aax: 0, cc: 2, ee: 4)
 ^~~     ~~~            ~~
  aa     bb             dd

dd: and ff: are expected, which should have been omitted.

3-3: Ignoring label of trailing closure

My patch

func f(aa: Int, bb: Int, cc: Int, dd: () -> Void) {}
f(bb: 1, aax: 0, cc: 2) {}

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:cc:', expected 'aa:bb:cc:')
f(bb: 1, aax: 0, cc: 2) {}
 ^~~     ~~~
  aa     bb

Xcode11.4:

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:cc:_:', expected 'aa:bb:cc:dd:')
f(bb: 1, aax: 0, cc: 2) {}
 ^~~     ~~~
  aa     bb

Trailing closure is supposed to be given as an empty label.
A label dd: is expected.

Parameter and argument that become trailing closure should be excluded from the labels.

Implementation problem

Among the implementations of the various behaviors described above, the code for the realization of goal 3 in the case of the positional mapping in (2-1) is unstable.

First, in order to achieve goal 3, LabelingFailure that generates the error message must have the following information.

4-1. which argument is variadic
4-2. which argument is trailing closure
4-3. whether a varidic parameter is omitted or not
4-4. whether a parameter with a default is omitted or not
4-5. Which parameter received trailing closure

These information can be retrieved from ArgumentFailureTracker::Bindings and AST. So I made LabelingFailure have Bindings and pass Bindings of ArgumentFailureTracker.

In the case of 2-2, the current code is well written.
However, in the case of 2-1, it works correctly but is unstable.

First, the Bindings of the ArgumentFailureTracker is a reference to the parameterBindings of the matchCallArgument, so it has the normal mapping information.

What's unstable is that it's using a different positional mapping in the case of (2-1), but it's passing Bindings based on normal mapping.

An example of a specific value is shown below.

func f(aa: Int, bb: Int, cc: Int, dd: Int..., ee: Int, ff: Int = 0, gg: Int = 0, hh: Int) {}
f(bb: 1, aax: 0, dd: 31, 32, 33, ee: 4, hh: 7, cc: 2)

In this case, as a result of the normal mapping process, Bindings will look like this

5-1
Bindings = [
  aa => (1) aax: 0,
  bb => (0) bb: 1,
  cc => (7) cc: 2,
  dd => (2) dd: 31, (3) _: 32, (4) _: 33,
  ee => (5) ee: 4,
  ff => ,
  gg => ,
  hh => (6) hh: 7
]

However, since positional mapping is used, the diagnostic results are as follows.

a.swift:2:2: error: incorrect argument labels in call (have 'bb:aax:dd:ee:hh:cc:', expected 'aa:bb:cc:dd:ee:hh:')
f(bb: 1, aax: 0, dd: 31, 32, 33, ee: 4, hh: 7, cc: 2)
 ^~~     ~~~     ~~              ~~     ~~     ~~
  aa     bb      cc              dd     ee     hh

The Bindings that represents this state are follows.

5-2
Bindings = [
  aa => (0) bb: 1,
  bb => (1) aax: 0,
  cc => (2) dd: 31, (3) _: 32, (4) _: 33,
  dd => (5) ee: 4,
  ee => (6) hh: 7,
  ff => ,
  gg => ,
  hh => (7) cc: 2
]

Thus, in a situation where 5-2 is expected, 5-1 has been passed.
This is why I think it's unstable.

However, this is working correctly.
This is because 5-1 and 5-2 have the same information about 4-1, 4-2, 4-3, 4-4, and 4-5.
And the reason this has the same information is because I aimed for 2-1-1.

In other words, the 2-1-1 and 4-X designs mesh together in a daungerous balance.

This is difficult to fix because ArgumentFailureTracker::Bindings is the reference.
In order to rewrite ArgumentFailureTracker::Bindings to pass 5-2 to the LabelingFailure, the parameterBindings of the matchCallArgument must also be rewritten.
This can result in unrelated side effects.

There is also no API for passing new Bindings to the MatchCallArgumentListener.

Additional proposal

Writing this far has given me an idea to solve this.

First, I talk about something else.

6. Problem of contradictory messages

If a positional mapping is used in the diagnosis of a label, it has a different mapping than parameterBindings.
Because parameterBindings is used for the subsequent type checking, type errors may not correspond to label errors.

My patch

func f(aa: Int, bb: Int, cc: String, dd: Int, ee: Int) {}
f(bb: 1, aax: 0, dd: "3", ee: 4, cc: 2)

a.swift:3:2: error: incorrect argument labels in call (have 'bb:aax:dd:ee:cc:', expected 'aa:bb:cc:dd:ee:')
f(bb: 1, aax: 0, dd: "3", ee: 4, cc: 2)
 ^~~     ~~~     ~~       ~~     ~~
  aa     bb      cc       dd     ee
a.swift:3:22: error: cannot convert value of type 'String' to expected argument type 'Int'
f(bb: 1, aax: 0, dd: "3", ee: 4, cc: 2)
                     ^
a.swift:3:38: error: cannot convert value of type 'Int' to expected argument type 'String'
f(bb: 1, aax: 0, dd: "3", ee: 4, cc: 2)
                                     ^

Xcode11.4: Same result.

An error in the label asks that the third argument dd: "3" be corrected to cc: "3".
That is, the compiler assumes that "3" is to be passed to cc:.
But on the other hand, it is also said that this "3" is a String and does not match the expected Int.
These two messages are contradictory.
If it expect "3" to be cc:, then the type we expect for "3" should be Int of cc:.

The same contradiction arises in the argument cc.
It's asking for cc: 2 to be modified to ee: 2, so the compiler should expect user to pass 2 to the parameter ee.
Nevertheless, it tells that the type of 2 is Int and does not match the expected String.

Solution

The problem of instability in 5-2 and the problem of contradictory messages in 6 can be solved collectively.
When a 2-1 positional mapping is used, all it has to do is rewrite the parameterBindings of the matchCallArgument to the 5-2 form.

That way, the 5-2 problem and the 6 problem will be solved at the same time.

Semantically, I think it makes sense for the code to do a type check in the state after the label is modified as a diagnostic behavior.

Originally, this patch wasn't going to touch on the 6 issue.
It's hard to review because it has a larger area of influence.

However, I thought it would be a better design to include this.
How do you think?

xedin · 2020-04-09T00:00:59Z

First of all, thank you for working on this @omochi! I think this is great! Couple of notes are below:

Goal 2

In my opinion diagnostics should be producing fix-it(s) only in cases 2-3, and 2-4. Unless types of argument/parameter pair are taking into consideration it's impossible to say what is going on with call in presence of multiple different kinds of problems with the call e.g. out-of-order arguments, missing labels etc., so it might make sense to produce a simple aggregate error and let users "disambiguate" the call.

Goal 3

I think we should always display labels in a way which is consistent with parameters e.g. if there are multiple variadic arguments drop all but one, if there is a trailing closure - pretend that argument is labeled.

Problem of contradictory messages

It seems like we should be able to fix this problem in a different way. In case of incorrect or extraneous label(s) it might make sense to ignore argument type completely because such labels could be considered an indication that user intended to call a different overload of a given name.

It seems relatively easy to fix with your changes if all of the labeling fixes were recorded individually, that way we could use hasFixFor in repairFailures when locator ends at ApplyArgToParam element and avoid recording fixes for arguments which already have a labeling conflict.

omochi · 2020-04-09T13:19:28Z

Thank you for reading my long message and giving next advice to me.
I will update implementation with following your comment.

omochi · 2020-04-10T13:49:03Z

@xedin I updated implementation.

However, I have not updated test cases yet.
Revising a test case is a lot of work, so I'd like to get your review before I get started.

New implementation:

(1)
has simple logic for emit label errors.
https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSSimplify.cpp#L646-L691

(2)
locate label fixes at CallExpr -> ApplyArgument -> ArgumentLabel(argIdx).

https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSSimplify.cpp#L794-L798
https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSSimplify.cpp#L825-L829

(3)
collect all label errors (RelabelArgument and MoveOutOfOrderArgument) and determine what diagnostics emit.

https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSFix.cpp#L313-L320
https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSFix.cpp#L829-L836

(4)
diagnose simple aggregated error if complex case.

https://github.com/apple/swift/blob/feb51bd6a89e85ddd0c93087502c94027fe10116/lib/Sema/CSDiagnostics.cpp#L931-L932

Diagnostics for various inputs:

struct TestBasic {
  func f(_ aa: Int, _ bb: Int, cc: Int, dd: Int, ee: Int, ff: Int) {}

  func test() {
    // 1 wrong
    f(0, 1, ccx: 2, dd: 3, ee: 4, ff: 5)
/*
a.swift:6:13: error: incorrect argument label in call (have 'ccx:', expected 'cc:')
    f(0, 1, ccx: 2, dd: 3, ee: 4, ff: 5)
            ^~~
            cc
*/

    // 1 missing
    f(0, 1, 2, dd: 3, ee: 4, ff: 5)
/*
a.swift:9:13: error: missing argument label 'cc:' in call
    f(0, 1, 2, dd: 3, ee: 4, ff: 5)
            ^
            cc: 
*/

    // 1 extra
    f(aa: 0, 1, cc: 2, dd: 3, ee: 4, ff: 5)
/*
a.swift:12:7: error: extraneous argument label 'aa:' in call
    f(aa: 0, 1, cc: 2, dd: 3, ee: 4, ff: 5)
      ^~~~
      
*/

    // 1 ooo
    f(0, 1, dd: 3, cc: 2, ee: 4, ff: 5)
/*
a.swift:15:20: error: argument 'cc' must precede argument 'dd'
    f(0, 1, dd: 3, cc: 2, ee: 4, ff: 5)
            ~~~~~~~^~~~~
            cc: 2,  
*/

    // 2 wrong
    f(0, 1, ccx: 2, ddx: 3, ee: 4, ff: 5)
/*
a.swift:18:6: error: too many label errors in call (have: '_:_:ccx:ddx:ee:ff:', expected: '_:_:cc:dd:ee:ff:')
    f(0, 1, ccx: 2, ddx: 3, ee: 4, ff: 5)
     ^
*/

    // 2 missing
    f(0, 1, 2, 3, ee: 4, ff: 5)
/*
a.swift:21:6: error: missing argument labels 'cc:dd:' in call
    f(0, 1, 2, 3, ee: 4, ff: 5)
     ^
            cc:  dd: 
*/

    // 2 extra
    f(aa: 0, bb: 1, cc: 2, dd: 3, ee: 4, ff: 5)
/*
a.swift:24:6: error: extraneous argument labels 'aa:bb:' in call
    f(aa: 0, bb: 1, cc: 2, dd: 3, ee: 4, ff: 5)
     ^~~~~   ~~~~
             
*/

    // 2 ooo
    f(0, 1, dd: 3, cc: 2, ff: 5, ee: 4)
/*
a.swift:27:20: error: argument 'cc' must precede argument 'dd'
    f(0, 1, dd: 3, cc: 2, ff: 5, ee: 4)
            ~~~~~~~^~~~~
            cc: 2,  
a.swift:27:34: error: argument 'ee' must precede argument 'ff'
    f(0, 1, dd: 3, cc: 2, ff: 5, ee: 4)
                          ~~~~~~~^~~~~
                          ee: 4,  
*/

    // 1 wrong + 1 missing
    f(0, 1, ccx: 2, 3, ee: 4, ff: 5)
/*
a.swift:30:6: error: too many label errors in call (have: '_:_:ccx:_:ee:ff:', expected: '_:_:cc:dd:ee:ff:')
    f(0, 1, ccx: 2, 3, ee: 4, ff: 5)
     ^
*/

    // 1 wrong + 1 extra
    f(aa: 0, 1, ccx: 2, dd: 3, ee: 4, ff: 5)
/*
a.swift:33:6: error: too many label errors in call (have: 'aa:_:ccx:dd:ee:ff:', expected: '_:_:cc:dd:ee:ff:')
    f(aa: 0, 1, ccx: 2, dd: 3, ee: 4, ff: 5)
     ^
*/

    // 1 wrong + 1 ooo
    f(0, 1, ccx: 2, dd: 3, ff: 5, ee: 4)
/*
a.swift:36:6: error: too many label errors in call (have: '_:_:ccx:dd:ff:ee:', expected: '_:_:cc:dd:ee:ff:')
    f(0, 1, ccx: 2, dd: 3, ff: 5, ee: 4)
     ^
*/
  }
}

struct TestTrailingClosure {
  func f1(aa: Int, bb: Int, cc: () -> Void = {}) {}

  func f2(aa: Int, bb: Int, _ cc: () -> Void = {}) {}

  func test() {
    f1(aax: 0, bbx: 1) {}
/*
a.swift:46:7: error: too many label errors in call (have: 'aax:bbx:cc:', expected: 'aa:bb:cc:')
    f1(aax: 0, bbx: 1) {}
      ^
*/

    f2(aax: 0, bbx: 1) {}
/*
a.swift:48:7: error: too many label errors in call (have: 'aax:bbx:_:', expected: 'aa:bb:_:')
    f2(aax: 0, bbx: 1) {}
      ^
*/

    f1(aax: 0, bbx: 1)
/*
a.swift:50:7: error: too many label errors in call (have: 'aax:bbx:', expected: 'aa:bb:')
    f1(aax: 0, bbx: 1)
      ^
*/

    f2(aax: 0, bbx: 1)
/*
a.swift:52:7: error: too many label errors in call (have: 'aax:bbx:', expected: 'aa:bb:')
    f2(aax: 0, bbx: 1)
      ^
*/
  }
}

struct TestVariadic {
  func f(aa: Int, bb: Int, cc: Int...) {}

  func test() {
    f(aax: 0, bbx: 1, cc: 2, 3, 4)
/*
a.swift:60:6: error: too many label errors in call (have: 'aax:bbx:cc:', expected: 'aa:bb:cc:')
    f(aax: 0, bbx: 1, cc: 2, 3, 4)
     ^
*/

    f(aax: 0, bbx: 1)
/*
a.swift:62:6: error: too many label errors in call (have: 'aax:bbx:', expected: 'aa:bb:')
    f(aax: 0, bbx: 1)
     ^
*/
  }
}

struct TestDefaulted {
  func f(aa: Int, bb: Int, cc: Int = 0) {}

  func test() {
    f(aax: 0, bbx: 1, cc: 2)
/*
a.swift:70:6: error: too many label errors in call (have: 'aax:bbx:cc:', expected: 'aa:bb:cc:')
    f(aax: 0, bbx: 1, cc: 2)
     ^
*/

    f(aax: 0, bbx: 1)
/*
a.swift:72:6: error: too many label errors in call (have: 'aax:bbx:', expected: 'aa:bb:')
    f(aax: 0, bbx: 1)
     ^
*/
  }
}

too many label errors in call is new message.
It has have: and expected:.
It doesn't have fix-its.

Am I understanding your opinion correctly?

omochi · 2020-04-11T18:49:13Z

@swift-ci Please smoke test

omochi · 2020-04-11T19:23:59Z

@xedin I updated implementation.
I think we could merge my patch soon.
Please review again!

Details are below.

Basic architecture is here.
#30444 (comment)

And I added new idea.
This is adjustment of unlabeled parameter binding position.
Code is here.
https://github.com/apple/swift/blob/74b2d33d5bbb1e462e89f84cd99fbc7a8e6330bd/lib/Sema/CSSimplify.cpp#L276-L315

This is an alternative for positional mapping.

Positional mapping in master is here.

swift/lib/Sema/CSSimplify.cpp

Lines 758 to 763 in b7654f1

    
           SmallVector<Identifier, 8> expectedLabels; 
        
           llvm::transform(params, std::back_inserter(expectedLabels), 
        
                           [](const AnyFunctionType::Param &param) { 
        
                             return param.getLabel(); 
        
                           }); 
        
           return listener.relabelArguments(expectedLabels);

My old implementation is here.
https://github.com/apple/swift/blob/1836317faeb731922fc620e8e1ba8d09eca23d8f/lib/Sema/CSSimplify.cpp#L708-L738

But they have problem what I wrote as 6. Problem of contradictory messages before.

Without positional mapping, following case was hard.

func f(aa: Int, _ bb: Int) {}
f(0, 1)

matchCallArguments does:
1-1. fail to bind aa: ordinarily.
1-2. bind params[1] to args[0] ordinarily.
1-3. bind aa: to args[1] semi positionally.

As a result, mapping is 0=>1, 1=>0.
So it makes one missing label and one out of order.
But we want 0=>0, 1=>1 and one missing label.

So I added new logic into 1-2.

When claim argument for params[1],
it looks that aa: is still unbound and bind to args[1]
so that save args[0] which params[0] will bind to later.

I checked that it works good by test case.
https://github.com/apple/swift/blob/fd44af86cc150f7d6cdbf3e105420c6a293719c3/test/Constraints/diagnostics.swift#L1514-L1617

I made test case for what I summarized here.
#30444 (comment)

https://github.com/apple/swift/blob/74b2d33d5bbb1e462e89f84cd99fbc7a8e6330bd/test/Constraints/diagnostics.swift#L1424-L1491

Now I resolved two worries written here.
#30444 (comment)
Because there is only one mapping now
and it prints no fix-its for aggregated diagnostics.

Other diagnostics behavior seems following your advices given me so far.

omochi · 2020-04-11T22:30:40Z

@swift-ci Please smoke test

xedin · 2020-04-12T00:32:18Z

Thank you, @omochi! I'll try to take a look on monday.

omochi · 2020-05-03T13:54:10Z

Sorry, my day job is getting very busy so I'll check this weekend or next weekend.

xedin · 2020-05-03T21:27:35Z

No worries, take your time!

omochi · 2020-05-16T12:53:41Z

My main business has come out of a hard situation.
It's about time I could resume my activities here.

First, I will rebase it on master.

I'm interested in splitting this work up and making it partly PR.
I will see if there's anything I can do so.

However,
changing the RelabelArguments to multiple RelabelArguments,
new Locator (LocatorPathElt::ArgumentLabel),
the logic for aggregating labeling errors are connected complexly.
So it would be difficult.

omochi · 2020-05-18T16:09:38Z

@swift-ci Please smoke test

omochi · 2020-05-19T01:19:38Z

@swift-ci Please Smoke Test

omochi · 2020-05-19T18:08:23Z

@xedin
I updated implementation.

Rebased on master.
Remove meaningless changes in tests.
Remove subjective too many label errors and use existing wrong_argument_labels.
Provide RelabelArgument fixes to LabelingFailure. It reduces logic in diagnoseLabelError which looks like logic in LablingFailure::diagnose.

By the way, since many changes have piled up now, it's getting hard to rebase to master.
So I'm going to try to reassemble commits.
After that, I will consider to break them into a few independent PRs.

omochi · 2020-05-20T15:46:51Z

@xedin
I reassembled commits.
Patch contents has not changed.
These commits are separated by small semantic part of patch.

It helps reviewer and future reader of these commits.

And, I plan to make another PR which contains only addition of test cases.

Because my work particularly focus corner cases,
changes of result for test input is important.

But I add some new test inputs in this patch.
Since they are new,
we can't easily check changes of result for these inputs
without seeing current result.

So I want to insert these test cases on master
before this PR will merge.

omochi · 2020-05-20T15:57:14Z

@swift-ci Please smoke test

callee overload which has label issue

… ... which represents just a one label error

fixes. New dianostic building logic considers pairs of argument and parameter well in complex situation.

from them.

`RelabelArgument` for each label issues.

issues in `matchCallArguments`. It finds all of out of order label matchings. It reports label issues individually.

by considering unbounded parameter which is at left of current parameter in `matchCallArgument`. It avoids matching which has unwanted crossing pairs between argument and parameter. It provides natural relabeling diagnostics without reconstructing aligned matching pairs when out of ordered mapping happens like old implementation.

omochi · 2020-05-24T07:47:27Z

@swift-ci please smoke test

omochi · 2020-05-27T10:55:11Z

I will split this patch and create other patch.
I make it draft until they are passed.

shahmishal · 2020-10-01T06:55:38Z

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

How to change the base branch: (Link)
More detail about the branch update: (Link)

theblixguy requested a review from xedin March 17, 2020 15:07

xedin requested changes Mar 19, 2020

View reviewed changes

omochi force-pushed the diagnose-label-ind branch from 472f096 to b0e0da8 Compare March 21, 2020 13:03

omochi force-pushed the diagnose-label-ind branch 2 times, most recently from 19f7a11 to 2597323 Compare March 21, 2020 16:19

omochi requested a review from xedin March 21, 2020 17:05

xedin reviewed Mar 24, 2020

View reviewed changes

omochi force-pushed the diagnose-label-ind branch 2 times, most recently from 6b3b9be to 3c03c44 Compare March 27, 2020 11:50

omochi force-pushed the diagnose-label-ind branch from 3c03c44 to 158d14f Compare April 1, 2020 09:37

omochi mentioned this pull request Apr 4, 2020

[Diagnostics] Improve {{none}} fix-it verifier #30791

Merged

omochi force-pushed the diagnose-label-ind branch 2 times, most recently from b9be64e to feb51bd Compare April 10, 2020 13:31

omochi force-pushed the diagnose-label-ind branch from 7f4ae69 to a14849f Compare May 18, 2020 15:17

omochi force-pushed the diagnose-label-ind branch from d9deb3f to c3c2bc3 Compare May 20, 2020 15:15

omochi mentioned this pull request May 21, 2020

[ConstraintSystem] [NFC] add tests for label matching diagnostics #31934

Merged

omochi added 9 commits May 24, 2020 15:32

[ConstraintSystem] add new locator path element ArgumentLabel

547dec5

[ConstraintSystem] add diagnostic note message for candidates of ...

8c762ac

callee overload which has label issue

[ConstraintSystem] change RelabelArguments fix to RelabelArgument…

3f6119d

… ... which represents just a one label error

[ConstraintSystem] change LabelingFailure to use RelabelArgument ...

d106472

fixes. New dianostic building logic considers pairs of argument and parameter well in complex situation.

[ConstraintSystem] gather label fixes and build LabelingFailure ...

8daace2

from them.

[ConstraintSystem] change MatchCallArgumentListener to build ...

92a0580

`RelabelArgument` for each label issues.

[ConstraintSystem] implement new logic of reporting label ...

8e6e963

issues in `matchCallArguments`. It finds all of out of order label matchings. It reports label issues individually.

[ConstraintSystem] remove actualArgNames in matchCallArguments

37cc05f

omochi force-pushed the diagnose-label-ind branch from c3c2bc3 to 9f2c842 Compare May 24, 2020 07:23

[ConstraintSystem][NFC] update tests for new individual labeling fixes

a9be9cf

omochi force-pushed the diagnose-label-ind branch from 9f2c842 to a9be9cf Compare May 24, 2020 07:32

omochi marked this pull request as draft May 27, 2020 10:53

This was referenced May 27, 2020

[ConstraintSystem] adjust matching position of unlabeled parameter #32037

Closed

[ConstraintSystem] new argument matching algorithm #32082

Closed

shahmishal closed this Oct 5, 2020

	if (llvm::any_of(getFixes(), [&](const ConstraintFix *fix) {
	auto *locator = fix->getLocator();
	// Since arguments to @dynamicCallable form either an array
	// or a dictionary and all have to match the same element type,
	// let's allow multiple invalid arguments.
	if (locator->findFirst<LocatorPathElt::DynamicCallable>())
	return false;

	return locator->findLast<LocatorPathElt::ApplyArgToParam>()
	? locator->getAnchor() == anchor
	: false;
	}))

	if (auto newArgIdx = listener.missingArgument(paramIdx)) {
	parameterBindings[paramIdx].push_back(*newArgIdx);
	continue;
	}

		#include "swift/Parse/Lexer.h"
		#include "swift/Sema/IDETypeChecking.h"

[ConstraintSystem] Diagnose label issues individually #30444

[ConstraintSystem] Diagnose label issues individually #30444

Conversation

omochi commented Mar 17, 2020 • edited

Summary

Change Log

Discussion

Design

About out of order

Examples

Concern about impact value

xedin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi Mar 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi commented Mar 21, 2020

omochi commented Mar 21, 2020 • edited

omochi commented Mar 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omochi commented Mar 27, 2020 • edited

xedin commented Mar 30, 2020

omochi commented Mar 31, 2020

omochi commented Apr 1, 2020

omochi commented Apr 1, 2020

omochi commented Apr 1, 2020

omochi commented Apr 2, 2020

premise

Goal 1

Goal 2

Goal 3

Implementation problem

Additional proposal

6. Problem of contradictory messages

Solution

xedin commented Apr 9, 2020

omochi commented Apr 9, 2020

omochi commented Apr 10, 2020

omochi commented Apr 11, 2020

omochi commented Apr 11, 2020 • edited

omochi commented Apr 11, 2020

xedin commented Apr 12, 2020

omochi commented May 3, 2020

xedin commented May 3, 2020

omochi commented May 16, 2020

omochi commented May 18, 2020

omochi commented May 19, 2020

omochi commented Mar 17, 2020 •

edited

omochi Mar 19, 2020 •

edited

omochi commented Mar 21, 2020 •

edited

omochi commented Mar 27, 2020 •

edited

omochi commented Apr 11, 2020 •

edited

omochi commented May 20, 2020 •

edited