-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix syntax tree creation when modifying source generated documents #78343
Conversation
There was a problem hiding this 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.
src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocumentState.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Workspace/Solution/SourceGeneratedDocumentState.cs
Outdated
Show resolved
Hide resolved
…it happened outside
Contract.ThrowIfTrue(sourceText is null && syntaxNode is null); | ||
Contract.ThrowIfTrue(sourceText is not null && syntaxNode is not null); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
src/Workspaces/Core/Portable/Workspace/Solution/SolutionCompilationState.cs
Outdated
Show resolved
Hide resolved
return this; | ||
} | ||
|
||
var sourceText = newRoot.GetText(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
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 withCSharpLanguageVersion.Default
, which breaks things.