[CodeCompletion] Explicitly support enum pattern matching #38627

ahoppen · 2021-07-26T16:23:04Z

Pattern matching in Swift can either be expression pattern matching by comparing two instances using the ~= operator or using enum matching by matching the enum case and its associated types (+ tuple pattern matching, but that’s not relevant here). We currenlty only consider the expression pattern matching case for code completion. To provide enum pattern matching results, we thus need to have a ~= operator between the code completion token and the match expression

For example, when we are completing

enum MyEnum {
  case myCase(String)
}

switch x {
case .#^COMPLETE^#
}

then we are looking up all overloads of ~= and try to match it to the call arguments (<Code Completion Type>, MyEnum).
The way we currently get #^COMPLETE^# to offer members of MyEnum, is that we are trying to make sure that the ~=<T: Equatable>(T, T) operator defined in the standard library is the best solution even though it has fixes associated with it. For that we need to carefully make sure to ignore other, more favourable overloads of ~= in filterSolutions so that ~=<T: Equatable>(T, T) has the best score.

This poses several problems:

If the user defines a custom overload of ~= that we don't prune when filtering solutions (e.g. func ~=(pattern: OtherType, value: MyEnum) -> Bool), it gets a better score than ~=<T: Equatable>(T, T) and thus we only offer members of OtherType instead of members from MyEnum
We are also suggesting static members of MyEnum, even though we can't pattern match them due to the lack of the ~= operator.

If we detect that the completion expression is in a pattern matching position, also suggests all enum members of the matched type. This allows us to remove the hack which deliberately ignores certain overloads of ~= since we no longer rely on ~=<T: Equatable>(T, T). It thus provides correct results in both of the above cases.

Fixes rdar://77263334 [SR-14547]

ahoppen · 2021-07-26T18:58:31Z

@swift-ci Please smoke test

rintaro · 2021-07-26T18:06:22Z

lib/IDE/CodeCompletion.cpp

@@ -4665,6 +4677,21 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
                             /*includeProtocolExtensionMembers*/true);
  }

+  /// Complete all enum members declared on \p T.
+  void getUnresolvedEnumElementCompletions(Type T) {


Patterns for enum cases are called EnumElementPattern. So I think getEnumElementPatternCompletions() is more correct?

You 👍ed. So are you renaming this?

Thanks. Good catch. I changed it at some point but that rename must have gotten lost in the refactoring. Renamed it again.

rintaro · 2021-07-29T20:46:53Z

lib/Parse/ParsePattern.cpp

@@ -1333,6 +1333,20 @@ ParserResult<Pattern> Parser::parseMatchingPattern(bool isExprBasic) {
  // matching-pattern ::= expr
  // Fall back to expression parsing for ambiguous forms. Name lookup will
  // disambiguate.
+  if (Tok.is(tok::period_prefix) && peekToken().is(tok::code_complete)) {


If I understand correctly, I think this doesn't cover case .foo(.#^HERE^#). Could you make it work in nested cases too?

Also considering case .foo(.#^HERE^#) case, we don't know foo is a static function or an enum element. If it's a static function, we can only use ~= because enum element patterns can't be nested in an expression pattern. Can we differentiate it in sawSolution()?

case .foo(.#^HERE^#) works because the parser correctly parses the .foo as an enum element pattern, so we don’t need to worry about it. I’ll add test cases for it.

rintaro · 2021-07-29T20:50:57Z

include/swift/IDE/CodeCompletion.h

@@ -533,6 +533,9 @@ enum class CompletionKind {
  None,
  Import,
  UnresolvedMember,
+  /// An unresolved member completion in a pattern matching position.
+  /// E.g. \code if case .#^COMPLETE^# = x {} \endcode
+  PatternMatching,


I'm not a fan of this naming. Could you include something that imply "after dot" and "without base expr/pattern"? For example PatternAfterDotWithoutBase or UnresolvedDotPattern?

Renamed to UnresolvedDotPattern.

rintaro · 2021-07-30T06:51:10Z

lib/Sema/TypeCheckCodeCompletion.cpp

+  addTypeToResultVectorIfNecessary(Results.ExprPatternTypes,
+                                   CompletionExprType);
+
+  if (auto MatchVar = PatternMatching->getMatchVar()) {


Can't we derive MatchVar or MatchingVarType from Solution? If not, could you make it so?
If we can do that, I think we don't need to add a dedicated completion kind. Instead, we can detect it in UnresolvedMemberTypeCheckCompletionCallback::sawSolution()

Not easily, I think. We could maybe search through the constraint locators for one that declares a variable named $match, but that feels awfully ad-hoc to me and potentially fragile if $match somehow leaks to nested scopes.

Also note that we only offer enum cases for the type of the match var, so UnresolvedMemberTypeCheckCompletionCallback would need to keep track of the match var types separately from the current Results.

We could merge PatternMatchingTypeCheckCompletionCallback and UnresolvedMemberTypeCheckCompletionCallback so that UnresolvedMemberTypeCheckCompletionCallback also has ExprPattern *PatternMatching and SmallVector<Type, 1> EnumPatternTypes members, which can be nullptr/empty if we are not completing in a pattern matching location. I’m not sure if I like that better though… WDYT?

ahoppen · 2021-07-30T10:13:37Z

@swift-ci Please smoke test

ahoppen · 2021-08-11T14:31:41Z

@rintaro helpfully noted that the previous implementation didn’t support nested enum pattern matching, e.g. in the following, we weren’t suggesting foo.

enum PlainEnum {
  case foo(Int)
}
enum WithNestedEnum {
  case bar(PlainEnum)
}
func completeInNestedPatternMatching(x: WithNestedEnum) {
  switch x {
  case .bar(.#^COMPLETE^#)
}

I remodeled the code completion to not be a custom completion kind anymore (like @rintaro suggested earlier). Instead, we discover if the code completion is inside a pattern matching location (by looking if it’s an argument to an implicit ~= operator where the second variable is named $match. If this is the case, we also suggest all enum members of $match’s type, like we were previously doing in the custom completion kind.

ahoppen · 2021-08-11T14:31:53Z

@swift-ci Please test

rintaro · 2021-08-12T17:23:54Z

lib/Sema/TypeCheckCodeCompletion.cpp

+  TupleExpr *ArgTuple =
+      dyn_cast_or_null<TupleExpr>(CS.getParentExpr(CompletionExpr));
+  if (!ArgTuple || !ArgTuple->isImplicit() || ArgTuple->getNumElements() != 2) {
+    return nullptr;
+  }
+
+  auto Binary = dyn_cast_or_null<BinaryExpr>(CS.getParentExpr(ArgTuple));
+  if (!Binary || !Binary->isImplicit()) {
+    return nullptr;
+  }


This probably conflicts with #38836 cc: @hamishknight

rintaro · 2021-08-12T17:28:36Z

lib/IDE/CodeCompletion.cpp

@@ -4665,6 +4675,21 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
                             /*includeProtocolExtensionMembers*/true);
  }

+  /// Complete all enum members declared on \p T.
+  void getUnresolvedEnumElementCompletions(Type T) {
+    if (!T->mayHaveMembers())


Do you think it makes sense to check T->getAnyNominal() is EnumDecl here?

Seems like a reasonable and easy performance improvement. 👍

ahoppen · 2021-08-25T16:57:25Z

@swift-ci Please smoke test

rintaro

Implementation LGTM except a couple of comments.
Please cooperate with Hamish when landing this.

rintaro · 2021-08-25T23:48:52Z

lib/IDE/CodeCompletion.cpp

@@ -4694,6 +4704,21 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
                             /*includeProtocolExtensionMembers*/true);
  }

+  /// Complete all enum members declared on \p T.
+  void getUnresolvedEnumElementCompletions(Type T) {
+    if (!T->mayHaveMembers() || !T->getAnyNominal() || !isa<EnumDecl>(T->getAnyNominal()))


T->getAnyNominal() == true implies T->mayHaveMembers() so

Suggested change

if (!T->mayHaveMembers() || !T->getAnyNominal() || !isa<EnumDecl>(T->getAnyNominal()))

if (!llvm::isa_and_nonnull<EnumDecl>(T->getAnyNominal()))

I imported isa_and_nonnull into swift namespace in #39078 so just

Suggested change

if (!T->mayHaveMembers() || !T->getAnyNominal() || !isa<EnumDecl>(T->getAnyNominal()))

if (!isa_and_nonnull<EnumDecl>(T->getAnyNominal()))

Nice. I didn’t know that existed. Thanks.

rintaro · 2021-08-25T23:49:53Z

lib/IDE/CodeCompletion.cpp

@@ -4665,6 +4677,21 @@ class CompletionLookup final : public swift::VisibleDeclConsumer {
                             /*includeProtocolExtensionMembers*/true);
  }

+  /// Complete all enum members declared on \p T.
+  void getUnresolvedEnumElementCompletions(Type T) {


You 👍ed. So are you renaming this?

Pattern matching in Swift can either be expression pattern matching by comparing two instances using the `~=` operator or using enum matching by matching the enum case and its associated types (+ tuple pattern matching, but that’s not relevant here). We currenlty only consider the expression pattern matching case for code completion. To provide enum pattern matching results, we thus need to have a `~=` operator between the code completion token and the match expression For example, when we are completing ```swift enum MyEnum { case myCase(String) } switch x { case .#^COMPLETE^# } ``` then we are looking up all overloads of `~=` and try to match it to the call arguments `(<Code Completion Type>, MyEnum)`. The way we currently get `#^COMPLETE^#` to offer members of `MyEnum`, is that we are trying to make sure that the `~=<T: Equatable>(T, T)` operator defined in the standard library is the best solution even though it has fixes associated with it. For that we need to carefully make sure to ignore other, more favourable overloads of `~=` in `filterSolutions` so that `~=<T: Equatable>(T, T)` has the best score. This poses several problems: - If the user defines a custom overload of `~=` that we don't prune when filtering solutions (e.g. `func ~=(pattern: OtherType, value: MyEnum) -> Bool`), it gets a better score than `~=<T: Equatable>(T, T)` and thus we only offer members of `OtherType` instead of members from `MyEnum` - We are also suggesting static members of `MyEnum`, even though we can't pattern match them due to the lack of the `~=` operator. If we detect that the completion expression is in a pattern matching position, also suggests all enum members of the matched type. This allows us to remove the hack which deliberately ignores certain overloads of `~=` since we no longer rely on `~=<T: Equatable>(T, T)`. It thus provides correct results in both of the above cases. Fixes rdar://77263334 [SR-14547]

ahoppen · 2021-09-01T11:59:40Z

@swift-ci Please smoke test

ahoppen requested a review from rintaro July 26, 2021 16:23

ahoppen force-pushed the pr/enum-matching-completion branch from d808efb to 6263d21 Compare July 26, 2021 18:58

ahoppen mentioned this pull request Jul 26, 2021

[WIP][CodeCompletion] Migrate argument position completion to the solver-based implementation #38266

Open

rintaro reviewed Jul 29, 2021

View reviewed changes

rintaro reviewed Jul 30, 2021

View reviewed changes

ahoppen force-pushed the pr/enum-matching-completion branch from 6263d21 to 1096697 Compare July 30, 2021 10:13

ahoppen force-pushed the pr/enum-matching-completion branch 2 times, most recently from 6eea740 to 5ecc2c9 Compare August 11, 2021 14:31

ahoppen changed the title ~~[CodeCompletion] Add a dedicated pattern matching completion kind~~ CodeCompletion] Explicitly support enum pattern matching Aug 12, 2021

ahoppen changed the title ~~CodeCompletion] Explicitly support enum pattern matching~~ [CodeCompletion] Explicitly support enum pattern matching Aug 12, 2021

rintaro reviewed Aug 13, 2021

View reviewed changes

ahoppen force-pushed the pr/enum-matching-completion branch 2 times, most recently from 74a8c0d to 3770c5f Compare August 25, 2021 16:57

rintaro approved these changes Aug 25, 2021

View reviewed changes

ahoppen force-pushed the pr/enum-matching-completion branch from 3770c5f to 12ff361 Compare September 1, 2021 11:59

ahoppen merged commit 5dea77e into swiftlang:main Sep 1, 2021

ahoppen deleted the pr/enum-matching-completion branch September 1, 2021 15:59

ahoppen mentioned this pull request Sep 2, 2021

[SR-14547] Enum cases not shown in code completion for enum with associated values conforming to Error when Foundation is imported #56899

Closed

	if (!T->mayHaveMembers() \|\| !T->getAnyNominal() \|\| !isa<EnumDecl>(T->getAnyNominal()))
	if (!llvm::isa_and_nonnull<EnumDecl>(T->getAnyNominal()))

	if (!T->mayHaveMembers() \|\| !T->getAnyNominal() \|\| !isa<EnumDecl>(T->getAnyNominal()))
	if (!isa_and_nonnull<EnumDecl>(T->getAnyNominal()))

[CodeCompletion] Explicitly support enum pattern matching #38627

[CodeCompletion] Explicitly support enum pattern matching #38627

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ahoppen commented Jul 26, 2021

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!

rintaro Jul 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!

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!

ahoppen commented Jul 30, 2021

Uh oh!

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

Uh oh!

Uh oh!

ahoppen commented Aug 11, 2021

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!

ahoppen commented Aug 25, 2021

Uh oh!

rintaro 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!

ahoppen commented Sep 1, 2021

Uh oh!

Uh oh!

ahoppen commented Jul 26, 2021 •

edited

Loading

rintaro Jul 29, 2021 •

edited

Loading

ahoppen commented Aug 11, 2021 •

edited

Loading