Skip to content

Fix syntax tree creation when modifying source generated documents #78343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

davidwengier
Copy link
Member

Fixes dotnet/razor#11787

In my previous PR I naively called syntaxRoot.SyntaxTree to get a tree, but turns out that will just happily lazily create one with CSharpLanguageVersion.Default, which breaks things.

@davidwengier davidwengier requested a review from a team as a code owner April 28, 2025 04:36
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 28, 2025
Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I think there's a bug.

Comment on lines +1398 to +1399
Contract.ThrowIfTrue(sourceText is null && syntaxNode is null);
Contract.ThrowIfTrue(sourceText is not null && syntaxNode is not null);
Copy link
Member

Choose a reason for hiding this comment

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

I like this generally -- it's clear we're always doing exactly one or the other.

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

I like the symmetry now in the code that routes around the text or node now.

return this;
}

var sourceText = newRoot.GetText();
Copy link
Member

Choose a reason for hiding this comment

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

It still feels to me like we could be avoiding this call if we used the CreateTreeWithLazyText helper in DocumentState.cs, but maybe it's fine. I don't think this is actually realizing the text, right? It's just returning a text that walks the tree as needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could call that, yes, but at this point the SourceGeneratedDocumentState still needs a fully realised SourceText at construction time. Making it fully lazy is beyond this PR IMO. I haven't looked at how the SourceText property is used, but there is probably a reasonably easy path to, if not fully lazy, at least lazy-as-long-as-its-synchronous.

@davidwengier
Copy link
Member Author

/azp run

@davidwengier davidwengier enabled auto-merge May 3, 2025 02:45
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@davidwengier davidwengier merged commit b6ec103 into dotnet:main May 3, 2025
27 of 28 checks passed
@davidwengier davidwengier deleted the FixSourceGeneratedDocumentVersions branch May 3, 2025 04:20
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone May 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead VSCode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[VS Code] [Cohosting] Code actions aren't working
2 participants