[CodeComplete] Default parameter names of completed closure to internal names #36551

ahoppen · 2021-03-23T11:39:01Z

If have a function that takes a trailing closure as follows

func sort(callback: (_ left: Int, _ right: Int) -> Bool) {}

completing a call to sort and expanding the trailing closure results in

sort { <#Int#>, <#Int#> in
  <#code#>
}

We should be doing a better job here and defaulting the trailing closure's to the internal names specified in the function signature. I.e. the final result should be

sort { left, right in
  <#code#>
}

This commit does exactly that.

Firstly, it keeps track of the closure's internal names (as specified in the declaration of sort) in the closure's type through a new InternalLabel property in AnyFunctionType::Param. Once the type containing the parameter gets canonicalized, the internal label is dropped.

Secondly, it adds a new option to ASTPrinter to always try and print parameter labels. With this option set to true, it will always print external paramter labels and, if they are present, print the internal parameter label as _ <internalLabel>.

Finally, we can use this new printing mode to print the trailing closure’s type as

<#T##callback: (Int, Int) -> Bool##(_ left: Int, _ right: Int) -> Bool#>

This is already correctly expanded by code-expand to the desired result. I also added a test case for that behaviour.

rintaro · 2021-03-23T17:35:15Z

I guess, at least, you need to change FunctionType::Profile() and AnyFunctionType::Param::operator== to take InternalLabel into account. Also getCanonicalType() should return function types without any InternalLabel so that type checker can know (_ a: Int) -> Void and (_ b: Int) -> Void is equivalent.

ahoppen · 2021-03-24T14:54:35Z

OK, I have done a 180º turnaround. After sleeping over it, I figured the internal names really have nothing to do in the type system because they don’t describe a function’s type. Instead, I discovered that we still keep the internal names around in the parameters TypeRepr and can extract them from there. The extraction is a little bit tedious, but I think it’s better than cluttering the type system with information that only code completion cares about. What do you think?

ahoppen · 2021-03-25T16:12:23Z

OK, 180º turnaround again to the way I was facing before. Turns out my initial implementation was nearly there and I was just confused because a whole bunch of the test suite failed on my machine, but I didn’t realize that was because it was already failing for my local setup on main.

So, internal names are again part of the type system and are cleared when canonicalizing the type.

@swift-ci Please test

rintaro · 2021-03-26T19:44:04Z

include/swift/AST/Types.h

Perhaps we don't need this, thus we don't need equalsIgnoringInternalName() as well?
getPlainType()->isEqual(b.getPlainType()) compares canonical types anyway (so arg: Array<T> "equals to" arg: [T]) I think we don't need to check "identical" here.

It would definitely work, but I thought it’s unintuitive if the == operator silently ignores one of the properties altogether, that’s why I went with the dedicated method.

Thinking about this some more, I think we should
a) make == check for strict equality without canonicalizing types (or remove it entirely) and provide equals functions that check the various forms of equality a Param can take or
b) accept that == that the definition of == is a bit sloppy, embrace it, don’t check for InternalLabel and add a doc comment describing the catches.

I can see advantages in either approach (clearer semantics in (a) but more concise syntax in (b)). I think I tend to lean towards (a) but don’t have too strong feelings about it. Do you @rintaro?

I'd suggest (b) for this PR. Here's the reasoning: Considering Type::operator== is delete we probably should delete AnyFunctionType::Param::operator== as well and make isEqual which compares "canonical" forms of the values. But, that also applies to AnyFunctionType::Yield::operator==. I don't think we should do it in this PR because AnyFunctionType::Yield is unrelated.

Sounds good to me.

After looking into it some more, I agree that ignoring the internal name in operator== is absolutely the right choice. I wasn’t aware that we are always using canonical types in operator== for types and that it’s rather more difficult to compare the sugared types. As such, ignoring the internal name in Param::operator== follows the current paradigm.

rintaro

This looks good to me. We still need serialization support for this. Do you intend to do it in followups?

rintaro · 2021-03-30T20:51:33Z

lib/AST/ASTContext.cpp

isGenericFunctionTypeCanonical as well?

Ah, I didn't realize there was isGenericFunctionTypeCanonical. Added it there as well and renamed isFunctionTypeCanonical to isAnyFunctionTypeCanonical (with Any) to imply there is a generic version as well.

…al names If have a function that takes a trailing closure as follows ``` func sort(callback: (_ left: Int, _ right: Int) -> Bool) {} ``` completing a call to `sort` and expanding the trailing closure results in ``` sort { <#Int#>, <#Int#> in <#code#> } ``` We should be doing a better job here and defaulting the trailing closure's to the internal names specified in the function signature. I.e. the final result should be ``` sort { left, right in <#code#> } ``` This commit does exactly that. Firstly, it keeps track of the closure's internal names (as specified in the declaration of `sort`) in the closure's type through a new `InternalLabel` property in `AnyFunctionType::Param`. Once the type containing the parameter gets canonicalized, the internal label is dropped. Secondly, it adds a new option to `ASTPrinter` to always try and print parameter labels. With this option set to true, it will always print external paramter labels and, if they are present, print the internal parameter label as `_ <internalLabel>`. Finally, we can use this new printing mode to print the trailing closure’s type as ``` <#T##callback: (Int, Int) -> Bool##(_ left: Int, _ right: Int) -> Bool#> ``` This is already correctly expanded by code-expand to the desired result. I also added a test case for that behaviour.

ahoppen · 2021-04-01T17:15:30Z

@swift-ci Please smoke test

ahoppen · 2021-04-01T17:27:52Z

This looks good to me. We still need serialization support for this. Do you intend to do it in followups?

I’ll look into it in a follow-up PR.

rintaro · 2021-04-02T22:56:38Z

@swift-ci Please smoke test

rintaro · 2021-04-05T16:02:35Z

@swift-ci Please smoke test

ahoppen · 2021-04-06T10:22:34Z

@swift-ci Please smoke test Windows

ahoppen · 2021-04-06T13:30:22Z

Windows failures is unrelated. Merging.

rintaro · 2021-04-06T16:34:19Z

test/IDE/complete_call_arg.swift

+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=CLOSURE_PARAM_WITH_INTERNAL_NAME | %FileCheck %s -check-prefix=CLOSURE_PARAM_WITH_INTERNAL_NAME
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=CLOSURE_PARAM_WITH_PARENS | %FileCheck %s -check-prefix=CLOSURE_PARAM_WITH_PARENS
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=OPTIONAL_CLOSURE_PARAM | %FileCheck %s -check-prefix=OPTIONAL_CLOSURE_PARAM
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=ESCAPING_OPTIONAL_CLOSURE_PARAM | %FileCheck %s -check-prefix=ESCAPING_OPTIONAL_CLOSURE_PARAM
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -code-completion-token=COMPLETE_CLOSURE_PARAM_WITHOUT_INTERNAL_NAMES | %FileCheck %s -check-prefix=COMPLETE_CLOSURE_PARAM_WITHOUT_INTERNAL_NAMES
+


Ah sorry, could you remove these lines? They are redundant. Since #36725 , this test file uses -batch-code-completion mode which checks all tokens in a single invocation. See also #28946

Didn’t notice this when rebasing. Thanks. I’ve set up a PR here: #36796

ahoppen requested review from CodaFi and rintaro March 23, 2021 11:39

ahoppen force-pushed the pr/internal-labels-in-closures branch 2 times, most recently from 783de20 to d977ac1 Compare March 24, 2021 14:53

ahoppen marked this pull request as ready for review March 24, 2021 14:54

ahoppen force-pushed the pr/internal-labels-in-closures branch from d977ac1 to 53ea176 Compare March 25, 2021 16:08

ahoppen changed the title ~~WIP: [CodeComplete] Default parameter names of completed closure to internal names~~ [CodeComplete] Default parameter names of completed closure to internal names Mar 25, 2021

ahoppen force-pushed the pr/internal-labels-in-closures branch from 53ea176 to 65aa5a0 Compare March 26, 2021 13:19

swiftlang deleted a comment from swift-ci Mar 26, 2021

rintaro reviewed Mar 26, 2021

View reviewed changes

ahoppen force-pushed the pr/internal-labels-in-closures branch from 65aa5a0 to f796af7 Compare March 30, 2021 10:22

rintaro reviewed Mar 31, 2021

View reviewed changes

ahoppen force-pushed the pr/internal-labels-in-closures branch from f796af7 to 865e80f Compare April 1, 2021 17:15

ahoppen merged commit fd8e349 into swiftlang:main Apr 6, 2021

ahoppen deleted the pr/internal-labels-in-closures branch April 6, 2021 13:30

rintaro reviewed Apr 6, 2021

View reviewed changes

This was referenced Apr 7, 2021

[CodeCompletion] Remove test lines that are already run using batch completion #36796

Merged

[Serialization] Serialize internal closure labels #36866

Merged

[CodeComplete] Default parameter names of completed closure to internal names #36551

[CodeComplete] Default parameter names of completed closure to internal names #36551

Uh oh!

Conversation

ahoppen commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Mar 23, 2021

Uh oh!

ahoppen commented Mar 24, 2021

Uh oh!

ahoppen commented Mar 25, 2021

Uh oh!

rintaro Mar 26, 2021 • 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!

ahoppen Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 29, 2021 • 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!

ahoppen Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Mar 30, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Apr 1, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Apr 1, 2021

Uh oh!

ahoppen commented Apr 1, 2021

Uh oh!

rintaro commented Apr 2, 2021

Uh oh!

rintaro commented Apr 5, 2021

Uh oh!

ahoppen commented Apr 6, 2021

Uh oh!

ahoppen commented Apr 6, 2021

Uh oh!

rintaro Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

ahoppen Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ahoppen commented Mar 23, 2021 •

edited

Loading

rintaro Mar 26, 2021 •

edited

Loading

rintaro Mar 29, 2021 •

edited

Loading

rintaro left a comment •

edited

Loading