Add support for source generators in Razor compiler/SDK #15756

captainsafia · 2021-02-09T05:57:38Z

Migrates the contents of dotnet/aspnetcore#28807 into the SDK repo with:

Fixes for packaging of source generator assemblies into outputs
Layout changes to output source-generators in SDK

dotnet-issue-labeler · 2021-02-09T05:57:41Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

captainsafia · 2021-02-09T17:15:13Z

/azp run

azure-pipelines · 2021-02-09T17:15:22Z

Azure Pipelines successfully started running 1 pipeline(s).

captainsafia · 2021-02-10T00:12:42Z

/azp run

azure-pipelines · 2021-02-10T00:12:51Z

Azure Pipelines successfully started running 1 pipeline(s).

pranavkm · 2021-02-10T01:40:19Z

src/Layout/redist/targets/GenerateLayout.targets

  </Target>

  <Target Name="MoveRazorSdkTools" AfterTargets="PublishRazorSdkTools">
    <ItemGroup>
      <_RazorToolOutput Include="$(ArtifactsBinDir)Microsoft.NET.Sdk.Razor.Tool\$(Configuration)\$(SdkTargetFramework)\publish\*.*" />
      <_MvcRazorExtensionOutput Include="$(ArtifactsBinDir)$(Configuration)\Sdks\Microsoft.NET.Sdk.Razor\tasks\$(SdkTargetFramework)\Microsoft.AspNetCore.Mvc.Razor.Extensions.dll" />
+      <_RazorSourceGeneratorsOutput Include="$(ArtifactsBinDir)Microsoft.NET.Sdk.Razor.SourceGenerators\$(Configuration)\netstandard2.0\publish\*.*" />


Slightly relevant: dotnet/installer#9635 (comment).

src/RazorSdk/SourceGenerators/Microsoft.NET.Sdk.Razor.SourceGenerators.csproj

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

pranavkm · 2021-02-10T01:44:51Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

+                }
+
+                Directory.CreateDirectory(Path.GetDirectoryName(file.GeneratedOutputPath));
+                File.WriteAllText(file.GeneratedOutputPath, csharpDocument.GeneratedCode);


Follow up: this compiles in to the assembly. There might be some runtime work involved here. See dotnet/aspnetcore#29862

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

src/RazorSdk/SourceGenerators/SourceGeneratorProjectItem.cs

src/RazorSdk/SourceGenerators/SourceTextRazorSourceDocument.cs

src/RazorSdk/Targets/Microsoft.NET.Sdk.Razor.Compilation.targets

pranavkm · 2021-02-10T01:58:56Z

src/RazorSdk/Targets/Sdk.Razor.CurrentVersion.targets

+    <_TargetingNET60OrLater Condition=" '$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND
+      $([MSBuild]::VersionGreaterThanOrEquals('$(TargetFrameworkVersion)', '6.0')) ">true</_TargetingNET60OrLater>
+
+    <_UseRazorSourceGenerator Condition="'$(_TargetingNET60OrLater)' == 'true'">true</_UseRazorSourceGenerator>


Do you have a sense for what it would take to un-jankify this file?

Took a stab at cleaning this up. Let me know what you think about using the Choose/When pattern for conditional setting these properties.

This is alright. Maybe a further step would be to try and isolate < 5 and this further to make sure the build order is easier to grock across tfms and we don't accidentally break things.

src/Tests/Microsoft.NET.Sdk.Razor.Tests/MvcBuildIntegrationTestLegacy.cs

SteveSandersonMS · 2021-02-15T11:00:49Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

+                    return null;
+                }
+
+                var fileWriteTime = File.GetLastWriteTimeUtc(portableExecutableReference.FilePath);


It's possible I'm misreading the flow here, but are we assuming that the last write time of a .dll on disk has some relationship to whether our cache is up-to-date?

What happens when someone changes a reference to an older version of a package (which was already cached locally) - don't we need to regenerate the compilation output then?

Yup, it's the same issue we've sorted multiple times with the Blazor SDK. We could make this more robust, but we're also waiting for better support for this sort of incrementality from the compiler (tracked dotnet/roslyn#51257).

Directly accessing the file system from a generator violates the assumptions we have around compilation. Performance is important but correctness is more so. 😄

We could make this more robust

Yes, I'd think we need to do so without waiting for new compiler features. I don't know if there was already an issue filed to track this, so just in case I filed https://github.com/dotnet/sdk/issues/15913

Ah, now I realise that @captainsafia has already filed dotnet/aspnetcore#30234

Would it make sense to keep https://github.com/dotnet/sdk/issues/15913 open to track the actual correctness issue (which should be addressed in the short term), and dotnet/aspnetcore#30234 to track the broader performance/architectural issue of accessing the filesystem at all (which can wait longer)?

If @captainsafia or @pranavkm prefers there being just one issue for all of this, please feel free to close mine as a duplicate.

@SteveSandersonMS Having another issue is fine. If only to capture this particular side-effect of requiring read/write interactions to compute the cache. Ultimately, whatever work is done here will be usurped by what we do long term do deal with the FS-access issue.

jaredpar · 2021-02-17T00:32:42Z

src/RazorSdk/SourceGenerators/RazorDiagnostics.cs

+            var descriptor = new DiagnosticDescriptor(
+                razorDiagnostic.Id,
+                razorDiagnostic.GetMessage(CultureInfo.CurrentCulture),
+                razorDiagnostic.GetMessage(CultureInfo.CurrentCulture),


This isn't the right pattern here. The compiler doesn't generate diagnostics based on CultureInfo.CurrentCulture but rather by the culture that's passed into the compiler options. In general you should be using LocalizableResourceString here. That will do the heavy lifting for you.

https://github.com/dotnet/roslyn-analyzers/blob/5102a0d89ef178b0ca741634ee006597bf5df2ed/src/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.cs#L40

Think that pattern should fit what you're doing here.

Nice tip -- I've tracked this in dotnet/aspnetcore#30237 since I don't think this will happen for preview2 but we can sort out the diagnostics story in preview3.

jaredpar · 2021-02-17T00:34:28Z

src/RazorSdk/SourceGenerators/RazorSourceGenerationContext.cs

+            globalOptions.TryGetValue("build_property.DesignTimeBuild", out var designTimeBuild);
+            if (!globalOptions.TryGetValue("build_property._RazorReferenceAssemblyTagHelpersOutputPath", out var refsTagHelperOutputCachePath))
+            {
+                throw new InvalidOperationException("_RazorReferenceAssemblyTagHelpersOutputPath is not specified.");


Rather than throwing consider adding a diagnostic with error severity. That will produce a better customer experience here

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

jaredpar · 2021-02-17T00:38:15Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

+                }
+
+                Directory.CreateDirectory(Path.GetDirectoryName(file.GeneratedOutputPath));
+                File.WriteAllText(file.GeneratedOutputPath, csharpDocument.GeneratedCode);


Generators should not access the file system directly. That breaks several assumptions by the build system here. This is something we need to address.

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

jaredpar · 2021-02-17T00:39:56Z

src/RazorSdk/SourceGenerators/RazorSourceGenerator.cs

+            Parallel.For(0, files.Count, GetParallelOptions(), i =>
+            {
+                var file = files[i];
+                if (File.GetLastWriteTimeUtc(file.GeneratedDeclarationPath) > File.GetLastWriteTimeUtc(file.AdditionalText.Path))


Why are we accessing the file system here? Generators should not be interacting with the file system.

jaredpar · 2021-02-17T01:13:41Z

Can we file an issue to track the direct access to the file system here? That is fine short term but near term it's something we need to address and find a different approach for.

captainsafia · 2021-02-17T01:59:21Z

@jaredpar Sure! I've created dotnet/aspnetcore#30234 and jotted a few notes about where things stand long term with this.

captainsafia added the Area-AspNetCore RazorSDK, BlazorWebAssemblySDK, dotnet-watch label Feb 9, 2021

captainsafia force-pushed the safia/rsg branch 2 times, most recently from 059e3f0 to 9a2b607 Compare February 9, 2021 20:31

captainsafia marked this pull request as ready for review February 10, 2021 00:12

captainsafia requested a review from pranavkm February 10, 2021 00:12

captainsafia mentioned this pull request Feb 10, 2021

Reimplement the Razor compiler using source generators & support incrementality dotnet/aspnetcore#26902

Closed

9 tasks

Add support for source generators in Razor compiler/SDK

bcba240

pranavkm reviewed Feb 10, 2021

View reviewed changes

captainsafia force-pushed the safia/rsg branch from 9a2b607 to bcba240 Compare February 10, 2021 02:01

Address feedback from peer review

5f18a1e

pranavkm approved these changes Feb 11, 2021

View reviewed changes

captainsafia added 2 commits February 10, 2021 19:28

Fix ordering for switch statement

3ca1b1a

Fix SimpleMvc50 project and conditional properties

90f897a

captainsafia merged commit 12631a8 into master Feb 11, 2021

SteveSandersonMS reviewed Feb 15, 2021

View reviewed changes

jaredpar reviewed Feb 17, 2021

View reviewed changes

captainsafia mentioned this pull request Feb 17, 2021

Improve error reporting in Razor source generators dotnet/aspnetcore#30237

Closed

SteveSandersonMS mentioned this pull request Feb 17, 2021

Razor source generator incrementality goes wrong on downgrades dotnet/aspnetcore#30246

Closed

theolivenbaum mentioned this pull request Mar 11, 2021

Check how Razor compiler uses source code generators curiosity-ai/h5#33

Open

akoeplinger deleted the safia/rsg branch April 12, 2021 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for source generators in Razor compiler/SDK #15756

Add support for source generators in Razor compiler/SDK #15756

captainsafia commented Feb 9, 2021 •

edited

Loading

dotnet-issue-labeler bot commented Feb 9, 2021

captainsafia commented Feb 9, 2021

azure-pipelines bot commented Feb 9, 2021

captainsafia commented Feb 10, 2021

azure-pipelines bot commented Feb 10, 2021

pranavkm Feb 10, 2021

pranavkm Feb 10, 2021

pranavkm Feb 10, 2021

captainsafia Feb 11, 2021

pranavkm Feb 11, 2021

SteveSandersonMS Feb 15, 2021

pranavkm Feb 17, 2021

jaredpar Feb 17, 2021 •

edited

Loading

SteveSandersonMS Feb 17, 2021

SteveSandersonMS Feb 17, 2021

captainsafia Feb 17, 2021

jaredpar Feb 17, 2021

jaredpar Feb 17, 2021

captainsafia Feb 17, 2021

jaredpar Feb 17, 2021

jaredpar Feb 17, 2021 •

edited

Loading

jaredpar Feb 17, 2021

jaredpar commented Feb 17, 2021

captainsafia commented Feb 17, 2021

Add support for source generators in Razor compiler/SDK #15756

Add support for source generators in Razor compiler/SDK #15756

Conversation

captainsafia commented Feb 9, 2021 • edited Loading

dotnet-issue-labeler bot commented Feb 9, 2021

captainsafia commented Feb 9, 2021

azure-pipelines bot commented Feb 9, 2021

captainsafia commented Feb 10, 2021

azure-pipelines bot commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar Feb 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Feb 17, 2021

captainsafia commented Feb 17, 2021

captainsafia commented Feb 9, 2021 •

edited

Loading

jaredpar Feb 17, 2021 •

edited

Loading

jaredpar Feb 17, 2021 •

edited

Loading