Trigger IRefactorNotify on Workspace.TryApplyChange #47579
Conversation
ryzngard
force-pushed
the
issues/47048
branch
from
2377877 to
3ddcc47
Compare
There was a problem hiding this comment.
Overall structure looks good. A few specific concerns:
- I'm worried this might be transitioning notifications from the UI thread to a background thread, and this has a deadlock risk.
- Can we also use this to clean up the features that have to clear rename tracking too? Or is that something you want to do later? That might clean up some of the RenameTrackingAction hackery.
- I also hope this doesn't break things now that we're triggering the "OnBefore" notifications after the actual rename instead before. Any worries about that causing issues?
| var solutionPair = await TestActionAsync( | ||
| workspace, | ||
| @"public interface ITest { }", | ||
| action, | ||
| ImmutableArray<TextSpan>.Empty, | ||
| ImmutableArray<TextSpan>.Empty, | ||
| ImmutableArray<TextSpan>.Empty, | ||
| ImmutableArray<TextSpan>.Empty, | ||
| testParameters); | ||
|
|
||
| var oldSolution = solutionPair.Item1; | ||
| var newSolution = solutionPair.Item2; |
There was a problem hiding this comment.
Is it worth breaking all of this into a helper?
| @@ -19,10 +19,6 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.Diagnostics.Naming | |||
| Return (New VisualBasicNamingStyleDiagnosticAnalyzer(), New NamingStyleCodeFixProvider()) | |||
| End Function | |||
|
|
|||
| Protected Overrides Function GetComposition() As TestComposition | |||
There was a problem hiding this comment.
[this is a comment to the file itself.]
Did we want some of the tests that exist on the C# side of things here as well?
| /// <summary> | ||
| /// Called after the rename is applied to the specified documents in the workspace. Return | ||
| /// <see langword="true"/> if this operation succeeded, or <see langword="false"/> if it failed. | ||
| /// </summary> | ||
| bool TryOnAfterGlobalSymbolRenamed(Workspace workspace, IEnumerable<DocumentId> changedDocumentIDs, string replacementText); |
There was a problem hiding this comment.
We've confirmed this isn't used by F# or TypeScript or anything? I don't quite know how they could, but it's my job to ask.
| documentName: "IInterface.vb", | ||
| newDocumentName: "IInterface2.vb"); | ||
| name: "IInterface2.vb"), | ||
| RenameHelpers.MakeSymbolPairs("IInterface", "IInterface2")); |
There was a problem hiding this comment.
Don't forget you can write `New Dictionary(Of String) From{ { "IInterface", "IInterface2" } }" and use collection initializers. On the C# side you might even be able to use target-typed new and omit all sorts of stuff.
There was a problem hiding this comment.
I've had comments in the past that it's less readable, so I've been doing this style 🤷♀️
|
Moving to draft PR while I update this to latest. Still targeting 16.9 |
|
Updated to #55033 |
Change so that we annotate symbols that are being renamed in the Renamer and similar scenarios such that when they are applied with
Workspace.TryApplyChangeswe notify of the symbol change.This allows our public Rename API to have the correct behavior for callers automatically if hosted in a workspace environment where IRefactorNotify is important.
Fixes #47048