Move indentation services down to Workspace layer. by CyrusNajmabadi · Pull Request #33978 · dotnet/roslyn

CyrusNajmabadi · 2019-03-08T23:45:34Z

For reasons that have never been quite clear to me, our indentation services were defined at the EditorFeatures layer (maybe because they thought it was an editor concept?). This was especially odd as similar features (like Formatting) are defined at the Workspaces layer.

This change simply moves things down to the Workspace layer (while also keeping around and obsoleting the existing editor-interfaces since they are consumed by F#/TS/JS).

This makes thigns much cleaner, and makes it possible to push other features down (like the wrapping-code-actions).

CyrusNajmabadi · 2019-03-08T23:49:37Z

Tagging @dpoeschl @sharwell

src/Workspaces/Core/Portable/Indentation/IIndentationService.cs

CyrusNajmabadi · 2019-03-09T00:22:28Z

@dotnet/roslyn-infrastructure @jasonmalinowski @jaredpar

Do you know what causes this error on pipelines:

 IlAsmDeploy -> F:\vsagent\1\s\artifacts\bin\IlAsmDeploy\Debug\netcoreapp2.1\win-x64\IlAsmDeploy.dll
F:\vsagent\1\s\src\Tools\ILAsm\IlAsmDeploy.csproj(46,11): error MSB4006: There is a circular dependency in the target dependency graph involving target "_PublishILAsm".

?

CyrusNajmabadi · 2019-03-09T00:33:11Z

Oh, the formatting stuff is even weirder than i first though. It looks like we actually run SmartTokenCommandHandler and FormatCommandHandler in the same chain. Though there's nothing that indicates that these should be ordered in any particular fashion. This is definitely a bit strange as that means you could hand Format handle enter first, followed by SmartToken, or vice versa.

In my current debug-session, SmartTokenCommandHandler called into FormatCommandHandler. There FormatCommandHandler bailed out very quickly (it looks like it only tries to handle close-parens and using-statements??). We then got back to SmartTokenCommandHandler, which did a bunch of checks, and then bailed out because it thought the editor handled things...

It's all pretty confusing about who/what/where and when is responsible for handling thse things. @heejaechang can you shed some light on the design here and the separation of responsibilities?

One good thin to know would be: say we had a bug in hitting 'enter'. How would you determine which of these three systems was responsible for handling fixing it?

heejaechang · 2019-03-09T01:16:06Z

we have 3 different scenario.

format - people issue format command
indentation - people ask where caret should go on blank line
auto formatting - this do bunch stuff
a. if it is one of certain token like ";" or "}", it finds starting point (which is heuristic on what considered as starting point) and format those span.
b. if it is enter, then it sees whether caret was before certain token like { or whether it is at the start location of token, and whether it is enter on a whitespace, and do things differently. here it is full case by case people added as people get feedbacks.

also, for 2 and 3, since formatting doesn't touch any expression level construct except certain cases like query expression, for 2 and 3 to handle "enter" or "indentation" of expression level, it adds some extra rules for things like argument list, parameter list and etc to format engine.

also for us to not re-implementation logic to determine where to put caret based on various formatting options for indentation, it uses formatting rules to determine indentation if there is new line indentation rule. otherwise, it uses heuristic to find indentation since there is no option to change its behavior.

and there are bunch of case by case for indentation from feedbacks. can't remember all those special cases.

heejaechang · 2019-03-09T01:19:51Z

ah, and we also has special case for razor and venus through some formatting rule provider and etc. we can't get rid of that due to venus. but hopefully for new razor, we don't need to use it.

CyrusNajmabadi · 2019-03-09T01:20:37Z

Could you clarify what the intended split is across all three indentation systems today? To help, i asked this question:

One good thin to know would be: say we had a bug in hitting 'enter'. How would you determine which of these three systems was responsible for handling fixing it?

CyrusNajmabadi · 2019-03-09T01:24:12Z

we have 3 different scenario.

Ok. But hte only scenario i'm really asking about is 'indentation/enter-handling'. My question is about why we've separated the logic out for that into mulitple different locations, with each location needing to know about and poke into the other locations to figure out what's going on. That's the part that seems very confusing, and makes it very difficult for, say, a feature to ask "where should i indent here?"

CyrusNajmabadi · 2019-03-09T01:28:59Z

Let me try to ask teh question as simply as possible:

We have:

internal partial class SmartIndent : ISmartIndent

And

internal class SmartTokenFormatterCommandHandler

Both are responsible for figuring out where to place the care after enter is hit. My questions are these:

Why do we have both of these?
What determines which logic each is responsible for?
Why can't we just merge the logic of these into the other?

I can't figure out any rhyme or reasons on these guys.

--

Note: if gitter would be better to discuss things, i'm happy to take it there. It might be easier to ask questions and build understanding versus trying to do this back and forth over github. Which would you prefer? Thanks!

CyrusNajmabadi · 2019-03-09T02:32:05Z

I guess it is time to clean things up here.

Alright, i'll see what i can do.

CyrusNajmabadi · 2019-03-09T09:07:52Z

I guess it is time to clean things up here.

@heejaechang i have cleaned up a lot here: #33989

This removes unnecessary separation of services for handling indentation.

--

THe end result gets rid of a lot. but it still ends up being complex. That's because the indentation service itself has all these different codepaths trying to decide what behavior to perform with all sorts of complex assumptions. I have no idea how to make that better, but at least it's all contained in this one service now.

sharwell

Ensuring this gets a design review to determine the approach we want for two specific items:

Are type forwarders preferred to redefining types?
We have active plans to eliminate external code depending on these interfaces, and need to determine which we want to pursue first.

CyrusNajmabadi · 2019-03-09T19:35:06Z

Ensuring this gets a design review to determine the approach we want for two specific items:

Are type forwarders preferred to redefining types?

I would prefer not here. There are changes i want to make to the interfaces in other PRs (for example, making the return types non-nullable). So this will be more than just a move.

CyrusNajmabadi · 2019-03-09T19:35:45Z

We have active plans to eliminate external code depending on these interfaces, and need to determine which we want to pursue first.

Can you explain these plans? What's the recommended way for a 3rd party language to participate in indentation?

sharwell · 2019-03-11T14:02:03Z

Can you explain these plans?

All components not in dotnet/roslyn (except for VS for Mac) must now go through an adapter assembly rather than directly access internal Roslyn APIs. New IVTs are not getting added, and ones already in place are being phased out.

#31393

What's the recommended way for a 3rd party language to participate in indentation?

Through the public API.

sharwell · 2019-03-11T14:07:52Z

I would prefer to migrate existing external components off of the internal indentation APIs before making changes that go beyond the abilities of type forwarders. Will need to review with F# and TypeScript to determine the complexity of these changes. Otherwise this change is vastly more complex to account for a situation that we can avoid from the start.

CyrusNajmabadi · 2019-03-11T16:03:11Z

What public API are you referring to?

CyrusNajmabadi · 2019-05-22T03:42:48Z

@CyrusNajmabadi Can you file an issue tracking removal of the newly obsolete API and link it from the code?

@tmat Done!

CyrusNajmabadi · 2019-05-23T02:22:22Z

@tmat feel free to merge when you havea chance.

tmat · 2019-05-23T16:46:22Z

@heejaechang @sharwell OK to merge?

CyrusNajmabadi · 2019-05-24T16:53:49Z

@heejaechang @sharwell OK to merge?

Ping. Thanks!

CyrusNajmabadi · 2019-05-24T17:08:29Z

@tmat, @sharwell says he's not blocking this, but he's not moving it forward. Would it be possible to directly pull in @heejaechang as i find pings don't reach him very well.

jinujoseph · 2019-05-24T20:14:47Z

spoke to @heejaechang , he will take a look at it soon.

heejaechang · 2019-05-24T21:50:13Z

src/EditorFeatures/CSharp/SplitStringLiteral/SplitStringLiteralCommandHandler.StringSplitter.cs


 namespace Microsoft.CodeAnalysis.Editor.CSharp.SplitStringLiteral
 {
+    using Microsoft.CodeAnalysis.Indentation;


move to where other using is?

can't. if we move it, then diffrent symbols will be found in Microsoft.CodeAnalysis.Editor.

so, we have a plan to move at some point then? or it has to be here until typescript/F# is moving to new API as well?

Yes, this is tracked with: #35872

heejaechang · 2019-05-24T21:50:41Z

...ures/Core/Implementation/Formatting/Indentation/AbstractSmartTokenFormatterCommandHandler.cs


 namespace Microsoft.CodeAnalysis.Editor.Implementation.Formatting.Indentation
 {
+    using Microsoft.CodeAnalysis.Indentation;


can't. if we move it, then diffrent symbols will be found in Microsoft.CodeAnalysis.Editor.

heejaechang · 2019-05-24T21:51:37Z

src/EditorFeatures/Core/Implementation/SmartIndent/SmartIndent.cs

-                if (asyncService != null)
+                // If we don't have a feature-layer service, try to fall back to the legacy
+                // editor-feature-layer interfaces.
+


is this for things like typescript or F#?

heejaechang · 2019-05-24T21:52:19Z

src/EditorFeatures/Core/Shared/Options/FeatureOnOffOptions.cs

            nameof(FeatureOnOffOptions), nameof(AutoFormattingOnTyping), defaultValue: true,
            storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.Auto Formatting On Typing"));

-        public static readonly PerLanguageOption<bool> AutoFormattingOnReturn = new PerLanguageOption<bool>(


where it go?

heejaechang · 2019-05-24T21:52:29Z

src/EditorFeatures/Core/Wrapping/BinaryExpression/AbstractBinaryExpressionWrapper.cs


 namespace Microsoft.CodeAnalysis.Editor.Wrapping.BinaryExpression
 {
+    using Microsoft.CodeAnalysis.Indentation;


can't. if we move it, then different symbols will be found in Microsoft.CodeAnalysis.Editor.

heejaechang · 2019-05-24T21:52:39Z

src/EditorFeatures/Core/Wrapping/SeparatedSyntaxList/AbstractSeparatedSyntaxListWrapper.cs


 namespace Microsoft.CodeAnalysis.Editor.Wrapping.SeparatedSyntaxList
 {
+    using Microsoft.CodeAnalysis.Indentation;


can't. if we move it, then different symbols will be found in Microsoft.CodeAnalysis.Editor.

heejaechang · 2019-05-24T21:53:47Z

src/Workspaces/Core/Portable/Formatting/FormattingOptions.cs


        internal static Option<bool> AllowDisjointSpanMerging { get; } = CreateOption(OptionGroup.Default, nameof(AllowDisjointSpanMerging), defaultValue: false);

+        internal static readonly PerLanguageOption<bool> AutoFormattingOnReturn = CreatePerLanguageOption(OptionGroup.Default, nameof(AutoFormattingOnReturn), defaultValue: true,


ah. I see. it moved to here.

heejaechang · 2019-05-24T21:55:13Z

src/Workspaces/Core/Portable/Formatting/FormattingOptions.cs

        internal static Option<bool> AllowDisjointSpanMerging { get; } = CreateOption(OptionGroup.Default, nameof(AllowDisjointSpanMerging), defaultValue: false);

+        internal static readonly PerLanguageOption<bool> AutoFormattingOnReturn = CreatePerLanguageOption(OptionGroup.Default, nameof(AutoFormattingOnReturn), defaultValue: true,
+            storageLocations: new RoamingProfileStorageLocation("TextEditor.%LANGUAGE%.Specific.Auto Formatting On Return"));


this option location is very specific to VS. so I am not sure what it means that Workspace layer saving option to place where it is specific to VS.

the option itself is a bit strange. it's used in one place:

public static bool ShouldUseSmartTokenFormatterInsteadOfIndenter( IEnumerable<AbstractFormattingRule> formattingRules, CompilationUnitSyntax root, TextLine line, OptionSet optionSet, CancellationToken cancellationToken) { Contract.ThrowIfNull(formattingRules); Contract.ThrowIfNull(root); if (!optionSet.GetOption(FormattingOptions.AutoFormattingOnReturn, LanguageNames.CSharp)) { return false; }

Which is in the indentation service itself. I think it's a way to allow us (potentially for manual testing purposes) to opt out of smart-indent. It's a bit wonky, but i'm not sure to do about it. We could try to figure something else out once the following two PRs go in here.

heejaechang

looks good to me!

CyrusNajmabadi · 2019-05-24T22:09:24Z

@heejaechang if you want, feel free to merge in. I'll move ahead with teh followup PRs and we can then decide how best to structure the options here.

CyrusNajmabadi · 2019-05-25T18:58:07Z

@heejaechang can you please merge in?

CyrusNajmabadi · 2019-05-28T17:50:11Z

ping @heejaechang

CyrusNajmabadi · 2019-05-28T21:23:11Z

Thanks!

CyrusNajmabadi · 2019-05-28T22:04:20Z

@heejaechang Did you squash this PR? I've having a heck of a time trying to merge it into all my other branches :-/

CyrusNajmabadi added 2 commits March 8, 2019 15:38

Move IndentationService down to Feature layer.

38f644f

MOve to workspaces

c5fa15b

CyrusNajmabadi requested a review from a team as a code owner March 8, 2019 23:45

Cleanup

6864d02

CyrusNajmabadi commented Mar 8, 2019

View reviewed changes

src/Workspaces/Core/Portable/Indentation/IIndentationService.cs Show resolved Hide resolved

Fix.

fcafe82

move ISmartTokenFormatter down as well

5505579

CyrusNajmabadi mentioned this pull request Mar 9, 2019

Merge all indentation related code into one service. #33989

Merged

Merge remote-tracking branch 'upstream/master' into moveIndentationDown

45fcb48

simplify

1ee0717

sharwell previously requested changes Mar 9, 2019

View reviewed changes

sharwell added Blocked Need Design Review The end user experience design needs to be reviewed and approved. Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 9, 2019

CyrusNajmabadi added 3 commits May 21, 2019 20:37

Move using inside

2babee5

Move using inside

3ce001a

Move using inside

0dc3b4f

CyrusNajmabadi mentioned this pull request May 22, 2019

Remove obsolete editor-layer IIndentationService #35872

Closed

Add links

ad99a7b

Merge branch 'master' into moveIndentationDown

4479804

sharwell removed the Blocked label May 24, 2019

heejaechang reviewed May 24, 2019

View reviewed changes

heejaechang approved these changes May 24, 2019

View reviewed changes

heejaechang merged commit 5e10660 into dotnet:master May 28, 2019

CyrusNajmabadi deleted the moveIndentationDown branch May 28, 2019 21:23

CyrusNajmabadi mentioned this pull request Mar 2, 2020

Simplify smart indentation implementation. #15711

Closed


		internal static Option<bool> AllowDisjointSpanMerging { get; } = CreateOption(OptionGroup.Default, nameof(AllowDisjointSpanMerging), defaultValue: false);

		internal static readonly PerLanguageOption<bool> AutoFormattingOnReturn = CreatePerLanguageOption(OptionGroup.Default, nameof(AutoFormattingOnReturn), defaultValue: true,

Conversation

CyrusNajmabadi commented Mar 8, 2019

Uh oh!

CyrusNajmabadi commented Mar 8, 2019

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

heejaechang commented Mar 9, 2019

Uh oh!

heejaechang commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

CyrusNajmabadi commented Mar 9, 2019

Uh oh!

sharwell commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Mar 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 11, 2019

Uh oh!

CyrusNajmabadi commented May 22, 2019

Uh oh!

CyrusNajmabadi commented May 23, 2019

Uh oh!

tmat commented May 23, 2019

Uh oh!

CyrusNajmabadi commented May 24, 2019

Uh oh!

CyrusNajmabadi commented May 24, 2019

Uh oh!

jinujoseph commented May 24, 2019

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!

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!

CyrusNajmabadi commented Mar 9, 2019 •

edited

Loading

sharwell commented Mar 11, 2019 •

edited

Loading

sharwell commented Mar 11, 2019 •

edited

Loading