Allow rename to (optionally) process source generated documents #78984
Conversation
You can ignore this commit, purely mechanical changes
| var newDocument = await newSolution.GetRequiredDocumentAsync(docId, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false); | ||
| var oldDocument = await oldSolution.GetRequiredDocumentAsync(docId, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
There is only one caller of this method (now?) so the abstraction of using delegates seemed unnecessary.
There was a problem hiding this comment.
View with whitespace off, most of the changes were just modernizing the file to raw string literals
| // scenarios that wanted it. | ||
| if (documentId.IsSourceGenerated) | ||
| { | ||
| _ = await partiallyRenamedSolution.GetRequiredDocumentAsync(documentId, includeSourceGenerated: true, _cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When I did the changes to allow WithDocumentXXX to work on source generated documents, I made it only work for already generated documents because Razor could guarantee that, and it seemed less risky for any other scenario. I didn't appreciate, however, how rename works and how much it likes to fork solutions.
This line of code is the downside of that decision, where this (and the other spot it appears) unfortunately has to force the generated document to be created, just to change it again. For now at least I think it's better to do this, than to allow the WithDocumentXXX methods to work on unrealised source generated documents. As per the comment in the file, this should only happen when Razor explicitly opts in to rename in source generated documents.
There was a problem hiding this comment.
i don't love this. but i don't mind it either, and i have no better solution. nad the fallout is contained and the comment is clear enough.
| var projectOpt = conflictResolution.CurrentSolution.GetProject(renamedSymbol.ContainingAssembly, cancellationToken); | ||
| var projectOpt = conflictResolution.CurrentSolution.GetProject(projectId); |
There was a problem hiding this comment.
The GetProject method that takes an assembly symbol uses the compilation tracker to work, and with source generators the compilation tracker stuff gets odd. Fortunately, the one caller of this method already has the project Id to hand, so it was easiest to just pass it in.
It's possible my understanding of the code is incorrect though, so maybe this is a bad change, that just happens to not be covered by any existing tests?
There was a problem hiding this comment.
looking up project by assembly always makes me nervous. are you sure those that ProjectId refers to the project the current symbol is in, not hte original symbol being renamed? this matters for things likes cascades across projects.
There was a problem hiding this comment.
basically, i love this. but needs a bit of an audit just to make firmly sure this project id is what we expect.
dibarbet
left a comment
dibarbet
left a comment
There was a problem hiding this comment.
someone more familiar with the rename internals should look at that part
| var textDiffService = renamedSolution.Services.GetRequiredService<IDocumentTextDifferencingService>(); | ||
|
|
||
| var documentEdits = await ProtocolConversions.ChangedDocumentsToTextDocumentEditsAsync(changedDocuments, renamedSolution.GetRequiredDocument, oldSolution.GetRequiredDocument, | ||
| var documentEdits = await ProtocolConversions.ChangedDocumentsToTextDocumentEditsAsync(changedDocuments, renamedSolution, oldSolution, |
There was a problem hiding this comment.
A couple of questions here
- Are you expecting the renamed locations in the generated files to be mapped here via the span mapping service? IIRC we do that.
- How do you know if the rename is touching razor generated files, vs other generated files? Do you exclude files you don't know about?
There was a problem hiding this comment.
Very good questions! I'll try to be brief in my answers, but things get complicated quick :)
- In the context of this PR, no I'm not expecting that. Razor will remap the results when they come through. Additionally in cohosting there is no span mapping service at all (yet?). Additionally additionally, the span mapping that this method does is only helping in IDE scenarios where Roslyn is using LSP for rename. Those will regress in cohosting for now, and I'm leaving that as a separate fix.
- At the moment no effort is made to constrain the documents, though that might have to come later for other reasons. I should check that the editor doesn't crash or do something horrible, but in general I don't think creating an edit for a source generated document is a terrible thing, because it will just be thrown away at some point, and the file will be regenerated. That's essentially what happens with the Razor source generated documents too, we don't ever expect to apply the edits, we just need Roslyn to tell us what they would be.
There was a problem hiding this comment.
Update to answer 2: Razor filters out any edits to source generated documents that aren't Razor documents by virtue of not being able to answer the question "Which Razor file is this generated document for?", so edits for other source generators will never escape into the real world. Going to push a commit to this PR to allow us to make this check more robust in future.
src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs
Outdated
Show resolved
Hide resolved
src/LanguageServer/Protocol/Handler/Rename/PrepareRenameHandler.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/ConflictEngine/MutableConflictResolution.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/ConflictEngine/RenamedSpansTracker.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Tentatively approving. morally i'm ok with this. and this really is mostly just an option flowing through.
A couple of things i would like to see:
- naming. I prefer
includeSourceGenerated - options. we have an options type... it feels like thsi sould be part of it.
- minor cleanup suggestions.
- get sign off from davidb on the LSP side. i don't know that stuff well and i want him ok with it. the rest of hte workspace/rename side seems fine.
| public async Task<IInlineRenameInfo> GetRenameInfoAsync(Document document, int position, CancellationToken cancellationToken) | ||
| { | ||
| var symbolicInfo = await SymbolicRenameInfo.GetRenameInfoAsync(document, position, cancellationToken).ConfigureAwait(false); | ||
| var symbolicInfo = await SymbolicRenameInfo.GetRenameInfoAsync(document, position, includeSourceGenerated: false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
The only place that opts in to renames in generated files is the one entry point Razor uses. For now anyway.
|
|
||
| var symbolicRenameInfo = await SymbolicRenameInfo.GetRenameInfoAsync( | ||
| document, position, cancellationToken).ConfigureAwait(false); | ||
| document, position, includeSourceGenerated: false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
same question. basically, it's not apparent to me at callsites what teh right value is here.
src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/ConflictEngine/ConflictResolver.Session.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/ConflictEngine/MutableConflictResolution.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Rename/SymbolicRenameLocations.ReferenceProcessing.cs
Outdated
Show resolved
Hide resolved
|
Thanks all! |
Fixes #11831 This needs dotnet/roslyn#78984 to go in first, or the test will fail. Which you should see in this PR!
4 participants
Part of dotnet/razor#11831
Razor side that completes the fix is dotnet/razor#11952
I am well out of my depth with rename, and have commented on a couple of spots that might be contentious, or are otherwise worth noting.