Simplify error handling and notifications in rename. #61515

CyrusNajmabadi · 2022-05-25T19:25:16Z

Followup to #61512.

CyrusNajmabadi · 2022-05-25T19:26:07Z

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

+                            var notificationService = _workspace.Services.GetService<INotificationService>();
+                            notificationService.SendNotification(
+                                error, EditorFeaturesResources.Rename_Symbol, NotificationSeverity.Error);
+                        }


single place in inline-rename, and single place in rename-code-action where we notify the user of an error we couldnt' handle.

CyrusNajmabadi · 2022-05-25T19:27:42Z

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

-                    // these calls can't be cancelled and must be allowed to complete.
-                    var root = renameTrackingSolutionSet.RenamedSolution.GetDocument(docId).GetSyntaxRootSynchronously(CancellationToken.None);
-                    finalSolution = finalSolution.WithDocumentSyntaxRoot(docId, root);
+                    RenameTrackingDismisser.DismissRenameTracking(workspace, changedDocuments);
                }


fairly certain not being in a finally here was a bug. the return at https://github.com/dotnet/roslyn/pull/61515/files?diff=unified&w=1#diff-444ebec94ff45aa833894aec3cf66ed4c3f13f5f4fc661c5b5ce17d730597d48L140 meant we could bail out without dismissing our rename tracking state.

CyrusNajmabadi · 2022-05-25T19:47:23Z

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

-                    NotificationSeverity.Error);
-                return;
-            }
+                return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_was_cancelled_or_is_not_valid);


super unfortunate that this external-notification api we have doesn't have any way for the component returning 'false' to say what the issue is. So we have to give this unhelpful message here.

CyrusNajmabadi · 2022-05-25T19:47:44Z

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

@@ -837,7 +844,10 @@ private void ApplyRename(Solution newSolution, IUIThreadOperationContext operati
                finalSolution = finalSolution.WithDocumentName(id, newDocument.Name);
            }

-            if (_workspace.TryApplyChanges(finalSolution))
+            if (!_workspace.TryApplyChanges(finalSolution))
+                return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_could_not_complete_due_to_external_change_to_workspace);


changed from the silent failure approach from ebfore to at least having a notification about what happened.

CyrusNajmabadi · 2022-05-25T19:48:25Z

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

-                        EditorFeaturesResources.Rename_Symbol,
-                        NotificationSeverity.Information);
-                }
+                    return (NotificationSeverity.Information, EditorFeaturesResources.Rename_operation_was_not_properly_completed_Some_file_might_not_have_been_updated);


kept this informational. but it certainly seems weird that we're effectivley saying "hey... heads up... things might be bad, but we have no idea why or waht you can do about it" :(

CyrusNajmabadi · 2022-05-25T19:49:28Z

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

@@ -217,18 +229,24 @@ private void UpdateWorkspaceForResetOfTypedIdentifier(Workspace workspace, Solut
                localUndoTransaction.AddUndo(undoPrimitiveBefore);

                if (!workspace.TryApplyChanges(newSolution))
-                {
-                    Contract.Fail("Rename Tracking could not update solution.");


fairly certain this would just corrupt state in the rename engine (due to the lack of finallys).

CyrusNajmabadi · 2022-05-25T19:50:54Z

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

+                    return (NotificationSeverity.Error, EditorFeaturesResources.Rename_operation_could_not_complete_due_to_external_change_to_workspace);
+
+                // If we successfully update the workspace then make sure the undo transaction is committed and is
+                // always able to undo anything any other external listener did.

                // Never resume tracking session on redo
                var undoPrimitiveAfter = new UndoPrimitive(_stateMachine.Buffer, trackingSessionId, shouldRestoreStateOnUndo: false);
                localUndoTransaction.AddUndo(undoPrimitiveAfter);

                localUndoTransaction.Complete();


not sure waht the best logic is to have here. previously, if applying failed, we would throw in Contract.Fail. so this code woudl never run. i'm preserving that logic here... which i think is desirable. if we didn't change the workspace we don't need an undo added to the stack.

but i'm not sure. we coudl always add these no matter what, but i'm not sure the implication of that.

we will at least add these on success, which was our logic from before.

@jasonmalinowski ?

CyrusNajmabadi · 2022-05-25T19:51:34Z

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

+                finally
+                {
+                    // If we successfully update the workspace then make sure the undo transaction is committed and is
+                    // always able to undo anything any other external listener did.


same concept as before. if the workspace application fails, we don't add these. but if it succeeds, we always add this, regardless of waht our external-notification system does.

ryzngard · 2022-05-25T21:51:59Z

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

+                RenameService.ActiveSession = null;
+            }
+
+            void LogPositionChanged(object sender, CaretPositionChangedEventArgs e)


We can get rid of the LogPositionChanged portion now. It is being resolved

Oh? Do tell, I'm interested :-)

CyrusNajmabadi · 2022-05-25T22:56:02Z

/azp run

azure-pipelines · 2022-05-25T22:56:23Z

Azure Pipelines successfully started running 4 pipeline(s).

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

…vider.RenameTrackingCommitter.cs

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

…vider.RenameTrackingCommitter.cs

dibarbet · 2022-05-25T20:30:54Z

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs

-                        EditorFeaturesResources.Rename_Symbol,
-                        NotificationSeverity.Information);
+                    if (!_refactorNotifyServices.TryOnAfterGlobalSymbolRenamed(workspace, changedDocuments, symbol, newName, throwOnFailure: false))
+                        return (NotificationSeverity.Information, EditorFeaturesResources.Rename_operation_was_not_properly_completed_Some_file_might_not_have_been_updated);


what is the difference in experience if an error vs information is returned? Just thinking if something fails wouldn't that always be an error, or is failure here expected?

I believe it affects the glyph we show in the notification window. i agree on this being wonky, but i decided to preserve semantics here vs change things...

Unify error handling in rename

645a140

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

CyrusNajmabadi commented May 25, 2022

View reviewed changes

CyrusNajmabadi added 4 commits May 25, 2022 12:28

Add comment

06e9da9

message severities

a3599eb

Restore

d8d517b

Simplify

fcf7da2

CyrusNajmabadi commented May 25, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into renameErrorHandling

116376e

CyrusNajmabadi requested review from dibarbet, ryzngard and jasonmalinowski May 25, 2022 19:52

CyrusNajmabadi marked this pull request as ready for review May 25, 2022 19:52

CyrusNajmabadi requested a review from a team as a code owner May 25, 2022 19:52

ryzngard reviewed May 25, 2022

View reviewed changes

CyrusNajmabadi and others added 2 commits May 25, 2022 16:00

Merge remote-tracking branch 'upstream/main' into renameErrorHandling

5b5a543

Merge remote-tracking branch 'upstream/main' into renameErrorHandling

d6c09fb

CyrusNajmabadi requested a review from ryzngard May 26, 2022 04:03

CyrusNajmabadi commented May 26, 2022

View reviewed changes

src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs Outdated Show resolved Hide resolved

Update src/EditorFeatures/Core/InlineRename/InlineRenameSession.cs

1fd3092

CyrusNajmabadi commented May 26, 2022

View reviewed changes

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs Outdated Show resolved Hide resolved

Update src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerPro…

752c9f2

…vider.RenameTrackingCommitter.cs

CyrusNajmabadi commented May 26, 2022

View reviewed changes

src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCommitter.cs Outdated Show resolved Hide resolved

Update src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerPro…

f0ee857

…vider.RenameTrackingCommitter.cs

jmarolf approved these changes May 26, 2022

View reviewed changes

dibarbet approved these changes May 26, 2022

View reviewed changes

ryzngard approved these changes May 26, 2022

View reviewed changes

CyrusNajmabadi merged commit 9de4073 into dotnet:main May 26, 2022

CyrusNajmabadi deleted the renameErrorHandling branch May 26, 2022 18:02

ghost added this to the Next milestone May 26, 2022

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify error handling and notifications in rename. #61515

Simplify error handling and notifications in rename. #61515

CyrusNajmabadi commented May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi May 25, 2022

ryzngard May 25, 2022

CyrusNajmabadi May 25, 2022

CyrusNajmabadi commented May 25, 2022

azure-pipelines bot commented May 25, 2022

dibarbet May 25, 2022

CyrusNajmabadi May 26, 2022

Simplify error handling and notifications in rename. #61515

Simplify error handling and notifications in rename. #61515

Conversation

CyrusNajmabadi commented May 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 25, 2022

azure-pipelines bot commented May 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment