Move the 'application/validation/resolution' portion of rename OOP. #43592
Conversation
|
|
||
| private TestParameters GetRemoteHostOptions(TestHost host) | ||
| { | ||
| return new TestParameters(options: Option(RemoteHostOptions.RemoteHostTest, host != TestHost.InProcess)); |
There was a problem hiding this comment.
nit: Why do we use a != instead of == TestHost.OutOfProcess here? Just a little harder to parse the negative when reading
There was a problem hiding this comment.
good question. The basic reason (copied from elsewhere) is that this means if we add more out-of-proc options (which happens in some case) then all of them will run OOP since they're not the .InProcess value.
| @@ -43,8 +44,7 @@ public async Task<IInlineRenameReplacementInfo> GetReplacementsAsync(string repl | |||
| var conflicts = await _renameLocationSet.ResolveConflictsAsync( | |||
| _renameInfo.GetFinalSymbolName(replacementText), nonConflictSymbols: null, cancellationToken: cancellationToken).ConfigureAwait(false); | |||
|
|
|||
| if (conflicts.ErrorMessage != null) | |||
| throw new ArgumentException(conflicts.ErrorMessage); | |||
| Contract.ThrowIfTrue(conflicts.ErrorMessage != null); | |||
There was a problem hiding this comment.
Should we add Contract.ThrowIfNotNull?
There was a problem hiding this comment.
i thought about it.. but it didn't really seem important/common enough to warrant it :)
src/Features/Core/Portable/EncapsulateField/AbstractEncapsulateFieldService.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/EncapsulateField/EncapsulateFieldResult.cs
Outdated
Show resolved
Hide resolved
|
|
||
| internal static class RemoteUtilities | ||
| { | ||
| public static async Task<DocumentTextChanges> GetDocumentTextChangesAsync( |
There was a problem hiding this comment.
Why are these unique to remote work?
There was a problem hiding this comment.
because normally we can just pass around solutions when in the same process. But for remote/oop we want to be able to say "here's the solution result of this operation" and we do that by passing along hte text changes necessary to convert the original solutoin to the same one that the OOP side see.s
| return builder.ToImmutable(); | ||
| } | ||
|
|
||
| public static async Task<Solution> UpdateSolutionAsync( |
There was a problem hiding this comment.
When would you use this over something like UpdateSolutionIfPossible from the remote workspace? http://sourceroslyn.io/#Microsoft.CodeAnalysis.Remote.Workspaces/Services/RemoteWorkspace.cs,115
There was a problem hiding this comment.
so, UpdateSolutionIfPossible is for updating the Remote workspaces's .CurrentSolution to match what is on the host side.
In the case here we don't want to actually update any workspaces. this is just allowing certain operations (which result in a new solutoin snapshot) to be able to do that in an OOP context.
| public DocumentId[] DocumentIds; | ||
| public SerializableRelatedLocation[] RelatedLocations; | ||
| public ImmutableArray<(DocumentId, ImmutableArray<TextChange>)> DocumentTextChanges; | ||
| public (DocumentId, ImmutableArray<(TextSpan oldSpan, TextSpan newSpan)>)[] DocumentToModifiedSpansMap; |
There was a problem hiding this comment.
Is there no serializable map type we can use? What's the benefit of array of tuples here?
There was a problem hiding this comment.
Is there no serializable map type we can use?
There is not unfortunately. That's because this all goes through JSON. And json can only represent maps using strings (and DocIds are not rountrippable to a simple string). So this is effectively just serializing out all the key/values to tuples since that is something that can go through json.
…eFieldService.cs Co-Authored-By: Andrew Hall <ryzngard@live.com>
ghost
added this to the 
Followup to #43554 and #43573. Those should go in first before reviewing.Rename is composed of two parts. The first finds the all the places to rename (and only needs to run once). The second checks for conflicts at all those locations whenever the user updates the name (so it may run many times). This PR moves the second part of this out of process.
This also moved encapsulate-field OOP.
This can be reviewed one commit at a time.