-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add support for source generators in Razor compiler #29916
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
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 the new compiler introduces some additional nullability warnings that you would need to address. But otherwise this looks good.
FYI @NTaylorMullen
@@ -130,9 +130,11 @@ public string FilePathWithoutExtension | |||
} | |||
} | |||
|
|||
internal RazorSourceDocument SourceDocument { get; set; } |
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.
Hmm, this is concerning. The intent of RazorProjectItem is to be decoupled from a SourceDocument entirely. Why do you need this here? Typically we just re-create a project item.
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.
One of the things that helped with perf was having a RazorSourceLineCollection
that was backed by the Roslyn SourceText
document (because that's what we're provided in a source generator). Here's what that looks like: https://github.com/dotnet/aspnetcore/pull/28807/files#diff-04e8fa11176d9e8cdfa5d0edeeb3ba7c1c6d20683201775277a3215a1fcb1a3f.
If I'm not mistaken, there isn't an API that lets you affect how a RazorProjectItem gets turned in to a RazorSourceDocument. We could create one where the default implementation reads from the stream and produces a source document and the source generator configures it with a custom provider. This one felt easier to implement as we did not have to introduce yet another concept, but you have more context here, so open to suggestions.
@@ -193,6 +193,11 @@ public static RazorSourceDocument ReadFrom(RazorProjectItem projectItem) | |||
filePath = projectItem.FilePath; | |||
} | |||
|
|||
if (projectItem.SourceDocument 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.
Why not just re-create a source document?
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.
This and the change above are designed to work in conjunction with some work we've done to support source generators.
We have a new SourceGeneratorProjectItem
(ref) type that gets constructed from the list of files that Roslyn provides in a generator's compilation context (ref).
These additional files are provided as SourceText
s. We have a SourceTextRazorSourceDocument
type (ref) that we create from the SourceText
.
Here we are updating the ReadFrom
method to directly use the SourceDocument
that we compute from the SourceText
provided by Roslyn instead of reading in the file contents from a stream.
You can view a hacky workaround I used to get around this here by reading the string of the SourceText
into a stream but this isn't ideal.
Hope this explanation was clear. Thoughts?
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.
Thanks for the clarification, definitely helps piece it all together a bit more. It's been a while since I've dug through the code but why can't we have the project item read directly from the Roslyn source text?
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 did create a ProjectItem implementation that reads directly from the Roslyn source text (ref). But currently, the project engine computes the source document (ref) from a project item by reading from a stream (ref).
We can try implementing an overload of ReadFrom
that takes in a SourceGeneratorProjectItem
public static RazorSourceDocument ReadFrom(SourceGeneratorProjectItem projectItem)
{
return new SourceGeneratorSourceDocument(projectItem.AdditionalText); // or similiar
}
But that would add source generator-specific logic in the compiler.
An alternative is to create some sort of abstraction where in a custom ProjectItem
can define how source documents should be read from it similar to @pranavkm's recommendation above.
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.
An alternative is to create some sort of abstraction where in a custom
ProjectItem
can define how source documents should be read from it similar to @pranavkm's recommendation above.
Ya I think that'd be better. It'd be a more defined route of translating from one type to another.
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.
Yep! Will create an issue to track this. We want to get the initial E2E with source generators working then refine things.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@captainsafia what in particular would you like me to review❔ If it's just the CODEOWNERS file, no objections from me /fyi dotnet/install-scripts#136 is blocking this PR at the moment |
Changing the PublicAPI.Shipped.txt files outside an X.Y.0 release is never the right idea. @JunTaoLuo is scrambling to fix up the places where we've made this mistake. Please don't make it worse. If necessary, change the PublicAPI.Unshipped.txt files to remove changed APIs. The syntax is to copy the line from PublicAPI.Shipped.txt (again, not changing that file) into PublicAPI.Unshipped.txt and add a |
@dougbu in this case it's changing the nullability annotation on a shipped API. We've made a number of those so far - aspnet/Announcements#444. |
I know. The problem isn't the nullabiltity changes but how they were handled in the API baseline files. PublicAPI.Shipped.txt files should change only immediately after X.Y.0 ships. |
9638fe1
to
0ff5f15
Compare
0ff5f15
to
e3a686b
Compare
…e#29916) * Add support for source generators in Razor compiler * Fix PublicAPI text files Commit migrated from dotnet/aspnetcore@307e1072b62a
Microsoft.CodeAnalysis.CSharp
that we consume otherwise we get some downstream assembly resolution issues in the version ofMicrosoft.AspNetCore.Razor.Extensions
that we use in the SDK repo.rzc
assembly name over in the sdk repo.Read
method that returns a text stream from the string of aSourceText
object but that's not ideal.Part of #26902