Conversation
ghost
added
the
|
|
||
| if (_trackedSession is not null) | ||
| { | ||
| _trackedSession.ReplacementTextChanged -= InlineRenameSession_ReplacementTextChanged; |
There was a problem hiding this comment.
This fix is unrelated. I noticed while debugging and it makes the code safer
| if (!_renameInfo.TryOnBeforeGlobalSymbolRenamed(_workspace, changedDocumentIDs, this.ReplacementText)) | ||
| return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_was_cancelled_or_is_not_valid); | ||
|
|
||
| if (finalSolution.WorkspaceVersion != _workspace.CurrentSolution.WorkspaceVersion) |
There was a problem hiding this comment.
Rather than just checking this, move this handling into the if (!TryApplyChanges()) path and you could then call the TryApplyChanges a second time.
There was a problem hiding this comment.
if it fails for a different reason, do we want to call TryApplyChanges twice?
There was a problem hiding this comment.
TryApplyChanges will return false if you hit the version issue, and will throw for anything else.
There was a problem hiding this comment.
note: We special case tryApplyChanges returning false in ApplyChangesOperation (https://github.com/dotnet/roslyn/blob/main/src/Workspaces/Core/Portable/CodeActions/Operations/ApplyChangesOperation.cs#L50). Not sure if that would be helpful here in any way.
The general idea though is that we still attempt to make the change, as long as it's just text changes to files taht themselves haven't been changed.
| this.initialState = null; | ||
| this.currentState = null; | ||
|
|
||
| if (_trackedSession is not null) |
There was a problem hiding this comment.
This part seems a bit strange to me: I would have expected the only thing that could possibly hold this type would have been the session -- once the session has been let go, this would have fallen away too. Did you see something being held being fixed by this, or you generally noticed this wasn't being updated? Because honestly I'd actually just delete this Disconnect method -- if anything leaks this type directly, we should know that up front it seems?
There was a problem hiding this comment.
_trackedSession only is used to hook up to ReplacementTextChanged, which then checks the currentState. If Disconnect is called prior to the last ReplacementTextChanged event we would nullref in InlineRenameSession_ReplacementTextChanged.
I agree lifetime here is a bit weird. Can pull this out. It was just something I noticed while debugging, did hit a nullref in InlineRenameSession_ReplacementTextChanged, so I fixed it. If we're resetting initial and current state, we no longer care about handling ReplacementTextChanged. Logically that makes sense to me.
There was a problem hiding this comment.
(there is a pattern that some parts of Roslyn tried doing which was "if I'm leaked as an object, don't leak other stuff", but it's not a well-liked pattern generally)
There was a problem hiding this comment.
(that second comment I posted after a call, but GitHub helpfully didn't show your reply yet)
There was a problem hiding this comment.
did hit a nullref in InlineRenameSession_ReplacementTextChanged
Is the null ref just because we're nulling everything else out there? Because that'd be a stronger argument for "just delete" if that's practical.
There was a problem hiding this comment.
It is, and we could do that. I'm happy to do a separate change for that instead of piling into here
Co-authored-by: Jason Malinowski <jason@jason-m.com>
| if (!_workspace.TryApplyChanges(finalSolution)) | ||
| return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_could_not_complete_due_to_external_change_to_workspace); | ||
| { | ||
| // If the workspace changed in TryOnBeforeGlobalSymbolRenamed retry, this prevents rename from failing for cases | ||
| // where text changes to other files or workspace state change doesn't impact the text changes being applied. | ||
| Logger.Log(FunctionId.Rename_TryApplyRename_WorkspaceChanged, message: null, LogLevel.Information); | ||
| finalSolution = CalculateFinalSolutionSynchronously(newSolution, _workspace, changedDocumentIDs, cancellationToken); | ||
|
|
||
| if (!_workspace.TryApplyChanges(finalSolution)) | ||
| return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_could_not_complete_due_to_external_change_to_workspace); | ||
| } |
There was a problem hiding this comment.
💭 Can we not check _workspace.CurrentSolution explicitly to avoid having two calls to TryApplyChanges?
There was a problem hiding this comment.
From my understanding this is preferred, and TryApplyChanges will throw for all cases except workspace versions being different
5 participants
When applying our changes for rename we switch to a background thread to compute the text/syntax changes and apply them. Then we call TryOnBeforeGlobalSymbolRenamed to indicate we're about to rename the item. For MAUI, this causes InvisibleEditors opening/closing and causes a mutation in the workspace, which updates the
Workspace.CurrentSolution.WorkspaceVersionmaking our calculated solution invalid to apply.The fix here is to recompute the changes on the mutated workspace if needed. Adding telemetry here to make sure we understand the impact, but the guess is that this keeps most users on the non UI blocking expensive work, and some users having an expensive recompute.
Fixes #65397