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

nathawes · 2021-07-06T07:40:51Z

No description provided.

nathawes · 2021-07-06T07:41:33Z

@swift-ci please test

nathawes · 2021-07-06T15:23:06Z

@swift-ci please test

xedin · 2021-07-06T16:24:06Z

@swift-ci please smoke test

xedin · 2021-07-06T22:32:10Z

@swift-ci please build toolchain macOS

swift-ci · 2021-07-07T03:23:30Z

macOS Toolchain
Download Toolchain
Git Sha - 6ddfa4bc3e60016869d45a2e5d432c8fa7e87d4c

Install command
tar -zxf swift-PR-38266-1047-osx.tar.gz --directory ~/

xedin

Solver changes look good to me, I have left a couple of minor comments inline.

xedin · 2021-07-07T18:41:43Z

lib/Sema/CSSimplify.cpp

+      Type nextArgTy = args[nextArgIdx].getPlainType();
+      if (auto *TVT = nextArgTy->getAs<TypeVariableType>()) {
+        if (TVT->getImpl().isCodeCompletionToken() &&
+            TVT->getASTContext().CompletionCallback)


Maybe remove CompletionCallback and wrap whole block (starting at line 346) into if (cs.isForCodeCompletion()) instead?

matchCallArgumentssImpl doesn't have cs currently. Should I pass it in? I think it'd need to be added to constraints::matchCallArguments and areConservativelyCompatibleArgumentLabels too.

xedin · 2021-07-07T18:43:21Z

lib/Sema/ConstraintSystem.cpp

-    increaseScore(SK_KeyPathSubscript);
+    // ...except if we're solving for code completion and the index expression
+    // contains the completion location
+    auto SE = dyn_cast_or_null<SubscriptExpr>(locator->getAnchor().dyn_cast<Expr*>());


Could be simplified down to getAsExpr<SubscriptExpr>(locator->getAnchor())

nathawes · 2021-07-15T07:47:27Z

@swift-ci please test

bnbarham · 2021-07-16T01:55:45Z

@swift-ci please test

swift-ci · 2021-07-16T03:05:41Z

Build failed
Swift Test Linux Platform
Git Sha - aadb18ba2fc5a210033edb694b94544205c2777a

xedin · 2021-07-16T17:31:21Z

@swift-ci please test Linux platform

swift-ci · 2021-07-16T19:18:26Z

Build failed
Swift Test Linux Platform
Git Sha - aadb18ba2fc5a210033edb694b94544205c2777a

xedin

Solver related changes look good to me! Left a couple of minor comments inline.

xedin · 2021-07-17T00:00:53Z

lib/Sema/CSSimplify.cpp

+}
+
+static bool
+isAfterCompletionToken(Optional<CompletionArgInfo> info, unsigned argIdx) {


Should this be a method on CompletionArgInfo instead?

xedin · 2021-07-17T00:06:35Z

lib/Sema/CSSimplify.cpp

+    // with the same impact as that for the fix to ignore arguments after the
+    // completion position when matching arguments.
+    if (isForCodeCompletion()) {
+      auto getArgIndex = [&](ConstraintLocator *loc) -> Optional<unsigned> {


Could you please make it work on ConstraintLocatorBuilder instead of allocating a new locator to extract index?

xedin · 2021-07-17T00:07:59Z

lib/Sema/CSSimplify.cpp

+        // position once.
+        bool isFirst = llvm::all_of(getFixes(), [&](const ConstraintFix *fix) {
+          auto *fixLocator = fix->getLocator();
+          if (fixLocator->getAnchor() != locator.getAnchor())


I think you'd want to check the kind of fix here as well, otherwise it's possible to find some unrelated fix that has ApplyArgToParam in the path and use it.

ahoppen

Thanks a lot for tackling this @nathawes! I’ve added a few comments/questions regarding the code inline. I haven’t had a chance to look at the test cases yet. I’ll do that hopefully tomorrow.

ahoppen · 2021-07-26T19:04:19Z

include/swift/Sema/CodeCompletionTypeChecking.h

+    /// The type associated with the code completion expression itself.
+    Type ExpectedType;
+    /// The innermost call expression containing the completion location.
+    Expr *CallE;


I think the name CallE is a bit confusing because I would expecting CallE to be a CallExpr but it can also be a SubscriptExpr.

It appears like you are only using it to check if the call is a subscript. Wouldn’t it be easier to just have a boolean flag?

Yeah, that's nicer and sidesteps the naming. Thanks!

ahoppen · 2021-07-26T19:05:15Z

include/swift/Sema/CodeCompletionTypeChecking.h

+    /// The innermost call expression containing the completion location.
+    Expr *CallE;
+    /// The FuncDecl or SubscriptDecl associated with the call.
+    Decl *FuncD;


Just to provide a little bit more context and compile-time safety, I think this could be a ValueDecl instead of a Decl.

ahoppen · 2021-07-26T19:07:27Z

include/swift/Sema/CodeCompletionTypeChecking.h

+    /// True if the completion is a noninitial term in a variadic argument.
+    bool IsNoninitialVariadic;
+    /// The base type of the call.
+    Type BaseType;


If we are completing on a free function, is BaseType a nullptr? I think it might be worth adding documentation for that.

ahoppen · 2021-07-26T19:08:58Z

include/swift/Sema/ConstraintSystem.h

+  Optional<unsigned> firstTrailingIdx;
+  unsigned argCount;
+
+  /// \returns true  if the given argument index is possibly about to be written


Nitpick: Two spaces after true

ahoppen · 2021-07-26T20:25:26Z

lib/Sema/CSSimplify.cpp

+    // missing label. Completion will suggest the label in such cases.
+    if (completionInfo && nextArgIdx < numArgs &&
+        args[nextArgIdx].getLabel().empty()) {
+      if (isCodeCompletionTypeVar(args[nextArgIdx].getPlainType()))


Style: I think you can just merge this condition with the previous if.

lib/Sema/CSSimplify.cpp

lib/Sema/TypeCheckCodeCompletion.cpp

ahoppen · 2021-07-26T21:17:43Z

lib/Sema/TypeCheckCodeCompletion.cpp

+  // solver can't know what kind of keypath is expected (e.g. KeyPath vs
+  // WritableKeyPath) so it ends up as a hole. Just assume KeyPath so we show
+  // the root type to users.
+  if (SelectedOverload->choice.getKind() == OverloadChoiceKind::KeyPathApplication) {


I don’t quite understand when we need this special case. Do you have a test case at hand that fails without it? I haven’t had a chance to look at all of them yet.

This was for an existing test case in IDE/complete_subscript.swift:

let _ = MyStruct<Int>()[#^INSTANCE_INT_BRACKET^# // INSTANCE_INT_BRACKET: Begin completions // INSTANCE_INT_BRACKET-DAG: Decl[Subscript]/CurrNominal/Flair[ArgLabels]: ['[']{#(x): Int#}, {#instance: Int#}[']'][#Int#]; // INSTANCE_INT_BRACKET-DAG: Pattern/CurrNominal/Flair[ArgLabels]: ['[']{#keyPath: KeyPath<MyStruct<Int>, Value>#}[']'][#Value#]; // INSTANCE_INT_BRACKET: End completions }

Without it the second result has no param or result type information (both are UnresolvedType), so it ends up being printed something like the below from memory:

['[']{#keyPath: _#}[']'][#_#];

I'll update the comment (it should say subscript rather than method too).

OK, that makes sense. But wouldn’t the betters approach be that the constraint solver yields two solutions to us. IIUC, we implicitly have the following two subscripts defined on every type

subscript<Value>(keyPath keyPath: KeyPath<Self, Value>) -> Value { get } subscript<Value>(keyPath keyPath: WritableKeyPath<Self, Value>) -> Value { get set }

so I would expect the constraint solver to return two solutions (one for KeyPath and one for WritableKeyPath).

I suspect it's not actually implemented like that, since regular overloaded subscripts do give us two solutions. @xedin do you know how keypath application is set up in the solver at the moment?

Unfortunately solver can't do anything about this. subscript[keyPath: ...] is not a regular overload so there is no parameter information for it, instead it infers whether it's KeyPath or WritableKeyPath based on the argument and in case of code completion it can't do that because there is no argument, so Nathan had to hardcode it to KeyPath to be safe if I remember correctly.

Ah, OK. That’s unfortunate but probably the best solution for now.

lib/IDE/CodeCompletion.cpp

ahoppen · 2021-07-26T21:26:57Z

lib/IDE/CodeCompletion.cpp

+    return true;
+  }
+
+  bool showGlobalCompletions = false;


Style: Capitalize this variable to be consistent.

ahoppen

And as promised, I’ve got a few comments on the test cases.

ahoppen · 2021-07-27T11:51:17Z

test/IDE/complete_ambiguous.swift

+
+  // ARG_NO_LABEL_PREV_WRONG: Begin completions, 2 items
+  // ARG_NO_LABEL_PREV_WRONG-DAG: Pattern/Local/Flair[ArgLabels]:     {#c: String#}[#String#]; name=c: String
+  // ARG_NO_LABEL_PREV_WRONG-DAG: Pattern/Local/Flair[ArgLabels]:     {#d: String#}[#String#]; name=d: String


What’s the rationale for not suggesting b here? I could see an argument for not suggesting it because wrongLabel matches b by positional matching but by that reasoning we also shouldn’t suggest c.

Yeah this seems wrong looking at it again. It's not matching positionally, the b overload is considered worse in the scoring because it fails to find an arg for b after a (and b is required) and then reports wrongLabel and the completion as extraneous arguments. The c:d: overload has nothing missing because c and d are defaulted, so just sees wrongLabel and the completion as extraneous and gets a better score. We don't actually bind any parameter to the completion in either case so it hits my fallback logic (needed for the existing OVERLOAD_LABEL2 case in test/IDE/completion_call_arg.swift) that binds it to the first unfulfilled parameter after the last fulfilled one (if there is one). I think there's a few things to try:

Update the matching logic to somehow skip over the bad label when binding, so there's no missing param, the completion arg is bound in both overloads, and they both just have one extraneous argument (and end up with the same score). This might have some undesirable consequences elsewhere though.

Just give up when we don't get a binding for the completion, giving no results in this case and the existing OVERLOAD_LABEL2 case (regressing it, but perhaps understandably?).

Change the filtering logic to keep all invalid solutions when there are no valid ones, rather than just the best one. From memory that caused some tests to fail, mostly around completion after == because it has tons of overloads and the fix to allow type mismatches made them all viable regardless of the type on the LHS. I suspect it might also give us some nonsense results for solutions with tons of fixes.

Got option 1 working without any other impact.

test/IDE/complete_ambiguous.swift

ahoppen · 2021-07-27T11:56:07Z

test/IDE/complete_call_arg.swift

-    let _ = obj.hasError(a1: #^HASERROR2^#
-    let _ = obj.hasError(a1: IC1, #^HASERROR3^#
+    let _ = obj.hasError(a1: #^HASERROR2?xfail=FIXME^#
+    let _ = obj.hasError(a1: IC1, #^HASERROR3?xfail=FIXME^#


What’s the issue why these need to be xfailed? Is the type checker just bailing out completely because of the invalid type? I think we should at least have a bug report for the xfail.

I think it's a general solver-based completion issue. The lookup the solver does to get the overloads does an isValid() check on the decls it finds and ignores them if they're not. I can change it to let them through when solving for code completion but because it changes what was previously an invariant I wanted to leave that as a separate change if you/Rintaro are ok with the regression in the meantime. I'll file a bug if so, or try to fix it here if not.

I think the regression is OK if it only applies if the function signature contains an error type. Would be something that we should fix at some point but I think it’s OK to do it in another PR. I’d just like a bug report so we don’t forget about it 😉

Awesome, I'm add it in the next push!

test/IDE/complete_call_arg.swift

nathawes · 2021-07-30T04:32:57Z

@swift-ci please smoke test

nathawes · 2021-07-30T04:33:49Z

@swift-ci please build toolchain macOS

bnbarham · 2021-07-30T04:36:42Z

@swift-ci please smoke test

bnbarham · 2021-07-30T04:36:50Z

@swift-ci please build toolchain macOS

ahoppen · 2021-07-30T09:49:26Z

Once the other tests passed, I’d also like to run the stress tester to make sure we aren’t introducing any unknown regressions through this change.

ahoppen · 2021-07-30T09:49:35Z

@swift-ci Please smoke test macOS

swift-ci · 2021-07-30T11:41:15Z

macOS Toolchain
Download Toolchain
Git Sha - 94a817e19a90ee723ec767b153f4264856082888

Install command
tar -zxf swift-PR-38266-1061-osx.tar.gz --directory ~/

ahoppen · 2021-07-30T13:59:08Z

@swift-ci Please SourceKit stress test

ahoppen · 2021-08-02T08:03:15Z

The stress tester found 353 new issues (one unexpected failure in key path completion is known but not in XFail yet). Would you mind taking a look at them? It looks to me like a lot of semantic functionality is failing if the source file contains an uncompleted call.

Note that the stress tester now continues execution on failure. The way I currently reduce stress tester failures is to give the project I’m interested in a new tag and then run the stress tester with the SK_STRESS_FILE_FILTER environment variable set so only the file that’s failing gets stress tested. E.g.

env SK_STRESS_FILE_FILTER='/ItemDetailInfoView.swift' DEVELOPER_DIR='/Applications/Xcode-GPE.app' CODE_SIGN_IDENTITY= CODE_SIGNING_REQUIRED=NO ENTITLEMENTS_REQUIRED=NO ENABLE_BITCODE=NO INDEX_ENABLE_DATA_STORE=NO GCC_TREAT_WARNINGS_AS_ERRORS=NO SWIFT_TREAT_WARNINGS_AS_ERRORS=NO ./run_sk_stress_test --filter-by-tag 'achn' --swiftc ~/swift-src/nbuild/toolchains/Release+Asserts/usr/local/bin/swiftc --skip-tools-clone --skip-tools-build main --add-xcodebuild-flags 'ARCHS=arm64 "OTHER_SWIFT_FLAGS=$(OTHER_SWIFT_FLAGS) -swift-version 5 -disallow-use-new-driver"'

From there I extract the build argument and run sourcekitd-test with the offset and request that’s failing.

nathawes · 2021-08-04T05:09:13Z

@swift-ci please test

bnbarham · 2021-08-04T05:10:46Z

@swift-ci please test

bnbarham · 2021-08-04T05:12:58Z

@swift-ci please SourceKit stress test

ahoppen · 2021-08-06T07:18:12Z

Nice, this fixes three big-ish XFails in the stress tester 🥳 Could you also add their associated test cases and mark them as fixed on bugs.swift.org once this PR is merged? (SR-14693, SR-14705, SR-14888)

Still a couple of XFails though. The padding ones reminded me of SR-14692, not sure if the issues are are related though.

ahoppen · 2021-08-06T20:49:43Z

I just took a look at the code completion performance measurements taken by the stress tester and it looks like there are some notable regressions. E.g. AppIconPickerView.swift at offset 1752 regressed by ~300x from 46e6 (ca. 7ms) to 15e9 (ca. 2 sec) instructions. All regressions appear to be inside result builders though, so this might be general badness of code completion inside result builders. Still, it might be worth investigating if there is an easy fix for the performance regressions.

…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).

nathawes · 2021-08-25T23:45:55Z

@swift-ci please test

bnbarham · 2021-08-25T23:47:01Z

@swift-ci please test

bnbarham · 2021-08-26T00:08:49Z

@swift-ci please SourceKit stress test

bnbarham · 2021-08-26T00:09:17Z

@swift-ci please build toolchain macOS

swift-ci · 2021-08-26T05:09:34Z

macOS Toolchain
Download Toolchain
Git Sha - 85fea89

Install command
tar -zxf swift-PR-38266-1095-osx.tar.gz --directory ~/

ahoppen · 2021-08-31T09:46:15Z

There are still 56 stress tester failures 😬

nathawes marked this pull request as draft July 6, 2021 07:41

nathawes force-pushed the migrate-arg-position-completion-to-solver branch from 4a2d8c4 to 6ddfa4b Compare July 6, 2021 15:17

nathawes marked this pull request as ready for review July 7, 2021 00:33

nathawes marked this pull request as draft July 7, 2021 00:33

nathawes marked this pull request as ready for review July 7, 2021 02:17

xedin approved these changes Jul 7, 2021

View reviewed changes

nathawes requested a review from xedin July 16, 2021 01:52

xedin approved these changes Jul 17, 2021

View reviewed changes

ahoppen reviewed Jul 26, 2021

View reviewed changes

ahoppen reviewed Jul 27, 2021

View reviewed changes

nathawes force-pushed the migrate-arg-position-completion-to-solver branch from aadb18b to 94a817e Compare July 30, 2021 04:23

nathawes force-pushed the migrate-arg-position-completion-to-solver branch 2 times, most recently from 253d15e to b60add0 Compare August 4, 2021 05:08

nathawes added 7 commits August 26, 2021 08:44

test fixes after rebase

24b22dc

fixups

af73580

more fixes

d83bff8

formatting

6eafb6e

add subscript tests + stress tester assertion hit fixes

3808cb1

fix two more issues found by the stress tester

85fea89

nathawes force-pushed the migrate-arg-position-completion-to-solver branch from b60add0 to 85fea89 Compare August 25, 2021 23:45

ahoppen mentioned this pull request Sep 20, 2021

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

Merged

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

nathawes commented Jul 6, 2021

Uh oh!

nathawes commented Jul 6, 2021

Uh oh!

nathawes commented Jul 6, 2021

Uh oh!

xedin commented Jul 6, 2021

Uh oh!

xedin commented Jul 6, 2021

Uh oh!

swift-ci commented Jul 7, 2021

Uh oh!

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

nathawes commented Jul 15, 2021

Uh oh!

bnbarham commented Jul 16, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

xedin commented Jul 16, 2021

Uh oh!

swift-ci commented Jul 16, 2021

Uh oh!

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

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

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

nathawes Jul 29, 2021 •

edited

Loading