Fix a lot of IntelliSense issues #1799

SeeminglyScience · 2022-05-12T19:23:35Z

Changes:

(each section is it's own commit for easier review)

Mark completion request handler as `Serial`

This stops completion from being cancelled whenever a DidChangeTextDocument notification is sent. So previously, if you typed Get- very quickly, you'd get no results at all. It also lets completion cancel some other expensive requests like code lens.

Note: the one not fantastic down side of this is it makes "missing" code lenses more likely. By missing I mean:

I have not yet figured out a way to tell VSCode to just try again next time it's able. Also we were already getting this sometimes, but this change does make it at least a bit more likely.

Fix tasks never completing if cancelled quickly

We were exiting SynchronousTask.ExecuteSynchronously early if the cancellation was already requested. This was not setting our tcs state correctly, causing the caller to just fall off without warning. This made some things like finally and using disposals never happen.

Make alias discovery possible on idle

The use of Runspace.SessionStateProxy while the pipeline thread is busy (even if you are on the pipeline thread) results in an exception.

This also allows us to cancel GetAliasesAsync. This was really the main reason I was in there, but I also remembered a report from @ninmonkey about an exception here so two birds one stone.

Add space to completion trigger characters

Note: We should be on the look out for feedback about this change and let it sit in preview slightly longer than we'd normally like to. It's possible this may need to be opt in, but I think the entire intellisense experience is now optimized enough to that the behavior is ideal.

Adding space to completion triggers lets us automatically give completion results for parameter values. This also required building in some support for marking completion results as "Incomplete". This means that the client will not cache results when standard identifier characters are typed.

Mainly, this solves the scenario where you type space, get file completion results, and then type -. You expect to then get parameter names, but without marking "Incomplete", it will instead filter the file results.

Also fixed a scenario where you type $script: and on typing the : it would not update to available script scope variables.

Make completion requests cancellable

That way any completion results from custom argument completers can be cancelled if they take too long and the user keeps typing. TabExpansion2 is a callback-like function similar to prompt, partially for customizing tab completion but also for providing a command to get completion results.

In non-remote runspaces, we can also use it as a completion source that we can cancel by way of issuing a pipeline stop request.

andyleejordan · 2022-05-13T20:10:17Z

src/PowerShellEditorServices/Server/PsesLanguageServer.cs

+                    // If PsesCompletionHandler is not marked as serial, then DidChangeTextDocument
+                    // notifications will end up cancelling completion. So quickly typing `Get-`
+                    // would result in no completions.
+                    //
+                    // This also lets completion requests interrupt time consuming background tasks
+                    // like the references code lens.
+                    .WithHandler<PsesCompletionHandler>(
+                        new JsonRpcHandlerOptions() { RequestProcessType = RequestProcessType.Serial })


andyleejordan · 2022-05-13T20:10:28Z

src/PowerShellEditorServices/Services/CodeLens/ICodeLensProvider.cs

        /// <returns>An array of CodeLenses.</returns>
-        CodeLens[] ProvideCodeLenses(ScriptFile scriptFile);
+        CodeLens[] ProvideCodeLenses(ScriptFile scriptFile, CancellationToken cancellationToken);


andyleejordan · 2022-05-13T20:10:56Z

src/PowerShellEditorServices/Services/CodeLens/ReferencesCodeLensProvider.cs

+                    // This async method is pretty dense with synchronous code
+                    // so it's helpful to add some yields.
+                    await Task.Yield();


andyleejordan · 2022-05-13T20:11:26Z

src/PowerShellEditorServices/Services/PowerShell/Execution/SynchronousTask.cs

@@ -35,7 +35,7 @@ internal abstract class SynchronousTask<TResult> : ISynchronousTask
            CancellationToken cancellationToken)
        {
            Logger = logger;
-            _taskCompletionSource = new TaskCompletionSource<TResult>();
+            _taskCompletionSource = new TaskCompletionSource<TResult>(TaskCreationOptions.RunContinuationsAsynchronously);


Can we comment exactly why we're doing this? (Like what did you hit that we have to set it.)

You can kind of think of it like .ConfigureAwait(false) for completion sources. I didn't necessarily need to add it here, it was just a troubleshooting step, but we probably should be adding it everywhere. Related blog post

I can add a comment here but maybe I should just update all the other instances as well? If we go that route would you still prefer a comment over each one?

We don't need to do it in this PR, but thanks for the blog post. I'll make an issue for further investigation.

andyleejordan · 2022-05-13T20:11:54Z

src/PowerShellEditorServices/Services/PowerShell/Utility/CommandHelpers.cs

+                new PSCommand()
+                    .AddCommand("Microsoft.PowerShell.Core\\Get-Command")
+                    .AddParameter("ListImported", true)
+                    .AddParameter("CommandType", CommandTypes.Alias),


Ah, makes sense.

andyleejordan · 2022-05-13T20:12:12Z

src/PowerShellEditorServices/Services/Symbols/SymbolsService.cs

+            CommandHelpers.AliasMap aliases = await CommandHelpers.GetAliasesAsync(
+                _executionService,
+                cancellationToken).ConfigureAwait(false);


🥳 yay for things being cancellable!

andyleejordan · 2022-05-13T20:12:46Z

src/PowerShellEditorServices/Services/Symbols/Vistors/AstOperations.cs

+
+                    // If the current runspace is not out of process, then we call TabExpansion2 so
+                    // that we have the ability to issue pipeline stop requests on cancellation.
+                    if (executionService is PsesInternalHost psesInternalHost


Oof, I would happily remove the IExecutionService interface as it makes us do things like this.

Yeah either the sync task itself needs to reference IExecutionService or we need to strip it out. Or maybe there should be another interface that has state or w/e.

Maybe we hold off on tackling that until we get a source generator that handles the interface definitions for us? 🤷

andyleejordan · 2022-05-13T20:13:18Z

src/PowerShellEditorServices/Services/Symbols/Vistors/AstOperations.cs

+                    // If the current runspace is not out of process, then we call TabExpansion2 so
+                    // that we have the ability to issue pipeline stop requests on cancellation.


andyleejordan · 2022-05-13T20:14:12Z

src/PowerShellEditorServices/Services/TextDocument/Handlers/CompletionHandler.cs

+                if (completionMatch.ResultType is CompletionResultType.Variable
+                    && completionMatch.CompletionText.EndsWith(":"))
+                {
+                    isIncomplete = true;


Nice work 🥳

This stops completion from being cancelled whenever a DidChangeTextDocument notification is sent. Also lets completion cancel some other expensive requests like code lens.

We were exiting SynchronousTask.ExecuteSynchronously early if the cancellation was already requested. This was not setting our tcs state correctly, causing the caller to just fall off without warning.

The use of `Runspace.SessionStateProxy` while the pipeline thread is busy (even if you are **on** the pipeline thread) results in an exception. This also allows us to cancel `GetAliasesAsync`.

- Cancellation tokens are now propagated to all code lens providers. - Provider code that was heavy in synchronous code has some cancellation checks and `Task.Yield` calls to help keep thread pool threads more flexible.

Adding space to completion triggers lets us automatically give completion results for parameter values. This also required building in some support for marking completion results as "Incomplete". This means that the client will not cache results when standard identifier characters are typed. Mainly, this solves the scenario where you type space, get file completion results, and then type `-`. You expect to then get parameter names, but without marking "Incomplete", it will instead filter the file results.

That way any completion results from custom argument completers can be cancelled if they take too long and the user keeps typing.

SeeminglyScience requested a review from a team May 12, 2022 19:23

andyleejordan changed the title ~~Fix a lot of intellisense issues~~ Fix a lot of IntelliSense issues May 12, 2022

andyleejordan approved these changes May 13, 2022

View reviewed changes

andyleejordan mentioned this pull request May 16, 2022

Use TaskCreationOptions.RunContinuationsAsynchronously #1800

Closed

SeeminglyScience added 7 commits May 17, 2022 12:26

Mark completion requests as Serial

5df33bb

This stops completion from being cancelled whenever a DidChangeTextDocument notification is sent. Also lets completion cancel some other expensive requests like code lens.

Fix tasks never completing if cancelled quickly

a991beb

We were exiting SynchronousTask.ExecuteSynchronously early if the cancellation was already requested. This was not setting our tcs state correctly, causing the caller to just fall off without warning.

Make alias discovery possible on idle

591c77e

The use of `Runspace.SessionStateProxy` while the pipeline thread is busy (even if you are **on** the pipeline thread) results in an exception. This also allows us to cancel `GetAliasesAsync`.

Make a lot more things cancellable

bdb8343

- Cancellation tokens are now propagated to all code lens providers. - Provider code that was heavy in synchronous code has some cancellation checks and `Task.Yield` calls to help keep thread pool threads more flexible.

Make completion requests cancellable

f31c1c2

That way any completion results from custom argument completers can be cancelled if they take too long and the user keeps typing.

Fix test and use record instead of tuple

8da0b79

SeeminglyScience force-pushed the fix-intellisense branch from 3f9f9c7 to 8da0b79 Compare May 17, 2022 16:27

SeeminglyScience and others added 2 commits May 17, 2022 13:08

Fix NRE in tests

220dd1a

Merge branch 'master' into fix-intellisense

1199612

andyleejordan merged commit 090448c into PowerShell:master May 17, 2022

andyleejordan added Issue-Enhancement A feature request (enhancement). Area-IntelliSense labels May 17, 2022

andyleejordan mentioned this pull request Jun 22, 2022

PowerShell extension v2022.6.1 is considerably slower than v2022.5.1 when debugging PowerShell/vscode-powershell#4038

Closed

6 tasks

SydneyhSmith mentioned this pull request Jul 7, 2022

Regression tests #1733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a lot of IntelliSense issues #1799

Fix a lot of IntelliSense issues #1799

SeeminglyScience commented May 12, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

SeeminglyScience May 16, 2022

andyleejordan May 16, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

SeeminglyScience May 16, 2022

andyleejordan May 13, 2022

andyleejordan May 13, 2022

		// If the current runspace is not out of process, then we call TabExpansion2 so
		// that we have the ability to issue pipeline stop requests on cancellation.

Fix a lot of IntelliSense issues #1799

Fix a lot of IntelliSense issues #1799

Conversation

SeeminglyScience commented May 12, 2022

Changes:

Mark completion request handler as Serial

Fix tasks never completing if cancelled quickly

Make alias discovery possible on idle

Add space to completion trigger characters

Make completion requests cancellable

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mark completion request handler as `Serial`