[LSP] Respect settings passed in by LSP in format handlers by allisonchou · Pull Request #52044 · dotnet/roslyn

allisonchou · 2021-03-22T10:22:46Z

Today, LSP passes in tabs/spaces settings as part of formatting requests. However, we do not respect these settings and instead use the document's options. We should respect the LSP settings.

Follow up from #51916.

Note, I think this will also require a change on the TypeScript side as they implement IEditorFormattingService. I opened a PR on their side here - https://devdiv.visualstudio.com/DevDiv/_git/TypeScript-VS/pullrequest/312160

allisonchou · 2021-03-22T10:35:12Z

            var span = textSpan ?? new TextSpan(0, root.FullSpan.Length);
            var formattingSpan = CommonFormattingHelpers.GetFormattingSpan(root, span);

-            var options = await document.GetDocumentOptionsWithInferredIndentationAsync(explicitFormat: true, indentationManagerService: _indentationManagerService, cancellationToken: cancellationToken).ConfigureAwait(false);


A bunch of lines in this PR were really long on my laptop, so I broke them up for easier future readability.

allisonchou · 2021-03-22T10:38:01Z

+        /// Returns the text changes necessary to format the document with custom document options.
+        /// If "textSpan" is provided, only the text changes necessary to format that span are needed.
+        /// </summary>
+        Task<IList<TextChange>> GetFormattingChangesAsync(Document document, TextSpan? textSpan, DocumentOptionSet documentOptions, CancellationToken cancellationToken);


TS + F# both use this interface so I had to add new methods instead of modifying the existing ones.

Do they not use the ExternalAccess patterns?

I believe they do for some interfaces, but for this one it looks like they just implement it directly?

I don't understand why this is better than adding a parameter? Isn't it a breaking change either way?

If we add the methods and merge the TypeScript change first, I don't think it would be a breaking change, since TypeScript will just have unused members that aren't associated with any interface method. When we then merge the interface changes in from the Roslyn side, those unassociated members will be tied to interface members.

However, if others feel strongly that editing the existing interface methods is the correct way to go here, then maybe a breaking change would be OK? It would just have to be a coordinated insertion with TypeScript (I think).

I don't think it would be a breaking change, since TypeScript will just have unused members that aren't associated with any interface method. When we then merge the interface changes in from the Roslyn side, those unassociated members will be tied to interface members.

This is still a breaking change AFAIK, since it's at the moment TypeScript is compiled that the compiler is sorting out what is and isn't a interface implementation. TypeScript needs to be recompiled no matter what.

Gotcha, in that case I modified the code to just update the existing signature.

allisonchou · 2021-03-22T10:39:04Z


            // The editor calls this handler for C# and VB comment characters, but we only need to process the one for the language that matches the document
-            if (autoInsertParams.Character == "\n" || autoInsertParams.Character == service.DocumentationCommentCharacter)
+            if (request.Character == "\n" || request.Character == service.DocumentationCommentCharacter)


For consistency, I changed this from autoInsertParams -> request since that's the variable name all other handlers use.

jasonmalinowski

See note about tab size vs. indentation size; I suspect this will do something pretty strange if we're not careful. In either case I didn't see any tests that set something other than 4. which we probably should add.

jasonmalinowski · 2021-03-26T22:26:35Z

+            int position,
+            DocumentOptionSet documentOptions,
+            CancellationToken cancellationToken)
+            // We don't need to pass in options here since C#/VB always returns no text changes for this case.


Will we need to at some point though? Or does F#/TypeScript need to? This is more a "if we're breaking the interface, do we need to break this too" sort of question.

Hmm, good question. I guess it makes sense to add it just in case we do add support for this method in the future.

jasonmalinowski · 2021-03-26T22:36:58Z

+            var documentOptions = await document.GetOptionsAsync(cancellationToken).ConfigureAwait(false);
+            var updatedOptions = documentOptions
+                .WithChangedOption(Formatting.FormattingOptions.UseTabs, !options.InsertSpaces)
+                .WithChangedOption(Formatting.FormattingOptions.TabSize, options.TabSize);


Does this also need to set Formatting.FormattingOptions.IndentationSize as well to match tab size? I wouldn't be surprised if VS Code doesn't support the distinction but usually you want these the same unless you have really good reasons not to do so.

Ah I see. LSP doesn't support indent size currently, only tab size, but I'll update the code so the two match.

dibarbet

the changes here look fine, as mentioned offline though I'm still not sure of the pros/cons of forking the document (for the request) with new options vs passing them in

dibarbet · 2021-03-30T23:06:05Z

+        /// If "textSpan" is provided, only the text changes necessary to format that span are needed.
        /// </summary>
-        Task<IList<TextChange>> GetFormattingChangesAsync(Document document, TextSpan? textSpan, CancellationToken cancellationToken);
+        Task<IList<TextChange>> GetFormattingChangesAsync(Document document, TextSpan? textSpan, DocumentOptionSet? documentOptions, CancellationToken cancellationToken);


this is fine, just wanted to bikeshed a little bit - should this be something like documentOptionsOverride? to clearly indicate that these are optional options to override whatever the default document options are

I would just do OptionsSet options to match how we do this elsewhere.

The implementations of these methods depend on specifically using DocumentOptionsSet as opposed to OptionsSet, so I don't know if we can switch the types here.

uniqueiniquity

Validated the breaking change from the TS side 👍

…matHandlers

allisonchou added 2 commits March 22, 2021 02:42

Use LSP options for format handlers

fc36bad

Don't add unnecessary method

8aea881

allisonchou added Area-IDE LSP issues related to the roslyn language server protocol implementation labels Mar 22, 2021

allisonchou requested review from a team as code owners March 22, 2021 10:22

allisonchou commented Mar 22, 2021

View reviewed changes

allisonchou requested review from CyrusNajmabadi, NTaylorMullen, davidwengier and dibarbet March 22, 2021 10:41

allisonchou mentioned this pull request Mar 24, 2021

Add support for setting tabs/spaces preference in Tools->Options dotnet/razor#3317

Merged

jasonmalinowski reviewed Mar 26, 2021

View reviewed changes

allisonchou added 3 commits March 30, 2021 14:09

Code review feedback

1f6e9a4

Code review feedback

6d32895

Add test and comment

822c332

allisonchou requested a review from jasonmalinowski March 30, 2021 22:55

dibarbet approved these changes Mar 31, 2021

View reviewed changes

uniqueiniquity approved these changes Apr 1, 2021

View reviewed changes

allisonchou added 2 commits April 2, 2021 12:52

Merge remote-tracking branch 'upstream/main' into UseLSPOptionsForFor…

9316ab9

…matHandlers

Fix merge conflicts

ad0596d

allisonchou merged commit 00f046a into dotnet:main Apr 12, 2021

ghost added this to the Next milestone Apr 12, 2021

allisonchou deleted the UseLSPOptionsForFormatHandlers branch April 12, 2021 18:15

dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021

Conversation

allisonchou commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

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

uniqueiniquity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

allisonchou commented Mar 22, 2021 •

edited

Loading

uniqueiniquity left a comment •

edited

Loading