Add a /blockstructure endpoint that uses Roslyn's outlining implemention #1209

rchande · 2018-05-31T21:50:00Z

We'll use this to implement language-specific folding in the C# extension

…ation

DustinCampbell · 2018-06-01T09:47:12Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+using System.Composition;
+using System.Reflection;
+using System.Threading;
+using System.Threading.Tasks;


Sort with System namespaces on top.

DustinCampbell · 2018-06-01T09:49:07Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+            _getBlockStructure = _blockStructureService.LazyGetMethod("GetBlockStructure");
+            _getSpans = _blockStructure.Value.GetProperty("Spans");
+            _getIsCollpasible = _blockSpan.Value.GetProperty("IsCollapsible");
+            _getTextSpan = _blockSpan.Value.GetProperty("TextSpan");


Could you file an issue on https://github.com/dotnet/roslyn/issues to add a public API here?

dotnet/roslyn#27332

DustinCampbell · 2018-06-01T09:49:24Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+            var service = _blockStructureService.LazyGetMethod("GetService").InvokeStatic(new[] { document });
+
+            var structure = _getBlockStructure.Invoke<object>(service, new object[] { document, CancellationToken.None });
+            IEnumerable spans = _getSpans.GetMethod.Invoke<IEnumerable>(structure, Array.Empty<object>());


DustinCampbell · 2018-06-01T09:49:31Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+            var structure = _getBlockStructure.Invoke<object>(service, new object[] { document, CancellationToken.None });
+            IEnumerable spans = _getSpans.GetMethod.Invoke<IEnumerable>(structure, Array.Empty<object>());
+
+            List<QuickFix> outliningSpans = new List<QuickFix>();


DustinCampbell · 2018-06-01T09:51:57Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+                    {
+                        Line = lines.GetLineFromPosition(textSpan.Start).LineNumber,
+                        EndLine = lines.GetLineFromPosition(textSpan.End).LineNumber
+                    });


Is there are reason not to add Column and EndColumn too? I know that VS Code's FoldingRange only handles lines but other editors might find this useful.

DustinCampbell

I'm very glad you're adding this!

DustinCampbell · 2018-06-01T09:58:34Z

src/OmniSharp.Roslyn.CSharp/Services/Structure/BlockStructureService.cs

+namespace OmniSharp.Roslyn.CSharp.Services.Structure
+{
+    [OmniSharpHandler(OmniSharpEndpoints.BlockStructure, LanguageNames.CSharp)]
+    public class BlockStructureService : IRequestHandler<Request, IEnumerable<QuickFix>>


Is QuickFix the right thing? I have two thoughts:

VS Code's FoldingRange also has a FoldingRangeKind that we might want to expose, so we might want a custom model.

QuickFix is a "kitchen sink" model with lots of baggage that even unnecessarily allocates a List<string>. I've never been a fan of overusing it in the protocol. If most of the properties on QuickFix aren't interesting, it's probably cleaner to just define a new BlockStructure model.

DustinCampbell · 2018-06-01T10:03:27Z

This aligns well with the /codestructure end point that I'm working on that'll serve as a replacement for /currentfilemembersastree and /currentfilemembersasflat

DustinCampbell · 2018-06-01T10:06:01Z

src/OmniSharp.Abstractions/OmniSharpEndpoints.cs

@@ -25,6 +25,7 @@ public static class OmniSharpEndpoints
        public const string Rename = "/rename";
        public const string SignatureHelp = "/signatureHelp";
        public const string MembersTree = "/currentfilemembersastree";
+        public const string BlockStructure = "/blockstructure";


Should this be a V2 endpoint like other new endpoints that have been added?

DustinCampbell · 2018-06-01T11:07:24Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+            var lineSpans = (await GetResponseAsync(testFile1)).
+                    Select(l => (l.Line, l.EndLine)).ToArray();
+
+            var expected = new [] { (0, 8), (2, 7), (4, 6) };


There is a markup feature for tests in OmniSharp that is similar to Roslyn's that you can use instead of raw numbers. You can see it used in places like this: https://github.com/OmniSharp/omnisharp-roslyn/blob/master/tests/OmniSharp.Roslyn.CSharp.Tests/FormattingFacts.cs#L141

rchande · 2018-06-01T21:17:16Z

@DustinCampbell I incorporated your feedback. Want to sign off?

david-driscoll

LGTM

DustinCampbell

Sorry for all of the additional feedback @rchande. I gave this PR another serious look. Most of my feedback are little nits or minor details. However, I'd like to consider whether the protocol endpoint that's being introduced has the right shape. After all, this is essentially a public OmniSharp API. Just passing the Roslyn types through is possibly not the right thing. Could you take a look at that feedback?

DustinCampbell · 2018-06-04T17:22:03Z

src/OmniSharp.Abstractions/Models/v2/BlockSpan.cs

+        /// <summary>
+        /// The span of text to collapse.
+        /// </summary>
+        public Range TextSpan { get; }


We might consider using "Range" terminology here. I don't think "Span" appears anywhere in OmniSharp's protocol.

DustinCampbell · 2018-06-04T17:22:17Z

src/OmniSharp.Abstractions/Models/v2/BlockSpan.cs

+        /// <summary>
+        /// The span of text to display in the hint on mouse hover.
+        /// </summary>
+        public Range HintSpan { get; }


Ditto here.

DustinCampbell · 2018-06-04T17:23:27Z

src/OmniSharp.Abstractions/Models/v2/BlockSpan.cs

@@ -0,0 +1,30 @@
+namespace OmniSharp.Models.V2
+{
+    public class BlockSpan


Should this be more specific to the purpose its indeed? E.g. CodeFoldingBlock or something like that?

DustinCampbell · 2018-06-04T17:23:43Z

src/OmniSharp.Abstractions/Models/v2/BlockSpan.cs

+        /// </summary>
+        public string BannerText { get; }
+
+        public string Type { get; }


What is this? What values are legal?

This is property on the internal Roslyn types that's also stringly-typed. I wasn't sure if we would need to use it at some point so I left it in, but I can also remove it.

I think it'd be good to think through the protocol API that makes sense for OmniSharp, rather than serving up internal API structures from Roslyn which haven't gone through a review process.

For example, we should provide the possible values here so that OmniSharp consumers understand what it's used for.

DustinCampbell · 2018-06-04T17:24:15Z

src/OmniSharp.Abstractions/Models/v2/Point.cs

+                   Column == point.Column;
+        }
+
+        public override int GetHashCode()


This will conflict with my PR. Sorry about that!

DustinCampbell · 2018-06-04T17:30:30Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+
+
+            var expected = testFile.Content.GetSpans()
+                .Select(t => t.ToRange(text.Lines)).ToArray();


Note that there's already a TestContent.GetRangeFromSpan(...) helper that you can use. Ultimately that calls an existing extension method in TestUtility on SourceText: https://github.com/OmniSharp/omnisharp-roslyn/blob/master/tests/TestUtility/SourceTextExtensions.cs.

That extension returns TestUtiltity.TextRange, but my product code returns OmniSharp.Models.V2.Range. These types have essentially the same shape and there are even duplicated test/product Point classes. I don't really want to write code that knows how to compare test and product Range/Points. Thoughts on the best way to resolve this?

TestUtility.TextRange has a ToRange method that returns an OmniSharp.Models.V2.Range. Maybe just use that and not write any extra code? 👍

DustinCampbell · 2018-06-04T17:30:47Z

src/OmniSharp.Roslyn/Extensions/TextSpanExtensions.cs

+{
+    public static class TextSpanExtensions
+    {
+        public static Range ToRange(this TextSpan textSpan, TextLineCollection lines)


I don't think you need to add this extension method. See my comment below.

DustinCampbell · 2018-06-04T17:31:36Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+{
+    void M()[|
+    {
+        if (false)[|


Note that this isn't collapsible in Visual Studio. I think we might be conflating the vertical indicators in VS with code folding.

Well, actually 😄. :

huh. I thought we hadn't done that. I stand happily corrected. 😄

DustinCampbell · 2018-06-04T17:34:05Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+
+        private async Task<BlockStructureResponse> GetResponseAsync(TestFile testFile)
+        {
+            SharedOmniSharpTestHost.AddFilesToWorkspace(new[] { testFile });


nit: you don't need to construct an array here. It's a params array parameter.

DustinCampbell · 2018-06-04T17:34:38Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+            };
+
+            var requestHandler = GetRequestHandler(SharedOmniSharpTestHost);
+            return await requestHandler.Handle(request);


There's no need to make this method async and await here. Just return the Task.

DustinCampbell · 2018-06-05T17:40:17Z

src/OmniSharp.Abstractions/Models/v2/BlockStructureRequest.cs

+namespace OmniSharp.Models.v2
+{
+    [OmniSharpEndpoint(OmniSharpEndpoints.V2.BlockStructure, typeof(BlockStructureRequest), typeof(BlockStructureResponse))]
+    public class BlockStructureRequest : Request


This should probably just inherit from SimpleFileRequest. It doesn't need to support all of the properties from Request.

…into outlining

rchande · 2018-06-06T16:58:44Z

src/OmniSharp.Abstractions/Models/v2/CodeFoldingBlock.cs

+
+    public class CodeFoldingBlockKind
+    {
+        public static readonly string Comment = nameof(Comment).ToLowerInvariant();


I chose to include these values because the VS Code folding API has special handling for these types of blocks

Nice! The type should probably be plural -- CodeFoldingBlockKinds.

rchande · 2018-06-06T16:59:00Z

@DustinCampbell I think I got everything, care to sign off now?

DustinCampbell

I really, really want to sign off. 😄 However, it looks like there's a mismerge in this PR that causes C# 7.3 support to be disabled. Also, could you take a look at the minor renames to the protocol that I suggested?

DustinCampbell · 2018-06-06T17:15:32Z

src/OmniSharp.Abstractions/Models/v2/CodeFoldingBlock.cs

+
+    public class CodeFoldingBlockKind
+    {
+        public static readonly string Comment = nameof(Comment).ToLowerInvariant();


Nice! The type should probably be plural -- CodeFoldingBlockKinds.

DustinCampbell · 2018-06-06T17:16:05Z

src/OmniSharp.Abstractions/Models/v2/CodeFoldingBlock.cs

+        /// If the block is one of the types specified in <see cref="CodeFoldingBlockKind"/>, that type.
+        /// Otherwise, null.
+        /// </summary>
+        public string Type { get; }


DustinCampbell · 2018-06-06T17:18:05Z

src/OmniSharp.Abstractions/Models/v2/CodeFoldingBlock.cs

+        public string Type { get; }
+    }
+
+    public class CodeFoldingBlockKind


This should probably be plural: CodeFoldingBlockKinds.

DustinCampbell · 2018-06-06T17:18:33Z

src/OmniSharp.MSBuild/ProjectFile/PropertyConverter.cs

@@ -47,7 +47,6 @@ public static LanguageVersion ToLanguageVersion(string propertyValue)
                case "7": return LanguageVersion.CSharp7;
                case "7.1": return LanguageVersion.CSharp7_1;
                case "7.2": return LanguageVersion.CSharp7_2;
-                case "7.3": return LanguageVersion.CSharp7_3;


I don't think you meant to break C# 7.3 did you? Mis-merge?

DustinCampbell · 2018-06-06T17:18:55Z

src/OmniSharp.Roslyn.CSharp/OmniSharp.Roslyn.CSharp.csproj

@@ -16,4 +16,8 @@
    <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="$(MicrosoftCodeAnalysisCSharpWorkspacesVersion)" />
  </ItemGroup>

+  <ItemGroup>
+    <Folder Include="Services\Blocks\" />


This looks unnecessary.

DustinCampbell · 2018-06-06T17:19:20Z

tests/OmniSharp.Roslyn.CSharp.Tests/BlockStructureFacts.cs

+                .Select(b => b.Range)
+                .ToArray();
+
+


nit: extra blank line.

…into outlining

… into outlining

DustinCampbell

Lovely 😄

Forgot to do this in #1209 :)

Add a /blockstructure endpoint that uses Roslyn's outlining implement…

ab2a160

…ation

rchande requested review from DustinCampbell, TheRealPiotrP and akshita31 May 31, 2018 21:50

DustinCampbell reviewed Jun 1, 2018

View reviewed changes

DustinCampbell changed the title ~~Add a /blockstructure endpoint that uses Roslyn's outlining implement…~~ Add a /blockstructure endpoint that uses Roslyn's outlining implemention Jun 1, 2018

DustinCampbell reviewed Jun 1, 2018

View reviewed changes

rchande mentioned this pull request Jun 1, 2018

Add public API for "BlockStructure" dotnet/roslyn#27332

Closed

Ravi Chande added 3 commits June 1, 2018 12:48

Add request type

8055a70

Add dedicated BlockStructure return type

163336d

Cr feedback

2b675c3

rchande mentioned this pull request Jun 1, 2018

Implement VS Code syntax-based folding API dotnet/vscode-csharp#2360

Merged

Ravi Chande added 2 commits June 1, 2018 12:58

Add missed file

e57dbc4

Clean up unused usings

1b76656

david-driscoll approved these changes Jun 2, 2018

View reviewed changes

DustinCampbell requested changes Jun 4, 2018

View reviewed changes

DustinCampbell reviewed Jun 5, 2018

View reviewed changes

Ravi Chande added 3 commits June 6, 2018 09:35

Wip

53828f6

Merge branch 'master' of https://github.com/OmniSharp/omnisharp-roslyn …

27d5eb3

…into outlining

CR feedback

a1c7841

rchande commented Jun 6, 2018

View reviewed changes

Use correct casing

6d94006

DustinCampbell requested changes Jun 6, 2018

View reviewed changes

DustinCampbell and others added 4 commits June 7, 2018 07:43

Merge branch 'master' into outlining

be90f02

CR feedback

af6ee65

Merge branch 'master' of https://github.com/OmniSharp/omnisharp-roslyn …

473500b

…into outlining

Merge branch 'outlining' of https://github.com/rchande/omnisharp-roslyn…

d8dbd6b

… into outlining

DustinCampbell approved these changes Jun 15, 2018

View reviewed changes

rchande merged commit c7b6f80 into OmniSharp:master Jun 18, 2018

rchande pushed a commit that referenced this pull request Jun 18, 2018

Update changelog.md for blockstructure

12c7a00

Forgot to do this in #1209 :)

rchande mentioned this pull request Jun 18, 2018

Update changelog.md for blockstructure #1228

Merged

bjorkstromm mentioned this pull request Jun 18, 2018

Implement /blockstructure for Cake #1229

Closed



		var expected = testFile.Content.GetSpans()
		.Select(t => t.ToRange(text.Lines)).ToArray();

Add a /blockstructure endpoint that uses Roslyn's outlining implemention #1209

Add a /blockstructure endpoint that uses Roslyn's outlining implemention #1209

Conversation

rchande commented May 31, 2018 • edited by DustinCampbell Loading

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

DustinCampbell Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

DustinCampbell commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchande commented Jun 1, 2018

david-driscoll left a comment

Choose a reason for hiding this comment

DustinCampbell left a 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

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

rchande commented Jun 6, 2018

DustinCampbell left a 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

DustinCampbell left a comment

Choose a reason for hiding this comment

rchande commented May 31, 2018 •

edited by DustinCampbell

Loading

DustinCampbell Jun 1, 2018 •

edited

Loading

DustinCampbell Jun 1, 2018 •

edited

Loading

DustinCampbell Jun 1, 2018 •

edited

Loading