[CodeCompletion] Update member completion to handle ambiguous and invalid base expressions #33749

nathawes · 2020-09-01T22:08:32Z

This hooks up member completion to the new typeCheckForCodeCompletion API to generate completions from all solutions the constraint solver produces – including ones requiring fixes – rather than relying on a single solution being applied to the AST (if any). This lets us still produce member completion results when the base expression is ambiguous or invalid.

Whenever typeCheckExpression is called on an expression containing a code completion expression and a CompletionCallback has been set, each solution formed is passed to the callback so the type and decl of the base expression can be extracted along with any other info needed to generate member completions.

…e expression to the DotExpr completion callback.

…mpletion. This hooks up member completion to the new typeCheckForCodeCompletion API to generate completions from all solutions the constraint solver produces (include ones requiring fixes) rather than relying purely the single solution being applied to the AST (if any). This lets us still give completion results in ambiguous and invalid code.

…tion` APIs

…g for code completion

…expression and use it If there was an invalid reference which was caught by pre-check, let's remove all context besides code completion itself and use it to produce code completion results.

…or code completion To aid code completion, we need to attempt to convert type holes back into underlying generic parameters if possible, since type of the code completion expression is used as "expected" (or contextual) type so it's helpful to know what requirements it has to filter the list of possible member candidates e.g. ```swift func test<T: P>(_: [T]) {} test(42.#^MEMBERS^#) ``` It's impossible to resolve `T` in this case but code completion expression should still have a type of `[T]` instead of `[<<hole>>]` because it helps to produce correct contextual member list based on a conformance requirement associated with generic parameter `T`.

…ion in cases where typeCheckExpression is never called. This happens when, e.g. an expression being switched on is invalid so expression patterns in the switch cases (which may contain the completion expression) are not checked. Also setup the Lookup object to handle member completion in ObjC selector expressions correctly, and fix passing the wrong expression when computing isStaticallyDerivedMetatype().

…etion` If an expression fails pre-check or constraint generation, code completion should be performed directly on the `CodeCompletionExpr` as a fallback. Let's factor that logic out from `solveForCodeCompletion` and put it directly into `typeCheckForCodeCompletion` because it's easier to establish fault conditions there.

…on` to operate on target instead of expression Using `SolutionApplicationTarget` make it easier to propage contextual information and avoid mistakes of using incorrect accessors for constraint generation.

…f `typeCheckForCodeCompletion` There is no way to separate viability/applicability checking from `TypeChecker::typeCheckForCodeCompletion` because multi-statement closures could be either type-checked together with enclosing context (e.g. when closure represents a function builder body), or separately - when it's just a regular closure. Due to this "duality" we need to attempt to run code completion code to determine whether body of the closure participated in type-check and if it didn't, fallback to a `typeCheckExpression`.

…n to `typeCheckExpression` This fallback to `typeCheckExpression` is triggered when it's determined that code completion expression is located inside of a multi-statement closure and its body is going going to participate in type-check.

…ts test. We were reporting methods that return function types that return void (rather than returning void directly) as being invalid in contexts that expect non-void expressions and testing for that incorrect behavior.

…sion Having all information possible makes it much easier to determine when it's appropriate to fallback to type-check code completion expression and whether it's possible to add some more context to it.

…e valid.

…ayed parsing as well We were previously only doing it when parsing up front.

…improved type relation.

…dds to single expression bodies for void-returning functions This makes TypeCheckNodeAtLoc match the regular function body typechecking behavior and fixes a crash in test/IDE/complete_value_expr.swift

…mbiguous.

…into `sawSolution` Instead of exposing archetypes during solution generation, let's do that based on solution data when a type of code completion expression is requested by code completion callback.

Code completion expression type could be a hole if and only if there is obosolutely no contextual information available e.g. `let _ = .#^MEMBER^#`

…ined If base type couldn't be determined (e.g. because base expression is an invalid reference), let's not attempt to do a lookup since it wouldn't produce any useful results anyway.

…tion when there's no expected type. The tests were matching the previous implementation's output, which sometimes produced 'unknown' and sometimes 'unrelated' in cases where there was no expected type from the context.

… in the new member completion implementation.

nathawes · 2020-09-01T22:08:48Z

@swift-ci please test

nathawes · 2020-09-01T22:09:11Z

@swift-ci Please Build Toolchain macOS Platform

swift-ci · 2020-09-03T23:22:19Z

Build failed
Swift Test Linux Platform
Git Sha - 491b691

swift-ci · 2020-09-03T23:28:10Z

Build failed
Swift Test OS X Platform
Git Sha - 491b691

rintaro · 2020-09-04T16:38:38Z

Toolchain https://ci.swift.org/job/swift-PR-toolchain-osx/677/artifact/branch-master/swift-PR-33749-677-osx.tar.gz

rintaro

Looks great in general 👍

rintaro · 2020-09-02T02:32:43Z

lib/Sema/TypeCheckCodeCompletion.cpp

+    ReferencedDecl = SelectedOverload->choice.getDeclOrNull();
+
+  auto Key = std::make_pair(BaseTy, ReferencedDecl);
+  auto Ret = BaseToSolutionIdx.insert({Key, Solutions.size()});


Could you explain why Solutions.size() is a part of the key?

It's the value here, rather than the key. If the key is not already in the BaseToSolutionIdx map we will insert a new entry into the Solutions vector, and Solutions.size() will be its index.

Ah, sorry. I misread the code 😓

rintaro · 2020-09-04T16:43:45Z

include/swift/Sema/IDETypeChecking.h

+    /// Called for each solution produced while  type-checking an expression containing a code
+    /// completion expression.
+    void sawSolution(const constraints::Solution &solution) override;
+  };


I feel like we should have dedicated header for code completion specific type checking. e.g. CompletionTypeChecking.h

Makes sense to me, particularly as we'll hopefully be adding a bunch more of these.

rintaro · 2020-09-04T16:51:45Z

lib/IDE/CodeCompletion.cpp

@@ -1334,9 +1335,12 @@ CodeCompletionResult *CodeCompletionResultBuilder::takeResult() {
    }

    auto typeRelation = ExpectedTypeRelation;
+    // FIXME: we don't actually have enough info to compute
+    // IsImplicitlyCurriedInstanceMethod here.


(Not in this PR) I think we should calculate it in each result building logic which knows the actual result type.

E.g.

Builder.addTypeAnnotation(resultTy, PrintOptions()); Builder.setExpectedTypeRelation(calculateMaxTypeRelation(resultTy, expectedTypes, DC));

rintaro · 2020-09-04T16:55:11Z

include/swift/Sema/IDETypeChecking.h

+    SourceLoc DotLoc;
+    DeclContext *DC;
+    CodeCompletionExpr *CompletionExpr;
+    SmallVector<SolutionInfo, 4> Solutions;


Naming Solution can be confusable with constraints::Solution. But I'm not able to come up with better name 🤔

Yeah, I'm not super happy with most of the names. I was hoping to come up with something better for "CompletionCollector" and "DotExprLookup" too.

How about
CompletionCollector -> TypeCheckCompletionCallback
DotExprLookup -> DotExprTypeCheckCompletionCallback
Solution -> Result

rintaro · 2020-09-04T16:59:18Z

lib/IDE/CodeCompletion.cpp

@@ -1526,6 +1530,8 @@ void CodeCompletionContext::sortCompletionResults(
 }

 namespace {
+class CompletionLookup;
+
 class CodeCompletionCallbacksImpl : public CodeCompletionCallbacks {
  CodeCompletionContext &CompletionContext;
  std::vector<RequestedCachedModule> RequestedModules;


Is this still needed in CodeCompletionCallbacksImpl?

Oh maybe not - thanks!

rintaro · 2020-09-04T17:06:49Z

lib/IDE/CodeCompletion.cpp

+    DotExprLookup Lookup(DotLoc, CurDeclContext, CodeCompleteTokenExpr);
+    llvm::SaveAndRestore<CompletionCollector*>
+      CompletionCollector(Context.CompletionCallback, &Lookup);
+    typeCheckContextAt(CurDeclContext, CompletionLoc);


(Not in this PR) When we migrate everything, we should rename this to something else :)
e.g.

performCompletionTypeChecking(CurDeclContext, ComletionLoc, Lookup);

rintaro · 2020-09-04T17:11:45Z

lib/IDE/CodeCompletion.cpp

+
+    // This (hopefully) only happens in cases where the expression isn't
+    // typechecked during normal compilation either (e.g. member completion in a
+    // switch case where there switched value is invalid). Having normal


According the the language reference, it's called "control expression"
https://docs.swift.org/swift-book/ReferenceManual/Statements.html#ID436

rintaro · 2020-09-04T17:18:00Z

lib/Sema/TypeCheckCodeCompletion.cpp

+  // If there was no completion expr (e.g. if the code completion location is
+  // within an ErrorExpr without an valid subexpr due to parser error recovery)
+  // bail.
+  if (!contextAnalyzer.hasCompletionExpr())
+    return false;


The original expression in ErrorExpr is now walked into. If you know other cases, please update the comment.

Right, I think this branch is impossible to get into at the moment since we're only doing this for member completion. Do you know if once we change the other completion kinds to use this approach there will ever be a case where no CodeCompletionExpr is formed within an ErrorExpr, or any other kind of expression containing the completion loc? I assumed this was possible, e.g. if the completion location is in the middle of some nonsensical tokens between statement starting tokens that are skipped over during parser recovery, but if we're always guaranteed to have a CodeCompletionExpr regardless of the completion kind this should probably be an assertion.

I don’t know off the top of my head. But I think we should keep this return false path, just to be on the safe side.

rintaro · 2020-09-04T17:25:41Z

lib/Sema/TypeCheckCodeCompletion.cpp

+    // First, pre-check the expression, validating any types that occur in the
+    // expression and folding sequence expressions.
+    auto failedPreCheck = ConstraintSystem::preCheckExpression(
+        expr, DC,
+        /*replaceInvalidRefsWithErrors=*/true);
+
+    target.setExpr(expr);
+
+    if (failedPreCheck)
+      return false;


If preCheckExpression(), it falls back to normal type checking which calls preCheckExpression() again. @xedin Is it safe to call preCheckExpression() on already pre-checked expressions?

Yeah, it's effectively a noop.

rintaro · 2020-09-04T17:31:24Z

lib/Sema/TypeCheckCodeCompletion.cpp

+                                               /*contextualType=*/Type(),
+                                               /*isDiscarded=*/true);
+
+    (void)solveForCodeCompletion(completionTarget);


Assert the result is not CompletionResult::NotApplicable?

rintaro · 2020-09-04T19:06:19Z

test/IDE/complete_enum_elements.swift

@@ -133,7 +133,7 @@ enum FooEnum: CaseIterable {
 // FOO_ENUM_DOT_INVALID-NEXT: Decl[EnumElement]/CurrNominal: Foo1[#FooEnum#]{{; name=.+$}}
 // FOO_ENUM_DOT_INVALID-NEXT: Decl[EnumElement]/CurrNominal: Foo2[#FooEnum#]{{; name=.+$}}
 // FOO_ENUM_DOT_INVALID-NEXT: Decl[StaticVar]/CurrNominal: alias1[#FooEnum#]{{; name=.+$}}
-// FOO_ENUM_DOT_INVALID-NEXT: Decl[InstanceMethod]/CurrNominal/TypeRelation[Invalid]: hash({#(self): FooEnum#})[#(into: inout Hasher) -> Void#]{{; name=.+$}}
+// FOO_ENUM_DOT_INVALID-NEXT: Decl[InstanceMethod]/CurrNominal: hash({#(self): FooEnum#})[#(into: inout Hasher) -> Void#]{{; name=.+$}}


This makes FOO_ENUM_DOT_INVALID the same as FOO_ENUM_DOT. Just remove FOO_ENUM_DOT_INVALID and modify the FileCheck argument to use FOO_ENUM_DOT

rintaro · 2020-09-04T19:31:23Z

lib/IDE/CodeCompletion.cpp

+void DotExprLookup::performLookup(ide::CodeCompletionContext &CompletionCtx,
+                                  CodeCompletionConsumer &Consumer,
+                                  bool IsInSelector) const {


It's not clear to me why the logic in this function should be in DotExprLookup especially considering this is declared in Sema. How about DotExprLookup just have an accessor for Solutions and inline this logic in trySolverCompletion?

…nreachable now.

…all refactorings (NFC) Also put subclasses of TypeCheckCompletionCallback into their own header.

…of-date comment (NFC)

nathawes · 2020-09-09T00:06:33Z

@swift-ci clean test

…NFC)

nathawes · 2020-09-09T00:32:42Z

@swift-ci please test

swift-ci · 2020-09-09T00:33:07Z

Build failed
Swift Test Linux Platform
Git Sha - d60116d

swift-ci · 2020-09-09T00:33:23Z

Build failed
Swift Test OS X Platform
Git Sha - d60116d

rintaro · 2020-09-09T05:45:45Z

lib/Parse/ParseExpr.cpp

+static bool isMemberCompletion(ASTNode Node) {
+  struct HasMemberCompletion: public ASTWalker {
+    bool Value = false;
+    std::pair<bool, Expr *> walkToExprPre(Expr *E) override {
+      if (auto *CCE = dyn_cast<CodeCompletionExpr>(E)) {
+        Value = CCE->getBase();
+        return {false, nullptr};
+      }
+      return {true, E};
+    }
+  };
+  HasMemberCompletion Check;
+  Node.walk(Check);
+  return Check.Value;
+}


Could you consolidate this with the one in ParseDecl?

rintaro · 2020-09-09T05:52:57Z

include/swift/Sema/CodeCompletionTypeChecking.h

@@ -0,0 +1,82 @@
+//


Missing the first line?

//===--- CodeCompletionTypeChecking.h --------------------------*- C++ -*-===//

…ParseExpr.cpp and ParseDecl.cpp Also: - propagate the Solution -> Result rename to Solution parameter of deliverDotExprResults - fixup header comment in CodeCompletionTypeChecking.h

nathawes · 2020-09-09T19:22:03Z

@swift-ci please test

swift-ci · 2020-09-09T19:24:40Z

Build failed
Swift Test OS X Platform
Git Sha - 7a06792

swift-ci · 2020-09-09T19:24:48Z

Build failed
Swift Test Linux Platform
Git Sha - 7a06792

nathawes · 2020-09-09T21:16:25Z

@swift-ci please test Linux

rintaro

LGTM

Nathan Hawes and others added 28 commits August 28, 2020 22:24

[CodeCompletion] Pass the CodeCompletionExpr rather than just the bas…

cf60b2f

…e expression to the DotExpr completion callback.

[CodeCompletion] Adjust DotExprLookup::sawSolution to use new `Solu…

a49404d

…tion` APIs

[CSSolver] add debug logging to solveForCodeCompletion

5bd19d2

[CSSolver] Update expression after constraint generation while solvin…

53de10f

…g for code completion

[CodeCompletion] Move CompletionCollector to ASTContext + bug fixes.

3d85615

[CSSolver/CodeCompletion] If pre-check fails extract code completion …

5872cef

…expression and use it If there was an invalid reference which was caught by pre-check, let's remove all context besides code completion itself and use it to produce code completion results.

[CSSolver] Augment solveForCodeCompletion to indicate when it fails

b71fdb5

[TypeChecker/CodeCompletion] Allow `{typeCheck, solve}ForCodeCompleti…

74f0ec5

…on` to operate on target instead of expression Using `SolutionApplicationTarget` make it easier to propage contextual information and avoid mistakes of using incorrect accessors for constraint generation.

[ConstraintSystem] Add Solution::hasType(ASTNode)

7a9c59a

[TypeChecker] Safe all contextual information about completion expres…

c5098cb

…sion Having all information possible makes it much easier to determine when it's appropriate to fallback to type-check code completion expression and whether it's possible to add some more context to it.

[CodeCompletion] Fix bug in logic determining whether void members ar…

8070c07

…e valid.

[Parse] Perform the single expression function body transform for del…

92a2e0d

…ayed parsing as well We were previously only doing it when parsing up front.

[test] Update code completion test with improved type relation result.

03b5296

[CodeCompletion][test] Update test/IDE/complete_exception.swift with …

b4553cd

…improved type relation.

[Sema] TypeCheckNodeAtLoc: strip out the implicit return the parser a…

35a9d68

…dds to single expression bodies for void-returning functions This makes TypeCheckNodeAtLoc match the regular function body typechecking behavior and fixes a crash in test/IDE/complete_value_expr.swift

[test] Add member completion tests where the base has errors, or is a…

9fc3bee

…mbiguous.

[TypeChecker/CodeCompletion] Move hole -> archetype conversion logic …

627a1c0

…into `sawSolution` Instead of exposing archetypes during solution generation, let's do that based on solution data when a type of code completion expression is requested by code completion callback.

[ConstraintSystem] Allow code completion type to be a hole

20f8a37

Code completion expression type could be a hole if and only if there is obosolutely no contextual information available e.g. `let _ = .#^MEMBER^#`

[CodeCompletion] Don't attempt lookup if base type couldn't be determ…

5625751

…ined If base type couldn't be determined (e.g. because base expression is an invalid reference), let's not attempt to do a lookup since it wouldn't produce any useful results anyway.

[CodeCompletion][NFC] Add doc comments and rename symbols for clarity…

491b691

… in the new member completion implementation.

nathawes marked this pull request as ready for review September 3, 2020 23:22

nathawes requested a review from rintaro September 3, 2020 23:22

rintaro reviewed Sep 4, 2020

View reviewed changes

Nathan Hawes added 3 commits September 8, 2020 11:43

[CodeCompletion] Remove old DotExpr completion handling code - it's u…

ee660c6

…nreachable now.

[CodeCompletion] Rename a few types/methods for clarity plus other sm…

9b8fb7d

…all refactorings (NFC) Also put subclasses of TypeCheckCompletionCallback into their own header.

[CodeCompletion] Remove unused forward declaration and fix up an out-…

d60116d

…of-date comment (NFC)

nathawes requested a review from rintaro September 9, 2020 00:09

[CodeCompletion] Remove unused field in CodeCompletionCallbacksImpl (…

7a06792

…NFC)

rintaro reviewed Sep 9, 2020

View reviewed changes

[CodeCompletion] Deduplicate the two isMemberCompletion functions in …

b15c1fd

…ParseExpr.cpp and ParseDecl.cpp Also: - propagate the Solution -> Result rename to Solution parameter of deliverDotExprResults - fixup header comment in CodeCompletionTypeChecking.h

nathawes requested a review from rintaro September 9, 2020 19:22

rintaro approved these changes Sep 10, 2020

View reviewed changes

nathawes merged commit a1ef6e4 into swiftlang:master Sep 10, 2020

nathawes deleted the new-member-completion branch September 10, 2020 01:51

rintaro mentioned this pull request Sep 17, 2020

[CodeCompletion] Calculate type relation when adding type annotation #33978

Merged

nathawes mentioned this pull request Sep 22, 2020

[CodeCompletion] Migrate unresolved member completion to make use of typeCheckForCodeCompletion #34035

Closed

rintaro mentioned this pull request Jul 20, 2021

[CodeCompletion] Allow preChecking an expression twice in code completion #38474

Merged

[CodeCompletion] Update member completion to handle ambiguous and invalid base expressions #33749

[CodeCompletion] Update member completion to handle ambiguous and invalid base expressions #33749

Uh oh!

Conversation

nathawes commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathawes commented Sep 1, 2020

Uh oh!

nathawes commented Sep 1, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

swift-ci commented Sep 3, 2020

Uh oh!

rintaro commented Sep 4, 2020

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!

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!

nathawes Sep 4, 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!

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!

nathawes commented Sep 9, 2020

Uh oh!

nathawes commented Sep 9, 2020

Uh oh!

swift-ci commented Sep 9, 2020

Uh oh!

swift-ci commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathawes commented Sep 9, 2020

Uh oh!

swift-ci commented Sep 9, 2020

Uh oh!

nathawes commented Sep 1, 2020 •

edited

Loading

nathawes Sep 4, 2020 •

edited

Loading