Skip to content

[ConstraintSystem] adjust matching position of unlabeled parameter #32037

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.

Sign up for GitHub

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 1 commit into from
Closed

[ConstraintSystem] adjust matching position of unlabeled parameter #32037

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 50 additions & 6 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
// Local function that retrieves the next unclaimed argument with the given
// name (which may be empty). This routine claims the argument.
auto claimNextNamed = [&](unsigned &nextArgIdx, Identifier paramLabel,
Optional<unsigned> paramIdx,
bool ignoreNameMismatch,
bool forVariadic = false) -> Optional<unsigned> {
// Skip over any claimed arguments.
Expand All @@ -298,6 +299,47 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
if (numClaimedArgs == numArgs)
return None;

/// When we determine which argument is bound to unlabeled parameter,
/// consider still unbounded parameter which is prior to current parameter.
/// In order not to intersect binding position that remaining parameter will
/// bind later, skip a unlabeled argument as much as it can.
///
/// For example:
/// @code
/// func f(aa: Int, _ bb: Int) {}
/// f(0, 1)
/// @endcode
/// Choice argument[1] for parameter[1] so that parameter[0] will be bounded
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: bounded -> bound.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

if (paramLabel.empty() && paramIdx && !forVariadic &&
!params[*paramIdx].isVariadic()) {
unsigned unboundedParamCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about numUnclaimedParams?

for (unsigned pi = 0; pi < *paramIdx; pi++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (parameterBindings[pi].empty()) {
if (params[pi].isVariadic() || paramInfo.hasDefaultArgument(pi))
continue;

unboundedParamCount++;
}
}

unsigned keepedArgCount = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be numUnclaimedArgs?

for (unsigned ai = nextArgIdx; ai < numArgs; ai++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (claimedArgs[ai])
continue;

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

if (keepedArgCount >= unboundedParamCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@omochi omochi May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

break;
}
keepedArgCount++;
}
}

// Go hunting for an unclaimed argument whose name does match.
Optional<unsigned> claimedWithSameName;
for (unsigned i = nextArgIdx; i != numArgs; ++i) {
Expand Down Expand Up @@ -390,8 +432,8 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
// Handle variadic parameters.
if (param.isVariadic()) {
// Claim the next argument with the name of this parameter.
auto claimed =
claimNextNamed(nextArgIdx, param.getLabel(), ignoreNameMismatch);
auto claimed = claimNextNamed(nextArgIdx, param.getLabel(), paramIdx,
ignoreNameMismatch);

// If there was no such argument, leave the parameter unfulfilled.
if (!claimed) {
Expand All @@ -412,8 +454,10 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
{
nextArgIdx = *claimed;
// Claim any additional unnamed arguments.
while (
(claimed = claimNextNamed(nextArgIdx, Identifier(), false, true))) {
while ((claimed = claimNextNamed(
nextArgIdx, /*paramLabel=*/Identifier(), /*paramIdx=*/None,
/*ignoreNameMismatch=*/false,
/*forVariadic=*/true))) {
parameterBindings[paramIdx].push_back(*claimed);
}
}
Expand All @@ -423,8 +467,8 @@ matchCallArguments(SmallVectorImpl<AnyFunctionType::Param> &args,
}

// Try to claim an argument for this parameter.
if (auto claimed =
claimNextNamed(nextArgIdx, param.getLabel(), ignoreNameMismatch)) {
if (auto claimed = claimNextNamed(nextArgIdx, param.getLabel(), paramIdx,
ignoreNameMismatch)) {
parameterBindings[paramIdx].push_back(*claimed);
return;
}
Expand Down
16 changes: 8 additions & 8 deletions test/Constraints/argument_matching.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -705,12 +705,10 @@ func testUnlabeledParameterBindingPosition() {
// expected-error@-1:6 {{missing argument label 'aa:' in call}}

f(0, xx: 1)
// expected-error@-1:7 {{missing argument for parameter 'aa' in call}}
// expected-error@-2:14 {{extra argument 'xx' in call}}
// expected-error@-1:6 {{incorrect argument labels in call (have '_:xx:', expected 'aa:_:')}}

f(xx: 0, 1)
// expected-error@-1:7 {{missing argument for parameter 'aa' in call}}
// expected-error@-2:14 {{extra argument in call}}
// expected-error@-1:6 {{incorrect argument label in call (have 'xx:_:', expected 'aa:_:')}}

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

f(xx: 91, 1, 92)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

bindings:
  aa: => `1`
  _ bb: => `92`

errors:
  - extra argument `xx: 91`
  - missing label for `aa:`

// expected-error@-1 {{extra arguments at positions #2, #3 in call}}
// expected-error@-1 {{extra arguments at positions #1, #3 in call}}
// expected-error@-2 {{missing argument for parameter 'aa' in call}}
}

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

f(bb: 1, 0, 2)
// expected-error@-1 {{unnamed argument #3 must precede argument 'bb'}}
// expected-error@-1 {{unnamed argument #2 must precede argument 'bb'}}
}

do {
Expand Down Expand Up @@ -755,15 +753,17 @@ func testUnlabeledParameterBindingPosition() {
func f(aa: Int, _ bb: Int, _ cc: Int) {}

f(0, 1)
// expected-error@-1:7 {{missing argument for parameter 'aa' in call}}
// expected-error@-1:6 {{missing argument label 'aa:' in call}}
// expected-error@-2:11 {{missing argument for parameter #3 in call}}
}

do {
// expected-note@+1 *{{'f(aa:_:_:)' declared here}}
func f(aa: Int, _ bb: Int = 81, _ cc: Int) {}

f(0, 1)
// expected-error@-1:7 {{missing argument for parameter 'aa' in call}}
// expected-error@-1:6 {{missing argument label 'aa:' in call}}
// expected-error@-2:11 {{missing argument for parameter #3 in call}}
}

do {
Expand Down