Make SolutionServices (and related) APIs public by CyrusNajmabadi · Pull Request #63241 · dotnet/roslyn

CyrusNajmabadi · 2022-08-05T19:58:09Z

Approved by Public API committee.

I have commented interesting lines to help explain the API shape changes that were approved and came along with things becoming public.

CyrusNajmabadi · 2022-08-05T19:58:31Z

src/Analyzers/CSharp/CodeFixes/InlineDeclaration/CSharpInlineDeclarationCodeFixProvider.cs

            identifier = currentRoot.GetCurrentNode(identifier);

-            var editor = document.GetSyntaxEditor(currentRoot);
+            var editor = new SyntaxEditor(currentRoot, document.Project.Solution.Services);


Reverted this code to the original pattern now that we have this public API.

CyrusNajmabadi · 2022-08-05T20:03:29Z

src/EditorFeatures/CSharp/Formatting/CSharpFormattingInteractionService.cs

        {
            var parsedDocument = ParsedDocument.CreateSynchronously(document, cancellationToken);
-            var options = textBuffer.GetSyntaxFormattingOptions(_editorOptionsService, parsedDocument.ProjectServices, explicitFormat: true);
+            var options = textBuffer.GetSyntaxFormattingOptions(_editorOptionsService, parsedDocument.LanguageServices, explicitFormat: true);


ProjectServices was renamed to LanugageServices on behalf of the API committee. This was preferred as the type exposes ILanguageServices. I originally didn't name it that way as we we had HostLanguageServices and i was naming the type HostXXX. However, the API group decided to drop Host. So now we have:

HostWorkspaceServices (existing, mutable) and its immutable counterpart: SolutionServices
HostLanguageServices (existing, mutable) and its immutable counterpart: LanguageServices

CyrusNajmabadi · 2022-08-05T20:04:07Z

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


            var (formattingChanges, finalCurlyBraceEnd) = FormatTrackingSpan(
                context.Document,
-                context.Document.LanguageServices.ProjectServices,


unnecessarily redundant with the ParsedDocument being passed in. removed this parameter.

CyrusNajmabadi · 2022-08-05T20:05:25Z

src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs

        /// Creates a new <see cref="SyntaxEditor"/> instance.
        /// </summary>
-        internal SyntaxEditor(SyntaxNode root, HostSolutionServices services)
+        public SyntaxEditor(SyntaxNode root, SolutionServices services)


New public API.

CyrusNajmabadi · 2022-08-05T20:06:03Z

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt

+Microsoft.CodeAnalysis.Host.SolutionServices.IsSupported(string languageName) -> bool
+Microsoft.CodeAnalysis.Host.SolutionServices.SupportedLanguages.get -> System.Collections.Generic.IEnumerable<string>
+Microsoft.CodeAnalysis.Project.Services.get -> Microsoft.CodeAnalysis.Host.LanguageServices
+Microsoft.CodeAnalysis.Solution.Services.get -> Microsoft.CodeAnalysis.Host.SolutionServices


this is worth reviewing to make sure it aligns with the shape people expect.

CyrusNajmabadi · 2022-08-05T20:06:17Z

src/Workspaces/Core/Portable/Workspace/Host/HostLanguageServices.cs

+        /// Immutable snapshot of the host services.  Preferable to use instead of this <see
+        /// cref="HostLanguageServices"/> when possible.
+        /// </summary>
+        public LanguageServices LanguageServices { get; }


New public API.

CyrusNajmabadi · 2022-08-05T20:06:40Z

src/Workspaces/Core/Portable/Workspace/Host/HostProjectServices.cs

+    /// <summary>
+    /// Per language services provided by the host environment.
+    /// </summary>
+    public sealed class LanguageServices


New public API. ANd all public members in this are the new public api shape. It's worth expanding this file to see/understand them.

CyrusNajmabadi · 2022-08-05T20:08:35Z

src/Workspaces/Core/Portable/Workspace/Host/HostProjectServices.cs

+        internal LanguageServices(HostLanguageServices services)
        {
            _services = services;
        }


note: GetRequiredService below could become an extension method. But that has the downside that the caller must know to include a using for Microsoft.CodeAnalysis.Host to see that extension. so it's just convenient to have thse be instance methods instead.

Instance methods are definitely better. Extension methods are not discoverable.

CyrusNajmabadi · 2022-08-05T20:08:53Z

src/Workspaces/Core/Portable/Workspace/Host/SolutionServices.cs

+    /// <summary>
+    /// Per solution services provided by the host environment.
+    /// </summary>
+    public sealed class SolutionServices


new public API. it's worth expanding this file to see the entire shape exposed.

CyrusNajmabadi · 2022-08-05T20:09:03Z

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs

+        /// Immutable snapshot of language services from the host environment associated with this project's language.
+        /// Use this over <see cref="LanguageServices"/> when possible.
+        /// </summary>
+        public Host.LanguageServices Services => LanguageServices.LanguageServices;


new public API.

CyrusNajmabadi · 2022-08-05T20:09:15Z

src/Workspaces/Core/Portable/Workspace/Solution/Solution.cs

+        /// Per solution services provided by the host environment.  Use this instead of <see
+        /// cref="Workspace.Services"/> when possible.
+        /// </summary>
+        public SolutionServices Services => _state.Services.SolutionServices;


new public API.

CyrusNajmabadi · 2022-08-05T20:10:00Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Extensions/DocumentExtensions.cs

        }
 #endif
-
-        public static SyntaxEditor GetSyntaxEditor(this Document document, SyntaxNode root)


removed this extension which just existed so we could customize CODE_STYLE behavior prior to these types becoming public.

CyrusNajmabadi · 2022-08-05T20:10:20Z

@dotnet/roslyn-ide please review.

src/EditorFeatures/Core/LanguageServer/Handlers/Hover/HoverHandler.cs

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs

tmat · 2022-08-05T22:23:58Z

Many places using Host.LanguageServices qualification. Can these be cleaned up?

src/Workspaces/Core/Portable/Workspace/Host/HostProjectServices.cs

CyrusNajmabadi · 2022-08-06T04:42:40Z

Many places using Host.LanguageServices qualification. Can these be cleaned up?

Yes. but i would strongly advise in a separate PR. Here's the issue, right now this PR adds the public type LanguageServices. However, we also have an internal namespace Microsoft.CodeAnalysis.LanguageServices. This is where types like ISyntaxFactsService are in. This makes a conflict between the type and namespace.

I can resolve the conflict by renaming the existing non-exported namespace to something like LanguageService (no trailing s). Then, LanguageServices can refer directly to this new type. However, that rename of that namespace touches 700+ files (due to all the places that namespace is imported). I would prefer to do this in an immediately following PR just make this manageable.

I will link to that PR immediately though.

CyrusNajmabadi · 2022-08-06T04:56:04Z

@tmat The followup PR is: #63249

CyrusNajmabadi · 2022-08-06T16:47:40Z

@tmat PTAL. Thanks :)

tmat · 2022-08-07T16:49:45Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/Utilities/ParsedDocument.cs

 /// of such feature to either not be invoked on a UI thread or be entirely synchronous.
 /// </remarks>
-internal readonly record struct ParsedDocument(DocumentId Id, SourceText Text, SyntaxNode Root, HostLanguageServices LanguageServices)
+internal readonly record struct ParsedDocument(DocumentId Id, SourceText Text, SyntaxNode Root, HostLanguageServices HostLanguageServices)


HostLanguageServices

For follow up: I'd expect this to be just LanguageServices. Is Workspace being accessed from these services?

Have noted this for follow-up

tmat

CyrusNajmabadi added 9 commits August 5, 2022 12:13

Add docs

579c99c

Rename to SolutionServices

3851226

Add docs

1b25f23

Rename type

4221b65

Rename

380c520

Make services public

1f8fa00

Move extension to be instance method

7a0cc16

rename

0150186

Add to PublicAPI

08aeb6a

CyrusNajmabadi requested review from a team, 333fred and JoeRobich as code owners August 5, 2022 19:58

ghost added the Area-IDE label Aug 5, 2022

CyrusNajmabadi commented Aug 5, 2022

View reviewed changes

CyrusNajmabadi mentioned this pull request Aug 5, 2022

Move options subsystem over to using SolutionService snapshots. #63245

Merged

tmat reviewed Aug 5, 2022

View reviewed changes

src/EditorFeatures/Core/LanguageServer/Handlers/Hover/HoverHandler.cs Show resolved Hide resolved

tmat reviewed Aug 5, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Solution/Project.cs Show resolved Hide resolved

genlu reviewed Aug 6, 2022

View reviewed changes

src/Workspaces/Core/Portable/Workspace/Host/HostProjectServices.cs Show resolved Hide resolved

REname file

da40e25

CyrusNajmabadi mentioned this pull request Aug 6, 2022

Rename internal 'LanguageServices' namespace to not collide with new 'LanguageServices' type. #63249

Merged

CyrusNajmabadi requested a review from tmat August 6, 2022 04:56

tmat reviewed Aug 7, 2022

View reviewed changes

tmat approved these changes Aug 7, 2022

View reviewed changes

CyrusNajmabadi merged commit 1c3cdb0 into dotnet:main Aug 7, 2022

CyrusNajmabadi deleted the solutionServices branch August 7, 2022 20:25

ghost added this to the Next milestone Aug 7, 2022

allisonchou mentioned this pull request Aug 16, 2022

[Automated] PRs inserted in VS build main-32815.244 #63417

Closed

chsienki mentioned this pull request Aug 18, 2022

[release/7.0.1xx-rc1] Update dependencies from dotnet/roslyn dotnet/sdk#27191

Closed

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022

Conversation

CyrusNajmabadi commented Aug 5, 2022 • 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!

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 Aug 5, 2022

Uh oh!

Uh oh!

Uh oh!

tmat commented Aug 5, 2022

Uh oh!

Uh oh!

CyrusNajmabadi commented Aug 6, 2022

Uh oh!

CyrusNajmabadi commented Aug 6, 2022

Uh oh!

CyrusNajmabadi commented Aug 6, 2022

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!

tmat left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Aug 5, 2022 •

edited

Loading