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

ahoppen · 2021-09-20T12:14:24Z

Revised version of #38266 with most stress tester failures fixed.

CC @nathawes

rdar://83435550

hamishknight

Cool! Hope you don't mind some drive-by nits :)

lib/IDE/CodeCompletion.cpp

lib/Sema/CSSimplify.cpp

lib/Sema/TypeCheckCodeCompletion.cpp

ahoppen · 2021-09-22T09:59:54Z

Thanks @hamishknight for the suggestions for the new ArgumentList API. I just rebased it so it worked and didn’t look if the code could be improved.

For now, I’m looking to merge the changes to solution scoring (which are needed to fix the stress tester issues) here: #39392

ahoppen · 2022-03-03T23:00:06Z

lib/Sema/TypeCheckCodeCompletion.cpp

+      // We don't care how much source code was ignored after the code
+      // completion token, just whether if there was any.
+      if (S.getFixedScore().Data[SK_IgnoreAfterCompletionToken] > 0 &&
+          minScore.Data[SK_IgnoreAfterCompletionToken] == 0) {


I am not really happy with this part yet. What I am really trying to achieve is to have a boolean flag in the score that indicates whether code after the code completion token was ignored – I don’t care how much. At the moment we might end up with solutions that have a SK_IgnoreAfterCompletionToken score of >1 (sometimes also created by combining two partial solutions, where each partial solution ignored arguments).

In the current implementation, we might filter out solutions with a SK_IgnoreAfterCompletionToken of 2 when comparing against a solution that has a SK_IgnoreAfterCompletionToken of 1 in the constraint system, even though we don’t really consider it worse.

Options that I have considered:

Only increase score in recordFix if SK_IgnoreAfterCompletionToken is 0 — doesn’t work because we might combine two partial solutions that both have SK_IgnoreAfterCompletionToken = 1

Hacking the operator< of Score to consider all SK_IgnoreAfterCompletionToken > 0 equally bad — this seems very surprising to me

Hack the operator+ of Score such that 1 + 1 = 1 for SK_IgnoreAfterCompletionToken — this makes Score addition non-linear, not sure what effects it might have

Leave it as-is for now because we don’t have any counter-examples that break yet

ahoppen · 2022-03-03T23:01:41Z

test/IDE/complete_call_arg.swift

+    var bar: Binding<MyStruct>
+
+    func test(index: Int) {
+      _ = bar[#^DYNAMIC_MEMBER_SUBSCRIPT_LOOKUP?xfail=FIXME^#index]


This is the test case for the

// FIXME: We currently dan't look through @dynamicMemberLookup applications // for subscripts

comment in ArgumentCompletion.cpp. I’ll file a bug report for this one and fix it in a follow-up PR.

xedin · 2022-03-04T05:22:49Z

include/swift/Sema/ConstraintSystem.h

@@ -126,6 +126,9 @@ class SavedTypeVariableBinding {
  /// The parent or fixed type.
  llvm::PointerUnion<TypeVariableType *, TypeBase *> ParentOrFixed;

+  /// The locator associated with bound type
+  ConstraintLocator *BoundLocator;


Probably Binding in this case.

xedin · 2022-03-04T05:24:17Z

include/swift/Sema/ConstraintSystem.h

@@ -486,14 +503,15 @@ class TypeVariableType::Implementation {
  }

  /// Assign a fixed type to this equivalence class.
-  void assignFixedType(Type type,
+  void assignFixedType(Type type, constraints::ConstraintLocator *loc,


Please change a name here to bindingLocation or something like that and add a comment about it.

xedin · 2022-03-04T05:37:13Z

lib/Sema/ConstraintSystem.cpp

+/// the types we do know instead of bailing out completely because \p type
+/// contains an error type.
+static Type replaceParamErrorTypeByPlaceholder(Type type, ValueDecl *value) {
+  if (auto funcType = type->getAs<AnyFunctionType>()) {


Minor stylistic comment - it's preferred not to nest code like this, if you could make an early return - prefer it.

xedin · 2022-03-04T05:39:04Z

lib/Sema/ConstraintSystem.cpp

+      assert(declParams->size() == typeParams.size());
+      SmallVector<AnyFunctionType::Param, 4> newParams;
+      newParams.reserve(declParams->size());
+      for (size_t i = 0; i < declParams->size(); i++) {


I know that there is an assert that declParams have the same size as typeParams but it's still kind of weird to use declParams here. This could be simplified down to for (auto i : indices(typeParams))

xedin · 2022-03-04T05:44:47Z

include/swift/Sema/ConstraintSystem.h

@@ -260,6 +263,10 @@ class TypeVariableType::Implementation {
  /// type is bound.
  llvm::PointerUnion<TypeVariableType *, TypeBase *> ParentOrFixed;

+  /// The locator describing where the type bound to this type variable
+  /// originated.
+  constraints::ConstraintLocator *BoundLocator = nullptr;


xedin · 2022-03-04T05:57:44Z

lib/Sema/CSSimplify.cpp

+        // Don't allow arguments after the completion position to be claimed
+        // during recovery. We ignore them if they are invalid, as the user may
+        // may be re-writing the callsite rather than updating an argument.
+        if (completionInfo && completionInfo->isBefore(nextArgIdx))


This is not a comment about this particular place but a general pattern here and other places in CSSimply.

Reading through the code in CSSimplify here makes me wonder - why don't we just add a new matchCallArgumentForCodeCompletion method instead? It seems like the rules here are pretty straight-forward - positional matches, with ignoring failures after the code completion token, out-of-order matching is disabled for the "after code completion token" (or should it be even disabled outright?), some fixes are ignored...

Another solution to consider - while generating constraints for a call we could produce a special applicable fn with code completion constraint if one of the arguments has a code completion token, this way we can control the "argument function", and potentially generate type variables on demand after matching parameters to arguments based on code completion rules.

I'm hoping that this would allow us to avoid IgnoreFailureAfterCompletionArg and new score kind.

I was able to migrate a lot of this logic into a new argument match listener for code completion and was able to use SK_Fix instead of the new score kind. I don’t think we can get rid of IgnoreFailureAfterCompletionArg though, because we need to have some way to detect whether we already ignored arguments for a given call.

I think we shouldn't use fixes for this purpose, instead we should add new storage to a constraint system which is going to keep track of that.

xedin · 2022-03-04T20:35:24Z

lib/Sema/CSSimplify.cpp

@@ -7631,14 +7783,14 @@ static bool isForKeyPathSubscript(ConstraintSystem &cs,
  return false;
 }

-static bool isForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,
-                                              ConstraintLocator *locator) {
+static bool mayBeForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,


Nit: lower-case b here.

I think the capital b is correct here as it’s “may be for key path subscript without label” where “be” is a verb.

Okay, makes sense, <locator> may be for ... definitely makes sense.

ahoppen · 2022-03-16T11:47:28Z

After some discussion with @xedin, we have decided to always ignore arguments after the code completion token because it defines away an entire family of ambiguities where it’s unclear whether these arguments should influence what’s suggested for the code completion token because we can’t know whether the user wants to rewrite the entire call site or only update one parameter. We might be able to revisit this decision in a follow-up PR. As a side-effect, it also simplifies this PR.

ahoppen · 2022-03-16T22:18:38Z

@swift-ci Please smoke test

ahoppen · 2022-03-16T22:18:42Z

@swift-ci Please SourceKit stress test

xedin

Solver related changes look good!

include/swift/Sema/ConstraintSystem.h

lib/Sema/CSGen.cpp

lib/Sema/CSSimplify.cpp

xedin · 2022-03-17T00:14:42Z

lib/Sema/CSSimplify.cpp

@@ -7631,14 +7783,14 @@ static bool isForKeyPathSubscript(ConstraintSystem &cs,
  return false;
 }

-static bool isForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,
-                                              ConstraintLocator *locator) {
+static bool mayBeForKeyPathSubscriptWithoutLabel(ConstraintSystem &cs,


Okay, makes sense, <locator> may be for ... definitely makes sense.

…a code completion token

…he solver-based implementation This hooks up call argument position completion to the typeCheckForCodeCompletion API to generate completions from all the solutions the constraint solver produces (even those requiring fixes), rather than relying on a single solution being applied to the AST (if any). Co-authored-by: Nathan Hawes <[email protected]>

ahoppen · 2022-03-17T14:17:12Z

@swift-ci Please smoke test

ahoppen · 2022-03-17T14:17:20Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-03-17T14:22:22Z

I addressed your review comments @xedin and made the following changes:

Don’t optimize constraints of arguments that are being ignored: CSGen.cpp:791
Extracted logic whether a argument should be claimed despite label mismatch (because it is the completion expression) to CompletionArgumentTracker (CSSimplify.cpp:1295). This makes sure that we only claim arguments despite label mismatches when we are performing solver-based completion. It broke a couple of test cases in the legacy code completion.

xedin

LGTM! We have discussed this offline but I'm just going to leave it here - the storage for ignored arguments would be switched to use Expr * and check in ConstraintWalker @ CSGen.cpp:791 would have to get wrapped into if (cs.isForCodeCompletion()) in a follow-up.

ahoppen requested a review from nathawes September 20, 2021 12:14

hamishknight reviewed Sep 20, 2021

View reviewed changes

ahoppen force-pushed the pr/solver-based-arg-completion branch 3 times, most recently from a925c49 to 556334b Compare February 11, 2022 11:37

ahoppen force-pushed the pr/solver-based-arg-completion branch 5 times, most recently from ea7ed3d to 54a9ce4 Compare March 2, 2022 12:07

ahoppen changed the title ~~[WIP][CodeCompletion] Migrate argument position completion to the solver-based implementation~~ [CodeCompletion] Migrate argument position completion to the solver-based implementation Mar 2, 2022

ahoppen force-pushed the pr/solver-based-arg-completion branch 2 times, most recently from 952284f to c498b53 Compare March 3, 2022 22:15

ahoppen marked this pull request as ready for review March 3, 2022 22:52

ahoppen commented Mar 3, 2022

View reviewed changes

ahoppen requested review from xedin and rintaro March 3, 2022 23:01

ahoppen mentioned this pull request Mar 4, 2022

[CodeCompletion] Migrate expression completions to solver-based #41633

Merged

xedin requested changes Mar 4, 2022

View reviewed changes

ahoppen force-pushed the pr/solver-based-arg-completion branch 3 times, most recently from 5359ac0 to 42cf5b2 Compare March 16, 2022 11:44

ahoppen force-pushed the pr/solver-based-arg-completion branch from 42cf5b2 to ca504b5 Compare March 16, 2022 22:18

xedin approved these changes Mar 17, 2022

View reviewed changes

[TypeChecker] Add method to check whether a type variable represents …

ca9056a

…a code completion token

ahoppen force-pushed the pr/solver-based-arg-completion branch from ca504b5 to f538d33 Compare March 17, 2022 14:16

xedin approved these changes Mar 17, 2022

View reviewed changes

ahoppen merged commit 17ac201 into swiftlang:main Mar 17, 2022

ahoppen deleted the pr/solver-based-arg-completion branch March 17, 2022 21:42

ahoppen mentioned this pull request Mar 17, 2022

[Stress tester XFails] Update XFails swiftlang/swift-source-compat-suite#647

Merged

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

hamishknight left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Sep 22, 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!

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!

ahoppen commented Mar 16, 2022

Uh oh!

ahoppen commented Mar 16, 2022

Uh oh!

ahoppen commented Mar 16, 2022

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Mar 17, 2022

Uh oh!

ahoppen commented Mar 17, 2022

Uh oh!

ahoppen commented Mar 17, 2022

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Sep 20, 2021 •

edited

Loading