Refactor brace completion to split into Features vs EditorFeatures #48468

dibarbet · 2020-10-09T07:24:18Z

Commit at a time is recommended, though the first commit is the bulk of the work :(

The first commit moves brace completion implementations from IEditorBraceCompletionSession to a new IBraceCompletionService that is available in the features layer (instead of editor features). This allows us to use it in LSP.
There were ~3 main issues with the existing impl

All the IEditorBraceCompletionSession implementations start off with editor concepts (text buffers, snapshots, IBraceCompletionSession) - So I can't really use it in LSP. Luckily in most of the cases (like the above) they just need some info that can be found from the document as well. This enables nullable and makes things async where possible.
The API shape for IEditorBraceCompletionSession isn't very LSP friendly - the AfterStart and AfterReturn methods apply the changes to the workspace rather than giving back edits. Additionally there's no way to return the actual brace completions or check if the caret is in a valid brace completion location.

The second commit uses the implementation to format braces after completion in razor via OnAutoInsert. We will eventually want to use LSP for brace completion in liveshare/codespaces C# files (since we require semantics for <>), but since they don't yet support over-typing, regular .cs files will use the same local implementation that they do now (in liveshare and codespaces).

Additionally, we use OnAutoInsert instead of OnTypeFormatting since OnAutoInsert supports snippets (moving the caret location). I previously spoke with @NTaylorMullen and the 'solution' for having OnTypeFormatting support snippets was OnAutoInsert.

@ajaybhargavb @NTaylorMullen there will be work on your side I believe to consume onautoinsert for this.

Roslyn.sln

src/EditorFeatures/CSharp/AutomaticCompletion/CSharpBraceCompletionSessionProvider.cs

dibarbet · 2020-10-27T01:28:55Z

src/Features/CSharp/Portable/BraceCompletion/CurlyBraceCompletionService.cs

+            return ImmutableArray.Create(BraceCompletionFormattingRule.ForIndentStyle(indentStyle)).AddRange(Formatter.GetDefaultFormattingRules(document));
+        }
+
+        private sealed class BraceCompletionFormattingRule : BaseFormattingRule


this type was just moved from the original curlybracecompletionsession

dibarbet · 2020-10-27T01:38:01Z

@CyrusNajmabadi I think I'm ready for this to be looked at now - happy to hear your thoughts! I put the overall ideas in the pr description

davidwengier

The LSP part of this makes sense to me (2nd commit). I don't feel too qualified to
comment on the first commit too much :P

dibarbet · 2020-10-27T17:56:28Z

src/EditorFeatures/CSharp/AutomaticCompletion/CSharpBraceCompletionSessionProvider.cs

-            _undoManager = undoManager;
-        }
-
-        protected override bool IsSupportedOpeningBrace(char openingBrace)


code in here was moved to IBraceCompletionService.IsValidForBraceCompletion

dibarbet · 2020-10-27T17:58:12Z

...ures/Core/Implementation/AutomaticCompletion/Sessions/AbstractTokenBraceCompletionSession.cs

-
-namespace Microsoft.CodeAnalysis.Editor.Implementation.AutomaticCompletion.Sessions
-{
-    internal abstract class AbstractTokenBraceCompletionSession : IEditorBraceCompletionSession


moved to AbstractTokenBraceCompletionService

dibarbet · 2020-10-27T17:59:14Z

...torFeatures/Core/Implementation/AutomaticCompletion/Sessions/EditorBraceCompletionSession.cs

+            _undoHistoryRegistry = undoHistoryRegistry;
+        }
+
+        public virtual void AfterReturn(IBraceCompletionSession session, CancellationToken cancellationToken)


this is code originally from CurlyBraceCompletionSession.AfterReturn, but heavily modified to generically apply any IBraceCompletionService.GetTextChangesAfterReturn

Currently the only implementation though is still CurlyBraceCompletionService

dibarbet · 2020-10-27T18:00:43Z

src/Features/CSharp/Portable/BraceCompletion/CurlyBraceCompletionService.cs

+                && await CheckClosingTokenKindAsync(context.Document, context.ClosingPoint, cancellationToken).ConfigureAwait(false);
+        }
+
+        public override async Task<BraceCompletionResult?> GetBraceCompletionAsync(BraceCompletionContext braceCompletionContext, CancellationToken cancellationToken)


this code implements the CurlyBraceCompletionSession.AfterStart functionality (along with providing the actual closing brace edit)

dibarbet · 2020-10-27T18:01:20Z

src/Features/CSharp/Portable/BraceCompletion/CurlyBraceCompletionService.cs

+            }
+        }
+
+        public override async Task<BraceCompletionResult?> GetTextChangeAfterReturnAsync(BraceCompletionContext context, CancellationToken cancellationToken, bool supportsVirtualSpace = true)


this comes from CurlyBraceCompletionSession.AfterStart, with modifications to use documents and supporting non-virtual spaces for the caret location

dibarbet · 2020-10-27T18:03:13Z

src/Features/CSharp/Portable/BraceCompletion/CurlyBraceCompletionService.cs

+            return Collapse(newDocumentText, changes.ToImmutableArray());
+        }
+
+        private static async Task<SourceText> FormatTrackingSpanAsync(Document document, int openingPoint, int closingPoint, bool shouldHonorAutoFormattingOnCloseBraceOption, CancellationToken cancellationToken, ImmutableArray<AbstractFormattingRule>? rules = null)


this is from the original method in CurlyBraceCompletionSession.FormatTrackingSpan, with extra logic added to apply the formatting to the document instead of editor subject buffer at the end.

dibarbet · 2020-10-27T18:03:49Z

src/Features/Core/Portable/BraceCompletion/AbstractTokenBraceCompletionService.cs

+
+namespace Microsoft.CodeAnalysis.BraceCompletion
+{
+    internal abstract class AbstractTokenBraceCompletionService : AbstractBraceCompletionService


from abstracttokenbracecompletionsession

dibarbet · 2020-10-27T18:04:05Z

src/Features/Core/Portable/BraceCompletion/BraceCompletionOptions.cs

+{
+    internal class BraceCompletionOptions
+    {
+        public static readonly PerLanguageOption2<bool> AutoFormattingOnCloseBrace = new(


Had to move these options down to the features layer

dibarbet · 2020-10-27T18:05:42Z

src/Features/VisualBasic/Portable/BraceCompletion/BracketBraceCompletionService.vb

+Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
+
+<Export(LanguageNames.VisualBasic, GetType(IBraceCompletionService)), [Shared]>
+Friend Class BracketBraceCompletionService


Pretty much all the VB implementation was just a straight move.

src/Features/CSharp/Portable/BraceCompletion/CurlyBraceCompletionService.cs

CyrusNajmabadi · 2020-12-03T22:22:45Z

src/Features/CSharp/Portable/BraceCompletion/InterpolationBraceCompletionService.cs

+
+            // We can be starting an interpolation expression if we're inside an interpolated string.
+            return token.Parent.IsKind(SyntaxKind.InterpolatedStringExpression) || token.Parent.IsParentKind(SyntaxKind.InterpolatedStringExpression);
+        }


as before, i don't understand most of this. so primary question is if this is tested well. if so, i'm ok.

src/Features/CSharp/Portable/BraceCompletion/ParenthesisBraceCompletionService.cs

src/Features/Core/Portable/BraceCompletion/AbstractBraceCompletionService.cs

src/Features/Core/Portable/BraceCompletion/IBraceCompletionService.cs

src/Features/Core/Portable/Shared/Extensions/DocumentExtensions.cs

CyrusNajmabadi

LGTM. handle nits as you see fit.

please confirm appropriate testing of some of hte hairier sections of code. if not covered well, please beef things up :)

dibarbet · 2020-12-04T21:07:02Z

@dotnet/roslyn-compiler / @RikkiGibson Need a review on the compiler changes in 4429701 (should include all compiler changes).

This extracts out the textchangerange merging logic into a shared helper. Normally the merging of text changes between versions is handled internally in SourceText (via ChangedText). However, the documents in the IDE are based on ChangedSourceText which is a custom implementation that uses VS editor snapshots to merge text changes between versions. Unfortunately I need to merge a set of text changes originally created from a VS editor snapshot, but the intermediate source text versions do not have corresponding VS editor snapshots. So ChangedSourceText is not able to accurately compute the merged text changes and always returns the whole file as the diff.

To resolve this, I extracted out the merge logic from ChangedText so that I could manually merge the text changes between versions myself. I did not want to copy the VS snapshot text into a new SourceText either as that could be expensive. Note the changes to be merged are relatively simple - a set of formatting changes around a span being merged into a text change that only inserts a new line.

RikkiGibson · 2020-12-04T21:31:12Z

src/Compilers/Core/Portable/InternalUtilities/TextChangeRangeExtensions.cs

+        /// <remarks>
+        /// Both `oldChanges` and `newChanges` must contain non-overlapping spans in ascending order.
+        /// </remarks>
+        public static ImmutableArray<TextChangeRange> Merge(ImmutableArray<TextChangeRange> oldChanges, ImmutableArray<TextChangeRange> newChanges)


Consider moving back to the original location and just changing the accessibility to internal.

There are not any IVTs to Features.CSharp from MS.CA, and adding IVTs causes tons of build errors as now it thinks there are duplicate types (e.g. ArrayBuilder, DeclarationInfo, nullable attributes).

dibarbet · 2020-12-04T22:17:17Z

@dotnet/roslyn-compiler could use one more review (thanks Rikki!). Context is here - #48468 (comment)

333fred

Compiler changes lgtm (commit 37)

dibarbet force-pushed the refactor_brace_completion branch 3 times, most recently from f13f9ff to ca0e4e5 Compare October 9, 2020 07:28

CyrusNajmabadi reviewed Oct 9, 2020

View reviewed changes

Roslyn.sln Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Oct 9, 2020

View reviewed changes

src/EditorFeatures/CSharp/AutomaticCompletion/CSharpBraceCompletionSessionProvider.cs Outdated Show resolved Hide resolved

dibarbet force-pushed the refactor_brace_completion branch 2 times, most recently from 70da75b to ff3ba2e Compare October 15, 2020 02:30

jinujoseph added the Area-IDE label Oct 15, 2020

dibarbet force-pushed the refactor_brace_completion branch 2 times, most recently from 55ccc64 to cb4c677 Compare October 16, 2020 06:22

dibarbet force-pushed the refactor_brace_completion branch 3 times, most recently from 92b2f21 to c68ae3e Compare October 27, 2020 01:26

dibarbet commented Oct 27, 2020

View reviewed changes

dibarbet changed the title ~~[WIP] Refactor brace completion to split into Features vs EditorFeatures~~ Refactor brace completion to split into Features vs EditorFeatures Oct 27, 2020

dibarbet marked this pull request as ready for review October 27, 2020 01:37

dibarbet requested a review from a team as a code owner October 27, 2020 01:37

dibarbet requested review from davidwengier and CyrusNajmabadi October 27, 2020 01:39

davidwengier reviewed Oct 27, 2020

View reviewed changes

dibarbet commented Oct 27, 2020

View reviewed changes