Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6c55132
Add parameter to allow opting in to renames in source generated docum…
davidwengier Jun 16, 2025
8a77f68
Allow source generated documents to be processed
davidwengier Jun 16, 2025
8f06ff5
Call GetRequiredDocumentAsync so that source generated documents can …
davidwengier Jun 16, 2025
58ca7fd
Make sure source generated documents are returned in the results
davidwengier Jun 16, 2025
d4aa226
Pass in project Id since we know it already anyway
davidwengier Jun 16, 2025
5a684cf
Make sure source generated documents exist so they can be updated
davidwengier Jun 16, 2025
0623c61
Modernize tests
davidwengier Jun 16, 2025
3721ae9
Add new tests
davidwengier Jun 16, 2025
4d56fb6
Allow better filtering of non-Razor source generated documents in future
davidwengier Jun 18, 2025
c7ef883
PR feedback
davidwengier Jun 18, 2025
0a3e34b
Add generated document contract check
davidwengier Jun 20, 2025
e9b4dd1
Merge remote-tracking branch 'upstream/main' into RazorCohostRename
davidwengier Jun 23, 2025
b12b57e
Rename allowRenameInGeneratedDocument to includeSourceGenerated
davidwengier Jun 27, 2025
2c4818b
Move flag to options
davidwengier Jun 27, 2025
0e534c0
Cleanup
davidwengier Jun 27, 2025
3114486
Merge remote-tracking branch 'upstream/main' into RazorCohostRename
CyrusNajmabadi Jun 27, 2025
a0ae77e
Simplify
CyrusNajmabadi Jun 27, 2025
8ef6ace
Pass options
CyrusNajmabadi Jun 27, 2025
ea30bb5
Pass options
CyrusNajmabadi Jun 27, 2025
4868f75
Pass options
CyrusNajmabadi Jun 27, 2025
26b72b3
Extract method for handling source generated documents
davidwengier Jun 30, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ protected AbstractEditorInlineRenameService(IEnumerable<IRefactorNotifyService>

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 'false' here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place that opts in to renames in generated files is the one entry point Razor uses. For now anyway.

if (symbolicInfo.LocalizedErrorMessage != null)
return new FailureInlineRenameInfo(symbolicInfo.LocalizedErrorMessage);

Expand Down
7 changes: 4 additions & 3 deletions src/Features/Core/Portable/Rename/SymbolicRenameInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,13 +111,13 @@ public string GetFinalSymbolName(string replacementText)
}

public static async Task<SymbolicRenameInfo> GetRenameInfoAsync(
Document document, int position, CancellationToken cancellationToken)
Document document, int position, bool includeSourceGenerated, CancellationToken cancellationToken)
{
var triggerToken = await GetTriggerTokenAsync(document, position, cancellationToken).ConfigureAwait(false);
if (triggerToken == default)
return new SymbolicRenameInfo(FeaturesResources.You_must_rename_an_identifier);

return await GetRenameInfoAsync(document, triggerToken, cancellationToken).ConfigureAwait(false);
return await GetRenameInfoAsync(document, triggerToken, includeSourceGenerated, cancellationToken).ConfigureAwait(false);
}

private static async Task<SyntaxToken> GetTriggerTokenAsync(Document document, int position, CancellationToken cancellationToken)
Expand All @@ -131,6 +131,7 @@ private static async Task<SyntaxToken> GetTriggerTokenAsync(Document document, i
private static async Task<SymbolicRenameInfo> GetRenameInfoAsync(
Document document,
SyntaxToken triggerToken,
bool includeSourceGenerated,
CancellationToken cancellationToken)
{
var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
Expand Down Expand Up @@ -221,7 +222,7 @@ private static async Task<SymbolicRenameInfo> GetRenameInfoAsync(
var solution = document.Project.Solution;
var sourceDocument = solution.GetRequiredDocument(location.SourceTree);

if (sourceDocument is SourceGeneratedDocument)
if (!includeSourceGenerated && sourceDocument is SourceGeneratedDocument)
{
// The file is generated so doesn't count towards valid spans
// we can edit.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
using Microsoft.CodeAnalysis.LanguageServer.Handler;
using Microsoft.CodeAnalysis.NavigateTo;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.SpellCheck;
using Microsoft.CodeAnalysis.Tags;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -388,15 +389,15 @@ public static LSP.Range TextSpanToRange(TextSpan textSpan, SourceText text)
/// Compute all the <see cref="LSP.TextDocumentEdit"/> for the input list of changed documents.
/// Additionally maps the locations of the changed documents if necessary.
/// </summary>
public static async Task<LSP.TextDocumentEdit[]> ChangedDocumentsToTextDocumentEditsAsync<T>(IEnumerable<DocumentId> changedDocuments, Func<DocumentId, T> getNewDocumentFunc,
Func<DocumentId, T> getOldDocumentFunc, IDocumentTextDifferencingService? textDiffService, CancellationToken cancellationToken) where T : TextDocument
public static async Task<LSP.TextDocumentEdit[]> ChangedDocumentsToTextDocumentEditsAsync(IEnumerable<DocumentId> changedDocuments, Solution newSolution,
Solution oldSolution, IDocumentTextDifferencingService? textDiffService, CancellationToken cancellationToken)
{
using var _ = ArrayBuilder<(DocumentUri Uri, LSP.TextEdit TextEdit)>.GetInstance(out var uriToTextEdits);

foreach (var docId in changedDocuments)
{
var newDocument = getNewDocumentFunc(docId);
var oldDocument = getOldDocumentFunc(docId);
var newDocument = await newSolution.GetRequiredDocumentAsync(docId, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false);
var oldDocument = await oldSolution.GetRequiredDocumentAsync(docId, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false);
Comment on lines +399 to +400
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only one caller of this method (now?) so the abstraction of using delegates seemed unnecessary.


var oldText = await oldDocument.GetValueTextAsync(cancellationToken).ConfigureAwait(false);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(LSP.PrepareRenamePar
var position = await document.GetPositionFromLinePositionAsync(linePosition, cancellationToken).ConfigureAwait(false);

var symbolicRenameInfo = await SymbolicRenameInfo.GetRenameInfoAsync(
document, position, cancellationToken).ConfigureAwait(false);
document, position, includeSourceGenerated: false, cancellationToken).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question. basically, it's not apparent to me at callsites what teh right value is here.

if (symbolicRenameInfo.IsError)
return null;

Expand Down
23 changes: 14 additions & 9 deletions src/LanguageServer/Protocol/Handler/Rename/RenameHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,24 @@ internal sealed class RenameHandler() : ILspServiceDocumentRequestHandler<LSP.Re
public TextDocumentIdentifier GetTextDocumentIdentifier(RenameParams request) => request.TextDocument;

public Task<WorkspaceEdit?> HandleRequestAsync(RenameParams request, RequestContext context, CancellationToken cancellationToken)
=> GetRenameEditAsync(context.GetRequiredDocument(), ProtocolConversions.PositionToLinePosition(request.Position), request.NewName, cancellationToken);
=> GetRenameEditAsync(context.GetRequiredDocument(), ProtocolConversions.PositionToLinePosition(request.Position), request.NewName, includeSourceGenerated: false, cancellationToken);

internal static async Task<WorkspaceEdit?> GetRenameEditAsync(Document document, LinePosition linePosition, string newName, CancellationToken cancellationToken)
internal static async Task<WorkspaceEdit?> GetRenameEditAsync(Document document, LinePosition linePosition, string newName, bool includeSourceGenerated, CancellationToken cancellationToken)
{
var oldSolution = document.Project.Solution;
var position = await document.GetPositionFromLinePositionAsync(linePosition, cancellationToken).ConfigureAwait(false);

var symbolicRenameInfo = await SymbolicRenameInfo.GetRenameInfoAsync(
document, position, cancellationToken).ConfigureAwait(false);
document, position, includeSourceGenerated, cancellationToken).ConfigureAwait(false);
if (symbolicRenameInfo.IsError)
return null;

var options = new SymbolRenameOptions(
RenameOverloads: false,
RenameInStrings: false,
RenameInComments: false,
RenameFile: false);
renameOverloads: false,
renameInStrings: false,
renameInComments: false,
renameFile: false,
renameInSourceGeneratedDocuments: includeSourceGenerated);

var renameLocationSet = await Renamer.FindRenameLocationsAsync(
oldSolution,
Expand All @@ -70,14 +71,18 @@ internal sealed class RenameHandler() : ILspServiceDocumentRequestHandler<LSP.Re
// Then we can just take the text changes from the first document to avoid returning duplicate edits.
renamedSolution = await renamedSolution.WithMergedLinkedFileChangesAsync(oldSolution, solutionChanges, cancellationToken: cancellationToken).ConfigureAwait(false);
solutionChanges = renamedSolution.GetChanges(oldSolution);

Contract.ThrowIfTrue(!includeSourceGenerated && !renamedSolution.CompilationState.FrozenSourceGeneratedDocumentStates.IsEmpty, "Renaming in generated documents is not allowed, but there are changes in source generated documents.");

var changedDocuments = solutionChanges
.GetProjectChanges()
.SelectMany(p => p.GetChangedDocuments(onlyGetDocumentsWithTextChanges: true))
.GroupBy(docId => renamedSolution.GetRequiredDocument(docId).FilePath, StringComparer.OrdinalIgnoreCase).Select(group => group.First());
.GroupBy(docId => renamedSolution.GetRequiredDocument(docId).FilePath, StringComparer.OrdinalIgnoreCase).Select(group => group.First())
.Concat(solutionChanges.GetExplicitlyChangedSourceGeneratedDocuments());

var textDiffService = renamedSolution.Services.GetRequiredService<IDocumentTextDifferencingService>();

var documentEdits = await ProtocolConversions.ChangedDocumentsToTextDocumentEditsAsync(changedDocuments, renamedSolution.GetRequiredDocument, oldSolution.GetRequiredDocument,
var documentEdits = await ProtocolConversions.ChangedDocumentsToTextDocumentEditsAsync(changedDocuments, renamedSolution, oldSolution,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of questions here

  1. Are you expecting the renamed locations in the generated files to be mapped here via the span mapping service? IIRC we do that.
  2. 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good questions! I'll try to be brief in my answers, but things get complicated quick :)

  1. 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.
  2. 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.

Copy link
Member Author

@davidwengier davidwengier Jun 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

textDiffService, cancellationToken).ConfigureAwait(false);

return new WorkspaceEdit { DocumentChanges = documentEdits };
Expand Down
Loading
Loading