[ConstraintSystem] adjust matching position of unlabeled parameter #32037

omochi · 2020-05-27T12:16:12Z

Summary

This patch adjust matching position of unlabeled parameter
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 more natural label error diagnostics.

Detail

This patch changes parameterBindings built in matchCallArgument.

Following is dumped value of parameterBindings.
Please also check change of diagnostics in diff of this patch.
It provides more natural result.

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

  f(0, 1)
  /*
   current:
     param[0]=1
     param[1]=0

   patch:
     param[0]=0
     param[1]=1
   */

  f(0, xx: 1)
  /*
   current:
     param[0]=2
     param[1]=0

   patch:
     param[0]=0
     param[1]=1
   */

  f(xx: 0, 1)
  /*
   current:
     param[0]=2
     param[1]=0

   patch:
     param[0]=0
     param[1]=1
   */

  f(xx: 91, 1, 92)
  /*
   current:
     param[0]=3
     param[1]=0

   patch:
     param[0]=3
     param[1]=1
   */
}

do {
  func f(_ aa: Int, bb: Int, _ cc: Int) { }

  f(bb: 1, 0, 2)
  /*
   current:
     param[0]=2
     param[1]=0
     param[2]=1

   patch:
     param[0]=1
     param[1]=0
     param[2]=2
   */
}

do {
  func f(aa: Int, _ bb: Int, _ cc: Int) {}

  f(0, 1)
  /*
   current:
     param[0]=2
     param[1]=0
     param[2]=1

   patch:
     param[0]=0
     param[1]=1
     param[2]=2
   */
}

do {
  func f(aa: Int, _ bb: Int = 81, _ cc: Int) {}

  f(0, 1)
  /*
   current:
     param[0]=2
     param[1]=0
     param[2]=1

   patch:
     param[0]=0
     param[1]=1
     param[2]=2
   */
}

Future plan and relation of this patch

This patch is split from my another project.
#30444

I have plan to make next patch that change LabelingFailure to use bindings built in matchCallArgument.
It will improve diagnostics like following cases:

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

f(aax: 0, bbx: 1, cc: 21, 22, 23, dd: 3, ff: 5) 
/*
  current:
    incorrect argument labels in call (have 'aax:bbx:cc:_:_:dd:ff:', expected 'aa:bb:cc:_:_:dd:ff:')
  
  will be:
    incorrect argument labels in call (have 'aax:bbx:cc:dd:ff:', expected 'aa:bb:cc:dd:ff:')
 */

This patch is needed before this future plan.

See below:

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

f(0, 1)
/*
  current:
    missing argument label 'aa:' in call

  binding based without this patch:
    incorrect argument labels in call (have '_:_:', expected 'aa:_:')

  binding based with this patch:
    missing argument label 'aa:' in call
  */

Current diagnostics is good for this case.
But binding based diagnostics will change it bad.
Because current bindings is param[0] = 1, param[1] = 0 as I wrote above.

This patch changes bindings here to param[0] = 0, param[1] = 1.
So this future plan will be good with this patch.

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 more natural label error diagnostics.

omochi · 2020-05-27T13:22:11Z

@swift-ci please smoke test

omochi · 2020-05-27T17:57:49Z

@swift-ci please smoke test

omochi · 2020-05-27T18:58:41Z

@swift-ci please smoke test OS X

omochi · 2020-05-27T20:17:35Z

Same CI failure #32042

omochi · 2020-05-28T01:50:19Z

@xedin Could you review this patch?

xedin · 2020-05-28T01:53:18Z

I am hoping to get to this really soon.

xedin

I think the idea is correct here but implementation I'm not so sure about. It seems like there might be a lot more sound algorithm hiding in a mess of matchCallArguments, should be re-think this from ground up?

I think we might want to experiment with following algorithm:

Try to claim all labeled arguments regardless of their position
- If argument was out of order but matched, adjust argument position to expected by parameter to make it seem like it was in the correct spot.
- If labeled argument couldn't be claimed at this step it's always marked as "extraneous"
  and argument positions are adjusted to pretend that it doesn't exist.
Remaining arguments are matched positionally in strict left-to-right order, first unclaimed argument is matched against first unclaimed parameter on the left.

WDYT?

xedin · 2020-05-28T19:12:21Z

lib/Sema/CSSimplify.cpp

+    /// func f(aa: Int, _ bb: Int) {}
+    /// f(0, 1)
+    /// @endcode
+    /// Choice argument[1] for parameter[1] so that parameter[0] will be bounded


Nit: bounded -> bound.

xedin · 2020-05-28T19:14:08Z

lib/Sema/CSSimplify.cpp

+    /// to argument[0] later.
+    ///
+    /// Because variadics parameter can be bounded with more than one arguments,
+    /// they don't this.


Let's re-phrase - Avoid considering variadic parameters because such parameters could be bound to one than one argument.

xedin · 2020-05-28T19:15:17Z

lib/Sema/CSSimplify.cpp

+      }
+
+      unsigned keepedArgCount = 0;
+      for (unsigned ai = nextArgIdx; ai < numArgs; ai++) {


Could you please re-name ai to be argIdx for clarity?

xedin · 2020-05-28T19:16:11Z

lib/Sema/CSSimplify.cpp

+    if (paramLabel.empty() && paramIdx && !forVariadic &&
+        !params[*paramIdx].isVariadic()) {
+      unsigned unboundedParamCount = 0;
+      for (unsigned pi = 0; pi < *paramIdx; pi++) {


Could you please re-name pi to be paramIdx for clarity?

xedin · 2020-05-28T19:17:33Z

lib/Sema/CSSimplify.cpp

+    /// they don't this.
+    if (paramLabel.empty() && paramIdx && !forVariadic &&
+        !params[*paramIdx].isVariadic()) {
+      unsigned unboundedParamCount = 0;


How about numUnclaimedParams?

xedin · 2020-05-28T19:21:59Z

test/Constraints/argument_matching.swift

@@ -719,15 +717,15 @@ func testUnlabeledParameterBindingPosition() {
    // expected-error@-1:17 {{extra argument 'xx' in call}}

    f(xx: 91, 1, 92)


Should this be diagnosed as extra argument xx: and a missing argument label 'aa:' at position #2 instead?

Hmm... Yes.
I think that most expected result here is:

bindings: aa: => `1` _ bb: => `92` errors: - extra argument `xx: 91` - missing label for `aa:`

xedin · 2020-05-28T19:30:40Z

lib/Sema/CSSimplify.cpp

+        }
+      }
+
+      unsigned keepedArgCount = 0;


Should this be numUnclaimedArgs?

xedin · 2020-05-28T19:32:56Z

lib/Sema/CSSimplify.cpp

+
+        nextArgIdx = std::max(nextArgIdx, ai);
+
+        if (keepedArgCount >= unboundedParamCount) {


Why this is relationship between unclaimed arguments and parameters important that way?

This is exactly important.

unboundedParamCount represents how many arguments should be skipped.
A number of skipping is derived from unclaimed parameters.
keepedArgCount is just counter to process skipping.

I explain with two examples.

example1

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

I use following definitions for explanation:

bindNextParameter.1 is bindNextParameter called from line 604.

bindNextParameter.2 is bindNextParameter called from line 689.

With example1, current behavior is following.

execution: bindNextParameter.1(paramIdx=0, nextArgIdx=0): params[aa:] => fail bindNextParameter.1(paramIdx=1, nextArgIdx=0): params[_ bb:] => `0` -- (t1) bindNextParameter.2(paramIdx=0, nextArgIdx=0): params[aa:] => `1` bindings: aa: => `1` _ bb: => `0` (crossing!) errors: - missing label for `aa:`

I want to shift binding target for _ bb: from 0 to 1 at (t1).
So it needs to skip one argument which is 0.
The number one is derived from unboundedParamCount.
keepedArgCount is counter which represents number of skipped arguments during skipping loop.

With example1, patched behavior is following:

execution: bindNextParameter.1(paramIdx=0, nextArgIdx=0): params[aa:] => fail bindNextParameter.1(paramIdx=1, nextArgIdx=0): adjustment: unboundedParamCount = 1 keepedArgCount = 1 nextArgIdx = 1 params[_ bb:] => `1` bindNextParameter.2(paramIdx=0, nextArgIdx=0): params[aa:] => `0` bindings: aa: => `0` _ bb: => `1` errors: - missing label for `aa:`

Labeling errors are same.
But it avoids crossed bindings.

example2

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

With example1, current behavior is following.

execution: bindNextParameter.1(paramIdx=0, nextArgIdx=0): params[aa:] => fail bindNextParameter.1(paramIdx=1, nextArgIdx=0): params[bb:] => fail bindNextParameter.1(paramIdx=2, nextArgIdx=0): params[_ cc:] => `0` -- (t2) bindNextParameter.1(paramIdx=3, nextArgIdx=0): params[dd:] => fail bindNextParameter.2(paramIdx=0, nextArgIdx=0): params[aa:] => `1` bindNextParameter.2(paramIdx=1, nextArgIdx=0): params[bb:] => `2` bindings: aa: => `1` bb: => `2` _ cc: => `0` (crossing!) dd: => none errors: - missing argument label for `aa:` - missing argument label for `bb:`

I want to shift binding target for _ cc: from 0 to 2 at (t2).
So it needs to skip two arguments which are 0 and 1.
The number two is derived from unboundedParamCount.
keepedArgCount is counter which represents number of skipped arguments during skipping loop.

With example1, patched behavior is following:

execution: bindNextParameter.1(paramIdx=0, nextArgIdx=0): params[aa:] => fail bindNextParameter.1(paramIdx=1, nextArgIdx=0): params[bb:] => fail bindNextParameter.1(paramIdx=2, nextArgIdx=0): adjustment: unboundedParamCount = 2 keepedArgCount = 2 nextArgIdx = 2 params[_ cc:] => `2` bindNextParameter.2(paramIdx=0, nextArgIdx=0): params[aa:] => `0` bindNextParameter.2(paramIdx=1, nextArgIdx=1): params[bb:] => `1` bindings: aa: => `0` bb: => `1` _ cc: => `2` dd: => none errors: - missing argument label for `aa:` - missing argument label for `bb:`

In this case, labeling errors are same.
But it avoids crossed bindings.

I understand the intent here but the problem is that unboundedParamCount is logically unrelated to keepedArgCount because unclaimed parameters could be scattered all over the call, their quantity shouldn't affect argument consideration... It's convenient that it works for some examples but I'm pretty sure there are examples made worse with these changes.

@xedin
I can understand what you say that possibility of case happen bad effect.
I created new patch at #32082 and revise this idea.
New algorithm do similar thing in stage 2 what I call.
This is run inside separated group by labeled argument binding in stage 1.
So it is well structured and I don't think bad effect more.

omochi · 2020-05-29T05:38:36Z

Reconsidering whole binding algorithm in matchCallArgument is interesting and exciting.
I believe there are more simple and better one.
I would love to try it 💪

I think your draft is basically good but have one concern.

With example below

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

it would be:

bindings:
  `_ aa:` => `1`
  `_ bb:` => none
errors:
  - extra argument `aa: 0`
  - missing argument for `_ bb:`

but it should be:

bindings:
  `_ aa:` => `aa: 0`
  `_ bb:` => `1`
errors:
  - extra label for `_ aa:`

It can be fixed with small amending.

Don't mark unclaimed labeled argument as extraneous at step 1-2.
Allow positional matching between labeled argument and unlabeled parameter at step 2.

I will make new patch in this way.

xedin · 2020-05-29T19:05:35Z

Right, I wonder if it would be possible to get access to internal labels while doing this matching... This way it would be much easier to diagnose certain cases where internal labels line up...

omochi · 2020-05-29T19:54:04Z

Does internal label mean parameter name inside function?
For example, aa in func f(_ aa: Int){}.

I did not distinguish between the two below.

func f1(_ aa: Int) {}

f1(aa: 1)
f1(xx: 1)

By the way, I succeeded to create new algorithm which can solve all cases we discussed so far at #32082 .

I now start to write explanation which will be long.

xedin · 2020-05-29T19:58:07Z

Yes, I meant the label which is used inside of function.

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

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)

omochi requested a review from xedin May 27, 2020 12:16

xedin requested changes May 28, 2020

View reviewed changes

omochi mentioned this pull request May 29, 2020

[ConstraintSystem] new argument matching algorithm #32082

Closed

shahmishal closed this Oct 5, 2020

		@@ -719,15 +717,15 @@ func testUnlabeledParameterBindingPosition() {
		// expected-error@-1:17 {{extra argument 'xx' in call}}

		f(xx: 91, 1, 92)


		nextArgIdx = std::max(nextArgIdx, ai);

		if (keepedArgCount >= unboundedParamCount) {

[ConstraintSystem] adjust matching position of unlabeled parameter #32037

[ConstraintSystem] adjust matching position of unlabeled parameter #32037

Uh oh!

Conversation

omochi commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Detail

Future plan and relation of this patch

Uh oh!

omochi commented May 27, 2020

Uh oh!

omochi commented May 27, 2020

Uh oh!

omochi commented May 27, 2020

Uh oh!

omochi commented May 27, 2020

Uh oh!

omochi commented May 28, 2020

Uh oh!

xedin commented May 28, 2020

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!

Choose a reason for hiding this comment

example1

example2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

omochi May 29, 2020 • 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!

omochi commented May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented May 29, 2020

Uh oh!

omochi commented May 29, 2020

Uh oh!

xedin commented May 29, 2020

Uh oh!

shahmishal commented Oct 1, 2020

Uh oh!

Uh oh!

omochi commented May 27, 2020 •

edited

Loading

omochi May 29, 2020 •

edited

Loading

omochi commented May 29, 2020 •

edited

Loading