Do not go back to Workspace in SyntaxEditor #62910

CyrusNajmabadi · 2022-07-25T17:15:40Z

We intend to add solutionInstance.Workspace to the banned api list for roslyn itself. It's highly problematic for OOP, as we have to try to replicate that concept, which isn't something sensical in that world. First, OOP cannot mutation things, so exposing the mutable workspace doesn't make much sense. Second, in VS you can have many workspaces, but in OOP we map that all back to one, which means we don't have a proper representation of VS state (for example, where you might have the VSWorkspace and the Metadata-as-source workspace). This means those features either cannot use OOP, or they try to use OOP and can end up with non-sensical results.

This is a series of PRs to get us off of that API so we can move to a much cleaner and simpler OOP model for the solution and services.

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

genlu · 2022-07-25T18:43:32Z

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

        {
        }

        internal SolutionState State => _state;

        internal int WorkspaceVersion => _state.WorkspaceVersion;

-        internal SolutionServices Services => _state.Services;
+        public HostWorkspaceServices Services => _state.Services;


Is this intended to be public? Need API review approval if so.

yes. this will go through API review.

API review tracked through #62914

genlu

Approved pending API review approval for the public API. Or you could change the accessibility back to internal for now and merge this before API review.

CyrusNajmabadi · 2022-07-25T19:31:56Z

Approved pending API review approval for the public API. Or you could change the accessibility back to internal for now and merge this before API review.

so it can't currently be internal, because it's accessed in CodeStyle layer. I think we'll run this like several high confidence APIs. Merge in, and get approval on thursday. If anything needs to change, we can do so then. Are you ok with that @tmat ?

tmat · 2022-07-25T23:22:25Z

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

@@ -176,7 +172,7 @@ public SolutionState WithNewWorkspace(Workspace workspace, int workspaceVersion)
        /// <summary>
        /// The Workspace this solution is associated with.
        /// </summary>
-        public Workspace Workspace => _solutionServices.Workspace;
+        public Workspace Workspace => Services.Workspace;


public Workspace Workspace => Services.Workspace;

For follow-up: Remove to avoid leaking Workspace here.

tmat

CyrusNajmabadi · 2022-07-26T00:15:07Z

Not merging this. Feedback from @jasonmalinowski is making us reconsider this.

CyrusNajmabadi · 2022-07-26T15:16:45Z

Ok, i changed the approach we're taking here to use a new immutable type that doesn't expose the workspace. Please take a look
@genlu @jasonmalinowski @tmat @dibarbet . I will also update the API propposal to reflect this new form.

...s/Core/CodeFixes/UseConditionalExpression/AbstractUseConditionalExpressionCodeFixProvider.cs

…UseConditionalExpressionCodeFixProvider.cs

CyrusNajmabadi · 2022-07-26T15:18:00Z

...p/CodeFixes/NewLines/EmbeddedStatementPlacement/EmbeddedStatementPlacementCodeFixProvider.cs

@@ -48,7 +48,7 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context)
        public static async Task<Document> FixAllAsync(Document document, ImmutableArray<Diagnostic> diagnostics, CodeActionOptionsProvider codeActionOptionsProvider, CancellationToken cancellationToken)
        {
            var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
-            var editor = new SyntaxEditor(root, document.Project.Solution.Workspace.Services);
+            var editor = document.GetSyntaxEditor(root);


these are at a layer (code_style) that cannot access internal apis. So while the API is in flux and hasn't been fully signed off on, this code has to go through an indirection (an extension method) so it can do the right thing in Roslyn vs CodeStyle.

...s/Core/CodeFixes/UseConditionalExpression/AbstractUseConditionalExpressionCodeFixProvider.cs

…UseConditionalExpressionCodeFixProvider.cs

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

…usNajmabadi/roslyn into syntaxEditorWorkspaceService

…ceService

tmat · 2022-07-26T17:43:47Z

...atures/Core/Portable/AddDebuggerDisplay/AbstractAddDebuggerDisplayCodeRefactoringProvider.cs

@@ -115,7 +115,7 @@ private async Task<Document> ApplyAsync(Document document, TTypeDeclarationSynta
            var syntaxRoot = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
            var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);

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


SyntaxEditor

Why not use document.GetSyntaxEditor here too?

this code isn't shared in the CODE_STYLE layer. so it wasn't necessary. a lot of the changes were done mechanically.

tmat · 2022-07-26T17:45:11Z

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

@@ -34,6 +34,14 @@ public SyntaxEditor(SyntaxNode root, Workspace workspace)
        /// Creates a new <see cref="SyntaxEditor"/> instance.
        /// </summary>
        public SyntaxEditor(SyntaxNode root, HostWorkspaceServices services)


SyntaxEditor(SyntaxNode root, HostWorkspaceServices services)

Add this to BannedSymbols?

yes. will do.

tmat · 2022-07-26T17:46:35Z

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

+namespace Microsoft.CodeAnalysis.Host
+{
+    // TODO(cyrusn): Make public.  Tracked through https://github.com/dotnet/roslyn/issues/62914
+    internal sealed class HostProjectServices


class

Can/should this be a struct?

i'm very very hesitant on structs as it is a breaking change to make them classes and it's hard to tell up front if being a struct will be an issue for us (for example, having to deal with potentially default values).

default value is the same as null in this case, in public APIs we would need to check either way

tmat · 2022-07-26T17:47:10Z

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

+
+        // This ensures a single instance of this type associated with each HostLanguageServices.
+        [Obsolete("Do not call directly.  Use HostLanguageServices.ProjectServices to acquire an instance")]
+        internal HostProjectServices(HostLanguageServices services)


Obsolete wouldn't be needed if this was a struct.

yes. but then i have to override == and validate all cases whre code checks identity are correct.

I don't think you need to override ==. Struct does not have to have value equality.

currently we have code that checks if the Services instances are the same instance. I wanted to avoid having to go find and audit all of those (esp. as we have no facilty for searching for use of reference equality == on these types. :)

tmat · 2022-07-26T17:47:32Z

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

+namespace Microsoft.CodeAnalysis.Host
+{
+    // TODO(cyrusn): Make public.  Tracked through https://github.com/dotnet/roslyn/issues/62914
+    internal sealed class HostSolutionServices


sealed

Struct?

tmat · 2022-07-26T17:50:13Z

Seems like in most (all?) cases the caller of SyntaxEditor ctor has a document on hand. Would it make sense to use the extension method in all these call sites?

CyrusNajmabadi · 2022-07-26T17:55:23Z

Seems like in most (all?) cases the caller of SyntaxEditor ctor has a document on hand. Would it make sense to use the extension method in all these call sites?

I don't have a problem with that, but it's currently not necessary. I'm trying to make minimal changes to get us off of Solution.Workspace (and hte other associated banned pathways). Extraneous work/cleanup just adds time to that :)

tmat · 2022-07-26T17:57:02Z

Yeah, but this leaves us in rather inconsistent state where we have multiple ways of using some APIs. I'd prefer not to increase the amount of inconsistency.

CyrusNajmabadi · 2022-07-26T17:57:45Z

Yeah, but this leaves us in rather inconsistent state where we have multiple ways of using some APIs. I'd prefer not to increase the amount of inconsistency.

The intent is to remove that extension once hte API review happens as it won't be needed anymore.

Do not go back to Workspace in SyntaxEditor

846a2aa

CyrusNajmabadi requested review from tmat and dibarbet July 25, 2022 17:15

CyrusNajmabadi requested a review from a team as a code owner July 25, 2022 17:15

dotnet-issue-labeler bot added the Area-IDE label Jul 25, 2022

CyrusNajmabadi added 6 commits July 25, 2022 10:31

Merge branch 'removeSolutionService' into syntaxEditorWorkspaceService

2a9e500

Add extension

d62a15b

Add to project file

02c2900

Make internal again

ce9d52e

Use extension

df48a0b

Make API public

d2fc5d4

CyrusNajmabadi requested a review from a team as a code owner July 25, 2022 18:03

genlu reviewed Jul 25, 2022

View reviewed changes

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

genlu reviewed Jul 25, 2022

View reviewed changes

CyrusNajmabadi requested a review from genlu July 25, 2022 19:17

genlu approved these changes Jul 25, 2022

View reviewed changes

tmat reviewed Jul 25, 2022

View reviewed changes

tmat approved these changes Jul 25, 2022

View reviewed changes

Introduce new api for this

ac35d7a

CyrusNajmabadi commented Jul 26, 2022

View reviewed changes

...s/Core/CodeFixes/UseConditionalExpression/AbstractUseConditionalExpressionCodeFixProvider.cs Outdated Show resolved Hide resolved

Update src/Analyzers/Core/CodeFixes/UseConditionalExpression/Abstract…

6c1d428

…UseConditionalExpressionCodeFixProvider.cs

CyrusNajmabadi commented Jul 26, 2022

View reviewed changes

...s/Core/CodeFixes/UseConditionalExpression/AbstractUseConditionalExpressionCodeFixProvider.cs Outdated Show resolved Hide resolved

Update src/Analyzers/Core/CodeFixes/UseConditionalExpression/Abstract…

516e2d9

…UseConditionalExpressionCodeFixProvider.cs

CyrusNajmabadi commented Jul 26, 2022

View reviewed changes

src/Workspaces/Core/Portable/PublicAPI.Unshipped.txt Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 2 commits July 26, 2022 08:18

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

ba557fa

cleanup

44bd667

CyrusNajmabadi added 2 commits July 26, 2022 08:19

Merge branch 'syntaxEditorWorkspaceService' of https://github.com/Cyr…

70c5649

…usNajmabadi/roslyn into syntaxEditorWorkspaceService

Extract types

634ba81

CyrusNajmabadi enabled auto-merge July 26, 2022 15:22

CyrusNajmabadi added 4 commits July 26, 2022 08:27

Add comments

db833a0

Rename method

4725566

Rename method

bd24b1b

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

e0dc9bd

…ceService

tmat reviewed Jul 26, 2022

View reviewed changes

CyrusNajmabadi merged commit 7b33e50 into dotnet:main Jul 26, 2022

ghost added this to the Next milestone Jul 26, 2022

CyrusNajmabadi deleted the syntaxEditorWorkspaceService branch July 26, 2022 17:51

allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022

This was referenced Jul 28, 2022

[Automated] PRs inserted in VS build $((Release.Artifacts._DD-CB-TestSignVS.BuildNumber) #63032

Closed

[Automated] PRs inserted in VS build main-32727.216 #63034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not go back to Workspace in SyntaxEditor #62910

Do not go back to Workspace in SyntaxEditor #62910

CyrusNajmabadi commented Jul 25, 2022 •

edited

Loading

genlu Jul 25, 2022

CyrusNajmabadi Jul 25, 2022

CyrusNajmabadi Jul 25, 2022

genlu left a comment

CyrusNajmabadi commented Jul 25, 2022

tmat Jul 25, 2022

CyrusNajmabadi Jul 26, 2022

tmat left a comment

CyrusNajmabadi commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

tmat Jul 26, 2022 •

edited

Loading

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

CyrusNajmabadi Jul 26, 2022

tmat Jul 26, 2022

tmat commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

tmat commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

Do not go back to Workspace in SyntaxEditor #62910

Do not go back to Workspace in SyntaxEditor #62910

Conversation

CyrusNajmabadi commented Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

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

tmat Jul 26, 2022 • edited 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

Choose a reason for hiding this comment

tmat commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

tmat commented Jul 26, 2022

CyrusNajmabadi commented Jul 26, 2022

CyrusNajmabadi commented Jul 25, 2022 •

edited

Loading

tmat Jul 26, 2022 •

edited

Loading