Skip to content

[ConstraintSystem] new argument matching algorithm #32082

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from

Conversation

omochi
Copy link
Contributor

@omochi omochi commented May 29, 2020

I created new algorithm of argument matching that produces expected result in all difficult situation we discussed at #32037 #30444.

  • EDIT at 2020/05/30 07:32:30 JST: add document.

  • EDIT at 2020/05/30 23:59:30 JST: add result section.

@xedin Could you review this?

I am going to write full explanation soon.

The new 3 stage argument matching algorithm

I propose a new 3 stage argument matching algorihm in this patch.

Introduction

The current matchCallArgument has a problem with argument matching.
This is that diagnostic message about label errors doesn't match the result of argument matching
where input source code has a compilation error.

It is difficult to build argument matching that matches desired diagnostic message.
I tried it once with patch here #32307 , but it wasn't perfect.

This patch completely overhauls existing matchCallArgument implementation and changes it new algorithm.

In this document, I describe this algorithm.

Expression form of bindings

I had parameter => argument form in past at #32307, but I actually use argument => parameter form because it's still easier to understand.

Requirements

I enumerate an nature of expected bindings.

R-1 Unlabeled arguments are aligned with labeled arguments

Unlabeled arguments should be aligned from where the preceding labeled arguments match.

For following example:

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

f(cc: 1, 2, aa: 3, 4)
/*
  bindings:
    cc: => cc:
    2 => _ dd:
    aa: => aa:
    4 => _ bb:
*/

2 should match _ dd: since it follows cc: and
4 should match _ bb: since it follows aa:.

R-2 Unlabeled arguments must be bound to same number of unlabeled parameters

When there are same number of unlabeled arguments and parameters, they must be bound with higher priority.

For following example:

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

f(1, xx: 2, 3)
/*
  bindings:
    1 => _ bb:
    xx: => none (extra argument)
    3 => _ cc:
*/

Since there are two unlabeled arguments in 1 and 3 and
two unlabeled parameters in _ bb: and _ cc:,
they must be bound.

xx: should not match _ bb: or _ cc:.

R-3 Unlabeled arguments(parameters) should be bound to labeled parameters(arguments) if possible

When there are more unlabeled arguments than unlabled parameters,
extra unlabeled arguments should be bound as much as labeled parameters.

For following example:

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

f(1, 2)
/*
  bindings:
    1 => aa:
    2 => _ bb:
*/

There are two arguments in 1 and 3,
more than one parameter in _ bb:.

So, the extra 1 should be matched aa:.

In this case, crossed binding such as the following are undesirable.

/*
  unexpected bindings:
    1 => _ bb:
    2 => aa:
*/

Also, it is undesirable to build the following extra and missing arguments.

/*
  unexpected bindings:
    none => aa: (missing argument)
    1 => _ bb:
    2 => none (extra argument)
*/

As shown below,
even if unlabeled arguments has an unrelated labeled arguments near it,
unlabeled arguments should take precedence over labeled arguments and match labeled parameter.

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

f(1, xx: 2, 3)
/*
  bindings:
    1 => aa:
    xx: => none (extra argument)
    3 => bb:
*/

Contrary to what I have said,
when there are more unlabeled parameters than unlabeled arguments,
extra unlabeled parameters should be matched labeled arguments.

For example:

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

f(xx: 1, 2)
/*
  bindings:
    xx: => _ aa:
    none => bb: (missing argument)
    2 => _ cc:
*/

R-4 Even different labels bind each other.

Even if it's a different label,
they should be matched if that's the only option.

For example:

func f(aa: Int) {}

f(xx: 1)
/*
  bindings:
    xx: => aa:
*/

It is undesirable to build the following extra and missing arguments.

/*
  unexpected bindings:
    none => aa: (missing argument)
    xx: => none (extra argument)
*/

Discussion

Based on the requirements, consider the necessary rules.

D-1 4 levels of priority

From requirements,
we can see that there are four levels of strength in binding of arguments.
In order of strength are the following

  1. labeled arguments and labeled parameters that match
  2. unlabeled arguments and unlabeled parameters
  3. unlabeled arguments and labeled parameters, or,
    labeled arguments and unlabeled parameters
  4. labeled arguments and labeled parameters that doesn't natch

D-2 Binding of unlabeled arguments

Consider the following situations.

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

f(1, 2, 3)

Now, we suppose to decide binding of _ cc:.

Since aa: and cc: are not claimed,
we would like to match unlabeled arguments to them if possible.
Now there are two unlabeled parameters and three unlabeled arguments,
so there is one unlabeled argument left over.
So, save 1 for binding labeled arguments later, and bind 2 to _ cc:.

Next, we suppose to decide binding of _ dd:.

Since aa: and cc: are not claimed,
we would like to match unlabeled arguments to them if possible.
Since we've already bind 2 to _ cc:,
there are one unlabeled parameter in _ dd: and two unlabeled arguments in 1 and 3, so there is one unlabeled argument left over.
So, save 1 again for binding labeled argumets later, and bind 3 to _ dd:.

After binding of unlabeled arguments is finished,
unlabeled argument in 1 and unlabeled parameters in aa:, bb: are left.

So, if we match them positionally, finally get the following bindings.

/*
  bindings:
    1 => aa:
    none => bb: (missing argument)
    2 => _ cc:
    3 => _ dd:
*/

In this way,
by examining the number of unlabeled parameters,
the number of labeled parameters at left,
and the number of unlabeled arguments,
bindings satisfying R-2 and R-3 can be made.

If unlabeled parameters are more than unlabeled arguments,
the same process can be done with the opposite view.

Proposal

Based on the above, I propose argument matching algorithm with three levels of grouping.

This method divides the steps into the following three stages.

  1. Stage 1: label matching
  2. Stage 2: unlabeled matching
  3. Stage 3: label agnostic matching

In the first label match,
look for a match between all parameters and arguments.
Once matching is complete,
split remaining arguments by bound pairs into groups.

For example:

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

f(1, 2, 3, ff: 4, 5, cc: 6, 7, 8)

In stage 1, cc: and ff: are matched.
By splitting arguments with these matchings, we group them as follows.

/*
  group 1 (most left):
    [1, 2, 3] => [aa:, _ bb:]
  
  group 2 (by cc:):
    [7, 8] => [dd:, _ ee:]

  group 3 (by ff:):
    [5] => [_ gg:, _ hh:]
*/

Next, in stage 2, do unlabeled matching among groups created in stage 1.
Use the procedure described in D-2 for matching.
Then we will get following bindings.

/*
  group 1:
    1 => aa:
    2 => none (extra argument)
    3 => _ bb:

  group 2:
    7 => dd:
    8 => _ ee:

  group 3:
    5 => _ gg:
    none => _ hh: (missing argument)
*/

Now we have complete bindings.

/*
  1 => aa:
  2 => none (extra argument)
  3 => _ bb:
  ff: => ff:
  5 => _ gg:
  none => _ hh: (missing argument)
  cc: => cc:
  7 => dd:
  8 => _ ee:
*/

In this way, we can get the desired bindings.

Next, consider following example:

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

f(1, xx: 2, 3, yy: 4, 5)

In this case, group will not be split in stage 1.
Unlabeled matching as following in stage 2.

/*
  1 => aa:
  3 => _ bb:
  5 => _ ee:
*/

In stage 2, as in stage 1,
split arguments by bound pairs into groups.

/*
  group 1 (by 1):
    [xx:] => []

  group 2 (by 3):
    [yy:] => [cc:, dd:]
*/

Next, in stage 3,
do posional matching ignoring labels among among groups created in stage 2.
Then we will get following bindings.

/*
  group 1:
    xx: => none (extra argument)

  group 2:
    yy: => cc:
    none => dd: (missing argument)
*/

Now we have complete bindings.

/*
  1 => aa:
  xx: => none (extra argument)
  3 => _ bb:
  yy: => cc:
  none => dd: (missing argument)
  5 => _ ee:
*/

In this way, we can get the desired bindings.

Implementation

The implementation is as follows.

  • claim: claims arguments same as current.

  • bindParameter: After bind given paramIdx and argIdx,
    bind variadic tails if necessary.

  • claimNextNamed: deleted.

  • bindNextParameter: deleted.

  • In the part with Step 1 in comment,
    label matching in stage 1 is done.

  • In the part with Step 2 in comment,
    label matching with typo correction in stage 1 is done.

  • bindUnlabeledParameters:
    Unlabeled matching in first half of stage 2.
    Save extra unlabeled arguments.

  • bindParameterPositionally:
    Match unlabeled and labeled in second half of stage 2.
    By argument ignoreLabel,
    it is used for label agnostic matching in stage 3.

  • iterateGroups: Higher function that iterate each group which is split by existing bindings. It is used for stage 2 and 3.

  • In the part with Step 3 in comment,
    stage 2 is done.

  • In the part with Step 4 in comment,
    stage 3 is done.

  • In the part with Step 5 in comment,
    it reports extra arguments.

  • In the part with Step 6 in comment,
    it reports missing arguments.

  • In the part with Step 7 in comment,
    it reports label errors.

Result

Inconsistent bindings with diagnostics and undesired bindings are built currently in some cases.
This patch improve these cases.
I show some good results below.

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

// (1)
f(1, 2)
/*
  current:
    bindings:
      1 => _ bb:
      2 => aa:
    errors:
      - missing argument label 'aa:' in call
    problem:
      - inconsitent bindings with diagnostics

  patch:
    bindings:
      1 => aa:
      2 => _ bb:
    errors:
      - missing argument label 'aa:' in call
    resolved:
      - consistent bindings with diagnostics
 */

// (2)
f(xx: 1, 2, 3)
/*
  current:
    bindings:
      none => aa: (missing argument)
      xx: => _ bb:
      2 => none (extra argument)
      3 => none (extra argument)
    errors:
      - extra arguments at positions #2, #3 in call
      - missing argument for parameter 'aa' in call
    problem:
      - undesired result

  patch:
    bindings:
      xx: => none (extra argument)
      2 => aa:
      3 => _ bb:
    errors:
      - extra argument 'xx' in call
    resolved:
      - desired result
 */

// (3)
func f(_ aa: Int, bb: Int) {}
f(1, xx: 2)
/*
  current:
    bindings:
      1 => _ aa:
      xx: => bb:
    errors:
      - incorrect argument label in call (have '_:xx:', expected '_:bb:')
    problem:
      - no problem

  patch:
    bindings:
      1 => _ aa:
      xx: => bb:
    errors:
      - incorrect argument label in call (have '_:xx:', expected '_:bb:')
    resolved:
      - no problem
 */

// (4)
func f(_ aa: Int, bb: Int, cc: Int = 99) {}
f(1, xx: 2)

/*
  current:
    bindings:
      1 => _ aa:
      xx: => none (extra argument)
      none => bb: (missing argument)
      none => cc: (defaulted)
    errors:
      - extra argument 'xx' in call
      - missing argument for parameter 'bb' in call
    problems:
      - undesired result
      - inconsistent with (3)

  patch:
    bindings:
      1 => _ aa:
      xx: => bb:
      none => cc: (defaulted)
    errors:
      - incorrect argument label in call (have '_:xx:', expected '_:bb:')
    resolved:
      - desired result
      - consitent with (3)
 */

@omochi omochi marked this pull request as draft May 29, 2020 16:40
@omochi
Copy link
Contributor Author

omochi commented May 29, 2020

@swift-ci please smoke test

@omochi omochi requested a review from xedin May 29, 2020 22:33
@omochi omochi marked this pull request as ready for review May 29, 2020 22:57
@omochi
Copy link
Contributor Author

omochi commented May 29, 2020

Unlabeled matching in stage 2 is similar to what I wrote at #32037.
This new way is processed inside separated group by bindings from labeled matching in stage 1.
So I don't think that cause bad effect.

@omochi
Copy link
Contributor Author

omochi commented May 29, 2020

@swift-ci please smoke test

@omochi
Copy link
Contributor Author

omochi commented May 30, 2020

I explain following case:

func r27212391(x: Int, _ y: Int) { }

r27212391(3, y: 5)
/*
  current:
    3 => _ y:
    y: => x:

  patch:
    3 => none (extra argument)
    y: => x:
    none => _ y:
*/

First, current typo correction considers y is typo of x.
So arguments and parameters are aligned with anchoring y: and x: like below.

func r27212391(x: Int, _ y: Int) { }
//             ^         ^missing
  r27212391(3, y: 5)
//          ^extra

I think typo correction in one character case is too strong.
I want to change this but I didn't here.

And I also explain following case:

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

f(1, xx: 2)
/*
  current:
    none => aa: (missing argument)
    1 => _ bb:
    2 => none (extra argument)
  
  patch:
    none => aa: (missing argument)
    0 => _ bb:
    xx: => none (extra argument)
*/

Bindings are same but logic is different.

In this case, xx: doesn't match aa:.
Only one unlabeled argument and parameter match.
Arguments and parameters are aligned with anchoring with them like below.

func f(aa: Int, _ bb: Int) { }
//      ^missing   ^
                 f(0, xx: 1)
//                     ^extra

These results are much different from current.
But I think it is consistent rule that keeps left to right ordering aligned with anchor which is from more strong binding.

@omochi
Copy link
Contributor Author

omochi commented May 30, 2020

I added result section in document.

@shahmishal
Copy link
Member

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)

@shahmishal shahmishal closed this Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants