Trigger refactor notify on Renamer.RenameDocumentAsync #55033
Conversation
…nges to notify on IRefactorNotify as needed
| /// TryOnAfterGlobalSymbolRenamed. All calls are made on the UI thread, the <see cref="ForegroundThreadAffinitizedObject"/> | ||
| /// is used to ensure this behavior. | ||
| /// </summary> | ||
| public static void TryNotifyChangesSynchronously( |
There was a problem hiding this comment.
er... do we have to be synchronous?
| try | ||
| { | ||
| var refactorNotifyTask = refactorNotifyServices.TryNotifyChangesAsync(workspace, newSolution, oldSolution, threadContext, cancellationToken); | ||
| refactorNotifyTask.Wait(cancellationToken); |
There was a problem hiding this comment.
@sharwell ? i feel like we should be using JTF here if we absolutely must be synchronous.
| } | ||
| } | ||
|
|
||
| private static async Task<ImmutableArray<SyntaxNode>> GatherAnnotatedNodesAsync(Document document, CancellationToken cancellationToken) |
There was a problem hiding this comment.
| private static async Task<ImmutableArray<SyntaxNode>> GatherAnnotatedNodesAsync(Document document, CancellationToken cancellationToken) | |
| private static async Task<ImmutableArray<SyntaxNode>> GatherRenameAnnotatedNodesAsync(Document document, CancellationToken cancellationToken) |
| documentEditor.ReplaceNode(node, | ||
| (currentNode, _) => | ||
| { | ||
| if (RenameSymbolAnnotation.TryAnnotateNode(currentNode, symbol, out var newNode)) |
| .DescendantNodesAndSelf(node => syntaxFacts.IsNamespaceDeclaration(node) || syntaxFacts.IsTypeDeclaration(node)) | ||
| .Where(node => syntaxFacts.IsTypeDeclaration(node)); | ||
|
|
||
| return typeNodes.SelectAsArray(node => (node, semanticModel.GetRequiredDeclaredSymbol(node, cancellationToken))); |
There was a problem hiding this comment.
i don't really get this. you are renaming all the type decls in the file?
| { | ||
| _refactorNotifyServices.TryNotifyChangesSynchronously(this, newSolution, currentSolution, _foregroundObject.ThreadingContext); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
this feels very expensive. TryApplyChanges is a very significant codepath. i'm not sure how i feel doing a large piece of work like this in here. Can this not be done by the feature instead?
| } | ||
|
|
||
| return solutionWithRenameSymbolAnnotations; | ||
| } |
There was a problem hiding this comment.
@CyrusNajmabadi What's the question?
There was a problem hiding this comment.
you wrote rename. i need someone with knowledge of the conflict resolver looking at this since i have no idea how it works :D
|
|
||
| // If the node is already annotated, assume the original annotation is | ||
| // more correct for representing the original symbol | ||
| if (syntaxNode.GetAnnotations(RenameSymbolKind).Any()) |
There was a problem hiding this comment.
when does this happen? i'm confused about the data flow here. when are we trying to annotate an already annotated symbol?
|
Ok, i'm finding this overall concept more complex than i would have expected. For RenameDocument we have a single symbol that is occasionally renamed along with teh doc right? So why can't we just do the rename-doc and rename-symbol, and then just issue a single notification that we renamed the symbol? |
We don't control the action here. We return a modified solution and the caller uses
This also benefits features that aren't just rename doc. Any feature renaming a symbol should invoke this API, and not all of them do. Not only that, this standardizes how we handle |
I general i do not want this on the existing notification API. it's codifyign deep into the bowels of our workspace something realllllly awful. I'd much rather have hacks at the fringes that are done in one-off fashions where we absolutely feel we need this, rather than adding this complex (and potentially expensive) code for every TryApplyChanges. |
It's codifying something into our workspace for VS that is required by VS to behave in it's system well. Awful or not it's an expense we have to pay. Is the worry that it's expensive, sets bad precedence, or something else? Having to do this in each individual feature has historically been problematic. The main benefit I see is we can mark things with annotations that signify intent. I'm definitely open to discussions on how we can decouple this from TryApplyChanges but still provide an easy-to-use annotation system for features. The open question is how to fit this in to scenarios for our public APIs in Renamer that return a |
|
Primarily i don't like the idea of an after-the-fact pass that has to find this info, as opposed to features deliberately telling the system that it did this rename and exactly what the rename was. |
This is essentially the problem though. Every feature knowing that it has to exist in a world where Visual Studio requires it to do something special is a little weird. Not to mention features that use our public rename APIs. We don't provide an equivalent public API for |
ONly features that rename. THose should be finite and easy to enumerate. I don't really see the need for a generalized solution here, especially one that seems to have unbounded complexity finding this information after the fact. |
What solution would you suggest for our public API usage? Just tell external people to deal with it and reconstruct all of the work we did to fit nicely into the VS ecosystem? @jmarolf and @Cosifne had another use case where they would use the Renamer API and love to not have to reconstruct IVSRefactorNotify logic |
Which public API are your referring to? |
THe Renamer API should definitely do this on their behalf. It already knows it is renaming right (that's its purpose). So it can do the notification. Because it is explicitly renaming, it also shoudl know exactly what informaiton to notify without doing a full walk over the solution to find the information after the fact. |
To make sure I am understanding you correctly @CyrusNajmabadi are you arguing that Today, its questionable if anything other than the rename tracking service is calling
Each of these either needs to re-invent a mechanism to call |
|
@jmarolf all rename feature sound go through a centralized system which both handles the code side properly as well as notifying any interested external parties. It should be in a single place, and it should be directed and precise as to what it does. We should not build a system that tries to detect a rename after the fact to notify that that happened. |
Neither. I believe there should be one central place where all this happens and that all code paths that need to rename flow through. |
Alright, so that central place should not be From my perspective I want Roslyn IDE developers to call Renamer apis as they make modifications to the solution snapshot and then on Or am I totally off base here and there isn't a problem with the design, but the implementation is doing too much work etc. and we need to clean that up? |
|
I talked to Andrew a bunch about this today. The primary thing I'm trying to say is that instead of tryapplychanges trying to back infer what happened with a linear sweep of the solution, it should just query the rename apis directly to ask what renames happened to form this solution. I believe the rename apis can store all relevant data and then trivially return that for the notification step without having a costly step that scales with the size of the solution. |
|
@CyrusNajmabadi I'm going to push a change that's incomplete for now, but outlines how I think this will work. Adding IRenameSymbolNotificationService which would cache the strings for symbol rename, and be used for the lookup for IRefactorNotify instead of going through the syntax tree. The last major bits are the implementation of this to make sure it works, and possibly going directly from a SymbolKey string to RQN so we don't have to use a compilation at all. Edit: Could also just store RQN of old and new symbol without SymbolKey involved. |
With RenameDocumentAsync, a specific solution is modified. The caller then applies that solution to the workspace. This presents a challenge in our current IRefactorNotify usage, since it historically has been handled on changes being made with the assumption that it's happening in application of a refactoring or code fix.
This change does two major things:
There will be a followup PR to reduce usage of IRefactorNotify in other areas. Fixes #47048