[CodeCompletion] Refactor how code completion results are returned to support cancellation #39631

ahoppen · 2021-10-07T17:07:50Z

This refactors a bunch of code-completion methods around performOperation to return their results via a callback only instead of the current mixed approach of indicating failure via a return value, returning an error string as an inout parameter and success results via a callback. The new guarantee should be that the callback is always called exactly once on control flow graph.

ahoppen · 2021-10-08T10:52:43Z

The previous version of this PR made code completion return an error if no code completion token was found, instead of returning empty results. It turns out that the stress tester expected empty results in that case and I’m worried changing the semantics will break clients. So I’m reverting to the original behavior of returning an empty result list in that case.

ahoppen · 2021-10-11T13:56:03Z

@swift-ci Please smoke test

ahoppen · 2021-10-13T11:34:02Z

Based on some discussion with @rintaro, we agreed that the previous version of this PR was not ideal because it exposed implementation details of libIDE, such as handleResultsAndModules to SourceKit.

I went a step further now and pushed the fact that the Callback of CompletionInstance::performOperation needs to invoke the code completion second pass into CompletionInstance. (CompletionInstance::performOperation is now private). Instead, we have dedicated methods on CompletionInstance for code completion, type context info and conforming methods.
I really like the new implementation because SourceKit is now really just converting results and not driving the completion instance anymore. The async return nature has a slight tendency of nested completion blocks but overall I think the control flow is a lot cleaner with fewer implicit dependencies.

This also moves the Printing*Consumer from libIDE to print methods in swift-ide-test, which was the only user anyway.

ahoppen · 2021-10-14T16:42:34Z

@swift-ci Please smoke test

rintaro · 2021-10-08T21:20:45Z

include/swift/IDE/CancellableResult.h

Could you explain why not Result = Other.Result?

It’s something to do with deleted copy constructor. IIUC we don’t want to destruct the Result in the union because it’s just uninitialized memory and it’s copy contractor might be deleted. I copied the implementation from llvm::Optional

https://github.com/apple/llvm-project/blob/e67e2a8d4c65aaa2fa0e41a32630ab5961e0851a/llvm/include/llvm/ADT/Optional.h#L112

rintaro · 2021-10-13T17:27:48Z

tools/SourceKit/lib/SwiftLang/SwiftConformingMethodList.cpp

Suggested change

void deliverResults(SourceKit::ConformingMethodListConsumer &SKConsumer,

static void deliverResults(SourceKit::ConformingMethodListConsumer &SKConsumer,

rintaro · 2021-10-15T05:15:31Z

tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp

rintaro · 2021-10-15T05:19:12Z

tools/SourceKit/lib/SwiftLang/SwiftCompletion.cpp

Suggested change

void deliverCodeCompleteResults(SourceKit::CodeCompletionConsumer &SKConsumer,

static void deliverCodeCompleteResults(SourceKit::CodeCompletionConsumer &SKConsumer,

rintaro · 2021-10-15T05:19:47Z

tools/SourceKit/lib/SwiftLang/SwiftTypeContextInfo.cpp

Suggested change

void deliverResults(SourceKit::TypeContextInfoConsumer &SKConsumer,

static void deliverResults(SourceKit::TypeContextInfoConsumer &SKConsumer,

ahoppen · 2021-10-26T13:31:55Z

@swift-ci Please smoke test

… support cancellation This refactors a bunch of code-completion methods around `performOperation` to return their results via a callback only instead of the current mixed approach of indicating failure via a return value, returning an error string as an inout parameter and success results via a callback. The new guarantee should be that the callback is always called exactly once on control flow graph. Other than a support for passing the (currently unused) cancelled state through the different instance, there should be no functionality change.

…ation We had some situations left that neither returned an error, nor called the callback with results in `performOperation`. Return an error in these and adjust the tests to correctly match the error.

…ntextInfo from SoruceKit to CompletionInstance The invocation of the code completion second pass should be implementation detail of `CompletionInstance`. Create a method on `CompletionInstance` that correctly invokes the second pass and just reutnrs the type context info results to the caller.

…mingMethodList from SoruceKit to CompletionInstance

…ompletion from SoruceKit to CompletionInstance

All users of `performCodeCompletionLikeOperation` have been migrated to dedicated methods on `CompletionInstance`.

…onInstance instead of generic performOperation We are migrating all users of `performOperation` to dedicated methods on `CodeCompletionInstance`. Do the same in `swift-ide-test`.

…pletionInstance instead of generic performOperation

…onInstance instead of generic performOperation

All users of `CompletionInstance::performOperation` have been migrated to dedicated methods. `performOperation` with its callback that needs to invoke the second pass is now an implementation detail of `CompletionInstance`.

ahoppen · 2021-10-28T09:11:31Z

Resolved a memory error in CancellableResult that occurred on Linux.

@swift-ci Please smoke test

ahoppen · 2021-10-29T10:03:23Z

@swift-ci Please smoke test

hamishknight · 2021-10-29T12:05:50Z

lib/IDE/CompletionInstance.cpp

    // If we're expecting a standard library, but there either isn't one, or it
    // failed to load, let's bail early and hand back an empty completion
    // result to avoid any downstream crashes.
-    if (CI.loadStdlibIfNeeded())
-      return true;
+    if (CI.loadStdlibIfNeeded()) {
+      Callback(CancellableResult<CompletionInstanceResult>::failure(
+          "failed to load the standard library"));
+      return;
+    }


Should the comment be updated here?

FWIW I think this was the only client that was handling a stdlib load failure different to other setup failures, we may be able to call loadStdlibIfNeeded from within CompilerInstance::setup now, which would probably help catch more cases of crashing when the stdlib fails to load.

Doing a quick search for loadStdlibIfNeeded, there are a couple more places that have specific handling for it and some of them don't seem to call CompilerInstance::setup at all. Unifying those might be a good task for another PR but I think it’s out of scope for this one.

ahoppen requested a review from rintaro October 7, 2021 17:07

ahoppen force-pushed the pr/cancel-completion-infrastructure branch 2 times, most recently from a729d2c to 81d3ca4 Compare October 8, 2021 10:50

swiftlang deleted a comment from swift-ci Oct 11, 2021

ahoppen force-pushed the pr/cancel-completion-infrastructure branch from 81d3ca4 to a2ec632 Compare October 11, 2021 13:55

ahoppen force-pushed the pr/cancel-completion-infrastructure branch from a2ec632 to 24a13ea Compare October 13, 2021 11:24

ahoppen force-pushed the pr/cancel-completion-infrastructure branch from 24a13ea to 82bc6b2 Compare October 14, 2021 16:41

rintaro reviewed Oct 15, 2021

View reviewed changes

ahoppen added 11 commits October 28, 2021 11:10

[CodeCompletion] Make sure callback is always called from performOper…

b6e03e3

…ation We had some situations left that neither returned an error, nor called the callback with results in `performOperation`. Return an error in these and adjust the tests to correctly match the error.

[SourceKit] Move invocation of code completion second pass for Confor…

367c981

…mingMethodList from SoruceKit to CompletionInstance

[SourceKit] Move invocation of code completion second pass for code c…

163ccf9

…ompletion from SoruceKit to CompletionInstance

[SourceKit] Remove performCompletionLikeOperation

70e3c99

All users of `performCodeCompletionLikeOperation` have been migrated to dedicated methods on `CompletionInstance`.

[swift-ide-test] Use dedicated method for typeContextInfo on Completi…

4ee9b0d

…onInstance instead of generic performOperation We are migrating all users of `performOperation` to dedicated methods on `CodeCompletionInstance`. Do the same in `swift-ide-test`.

[swift-ide-test] Use dedicated method for conformingMethodList on Com…

974829e

…pletionInstance instead of generic performOperation

[swift-ide-test] Use dedicated method for code completion on Completi…

76f2dbe

…onInstance instead of generic performOperation

[SourceKit] Make CompletionInstance::performOperation private

6b66d58

All users of `CompletionInstance::performOperation` have been migrated to dedicated methods. `performOperation` with its callback that needs to invoke the second pass is now an implementation detail of `CompletionInstance`.

[SourceKit] Make completion-like helper functions static

95ae8a4

ahoppen force-pushed the pr/cancel-completion-infrastructure branch from 540bef8 to 95ae8a4 Compare October 28, 2021 09:11

[SourceKit] Pass CompletionContext by reference to CompletionInstance

c9f5331

ahoppen force-pushed the pr/cancel-completion-infrastructure branch from ee77b3f to c9f5331 Compare October 29, 2021 10:00

hamishknight reviewed Oct 29, 2021

View reviewed changes

ahoppen merged commit 86a1bfd into swiftlang:main Nov 9, 2021

ahoppen deleted the pr/cancel-completion-infrastructure branch November 9, 2021 12:35

ahoppen mentioned this pull request Nov 9, 2021

[Frontend] Load standard libarary in CompilerInstance::setup #40107

Merged

	void deliverResults(SourceKit::ConformingMethodListConsumer &SKConsumer,
	static void deliverResults(SourceKit::ConformingMethodListConsumer &SKConsumer,

	void deliverCodeCompleteResults(SourceKit::CodeCompletionConsumer &SKConsumer,
	static void deliverCodeCompleteResults(SourceKit::CodeCompletionConsumer &SKConsumer,

	void deliverResults(SourceKit::TypeContextInfoConsumer &SKConsumer,
	static void deliverResults(SourceKit::TypeContextInfoConsumer &SKConsumer,

[CodeCompletion] Refactor how code completion results are returned to support cancellation #39631

[CodeCompletion] Refactor how code completion results are returned to support cancellation #39631

Uh oh!

Conversation

ahoppen commented Oct 7, 2021

Uh oh!

ahoppen commented Oct 8, 2021

Uh oh!

ahoppen commented Oct 11, 2021

Uh oh!

ahoppen commented Oct 13, 2021

Uh oh!

ahoppen commented Oct 14, 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!

ahoppen commented Oct 26, 2021

Uh oh!

ahoppen commented Oct 28, 2021

Uh oh!

ahoppen commented Oct 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants