Skip to content
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.

Sign up for GitHub

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

Sg cleanup post cohost #11534

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+74 −69
Open

Sg cleanup post cohost #11534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b04f41
Remove unneeded design time generation
chsienki Feb 19, 2025
db30121
Remove static logger
chsienki Feb 19, 2025
1564060
Simplify
chsienki Feb 19, 2025
4c333b5
Pass through cancellation token to process
chsienki Feb 19, 2025
cbc728e
Flatten nested tuples
chsienki Feb 19, 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
18 changes: 18 additions & 0 deletions ...ft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/IncrementalValueProviderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,24 @@ internal static IncrementalValueProvider<bool> CheckGlobalFlagSet(this Increment
{
return optionsProvider.Select((provider, _) => provider.GlobalOptions.TryGetValue($"build_property.{flagName}", out var flagValue) && flagValue == "true");
}

internal static IncrementalValuesProvider<ValueTuple<T1, T2, T3>> Combine<T1, T2, T3>(this IncrementalValuesProvider<ValueTuple<T1, T2>> provider, IncrementalValueProvider<T3> target)
{
return IncrementalValueProviderExtensions.Combine(provider, target)
.Select((p, _) => (p.Left.Item1, p.Left.Item2, p.Right));
Copy link
Member

Choose a reason for hiding this comment

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

Would it be useful to have a Select extension that takes a lambda that is only passed one argument?

}

internal static IncrementalValuesProvider<ValueTuple<T1, T2, T3, T4>> Combine<T1, T2, T3, T4>(this IncrementalValuesProvider<ValueTuple<T1, T2, T3>> provider, IncrementalValueProvider<T4> target)
{
return IncrementalValueProviderExtensions.Combine(provider, target)
.Select((p, _) => (p.Left.Item1, p.Left.Item2, p.Left.Item3, p.Right));
}

internal static IncrementalValueProvider<ValueTuple<T1, T2, T3>> Combine<T1, T2, T3>(this IncrementalValueProvider<ValueTuple<T1, T2>> provider, IncrementalValueProvider<T3> target)
{
return IncrementalValueProviderExtensions.Combine(provider, target)
.Select((p, _) => (p.Left.Item1, p.Left.Item2, p.Right));
}
}

internal sealed class LambdaComparer<T> : IEqualityComparer<T>
Expand Down
14 changes: 5 additions & 9 deletions ...t.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.RazorProviders.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,16 @@ namespace Microsoft.NET.Sdk.Razor.SourceGenerators
{
public partial class RazorSourceGenerator
{
private (RazorSourceGenerationOptions?, Diagnostic?) ComputeRazorSourceGeneratorOptions((((AnalyzerConfigOptionsProvider, ParseOptions), ImmutableArray<MetadataReference>), bool) pair, CancellationToken ct)
private (RazorSourceGenerationOptions?, Diagnostic?) ComputeRazorSourceGeneratorOptions(AnalyzerConfigOptionsProvider analyzerConfigOptions, ParseOptions parseOptions, ImmutableArray<MetadataReference> references, bool isGeneratorSuppressed)
{
var (((options, parseOptions), references), isSuppressed) = pair;
var globalOptions = options.GlobalOptions;

if (isSuppressed)
if (isGeneratorSuppressed)
{
return default;
}

Log.ComputeRazorSourceGeneratorOptions();
RazorSourceGeneratorEventSource.Log.ComputeRazorSourceGeneratorOptions();

var globalOptions = analyzerConfigOptions.GlobalOptions;
globalOptions.TryGetValue("build_property.RazorConfiguration", out var configurationName);
globalOptions.TryGetValue("build_property.RootNamespace", out var rootNamespace);
globalOptions.TryGetValue("build_property.SupportLocalizedComponentNames", out var supportLocalizedComponentNames);
Expand All @@ -49,9 +47,7 @@ public partial class RazorSourceGenerator
.Where(r => r.Display is { } display && display.EndsWith("Microsoft.AspNetCore.Components.dll", StringComparison.Ordinal))
.ToImmutableArray();

var isComponentParameterSupported = minimalReferences.Length == 0
? false
: CSharpCompilation.Create("components", references: minimalReferences).HasAddComponentParameter();
var isComponentParameterSupported = minimalReferences.Length != 0 && CSharpCompilation.Create("components", references: minimalReferences).HasAddComponentParameter();

var razorConfiguration = new RazorConfiguration(razorLanguageVersion, configurationName ?? "default", Extensions: [], UseConsolidatedMvcViews: true, SuppressAddComponentParameter: !isComponentParameterSupported);

Expand Down
111 changes: 51 additions & 60 deletions ...mpiler/Microsoft.CodeAnalysis.Razor.Compiler/src/SourceGenerators/RazorSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace Microsoft.NET.Sdk.Razor.SourceGenerators
#pragma warning restore RS1041 // This compiler extension should not be implemented in an assembly with target framework '.NET 8.0'. References to other target frameworks will cause the compiler to behave unpredictably.
public partial class RazorSourceGenerator : IIncrementalGenerator
{
private static RazorSourceGeneratorEventSource Log => RazorSourceGeneratorEventSource.Log;

internal static bool UseRazorCohostServer { get; set; } = false;

// Testing usage only.
Expand Down Expand Up @@ -48,7 +46,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
.Combine(parseOptions)
.Combine(metadataRefs.Collect())
.SuppressIfNeeded(isGeneratorSuppressed)
.Select(ComputeRazorSourceGeneratorOptions)
.Select((p, _) => ComputeRazorSourceGeneratorOptions(p.Item1.Item1, p.Item1.Item2, p.Item1.Item3, p.Item2))
Copy link
Member

Choose a reason for hiding this comment

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

For clarity, consider adding named arguments for the values passed to ComputeRazorSourceGeneratorOptions. Or, add a body and deconstruct p into variables that are passed separately. Or, add a ComputeRazorSourceGeneratorOptions overload that take the expected ValueTuple with named members.

.ReportDiagnostics(context);

var sourceItems = additionalTexts
Expand Down Expand Up @@ -82,15 +80,15 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
var generatedDeclarationText = componentFiles
.Combine(importFiles.Collect())
.Combine(razorSourceGeneratorOptions)
.WithLambdaComparer((old, @new) => old.Right.Equals(@new.Right) && old.Left.Left.Equals(@new.Left.Left) && old.Left.Right.SequenceEqual(@new.Left.Right))
.Select(static (pair, _) =>
.WithLambdaComparer((old, @new) => old.Item3.Equals(@new.Item3) && old.Item1.Equals(@new.Item1) && old.Item2.SequenceEqual(@new.Item2))
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a body to this lambda and deconstructing old and new into variables before performing the comparison. Alternatively, create a helper method that names the tuple parameters.

.Select(static (pair, cancellationToken) =>
{
var ((sourceItem, importFiles), razorSourceGeneratorOptions) = pair;
var (sourceItem, importFiles, razorSourceGeneratorOptions) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.

RazorSourceGeneratorEventSource.Log.GenerateDeclarationCodeStart(sourceItem.FilePath);

var projectEngine = GetDeclarationProjectEngine(sourceItem, importFiles, razorSourceGeneratorOptions);

var codeGen = projectEngine.Process(sourceItem);
var codeGen = projectEngine.Process(sourceItem, cancellationToken);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: consider renaming codeGen to codeDocument.

Copy link
Member

Choose a reason for hiding this comment

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

Love the cancellation! Should more of the RazorProjectEngine "Process" methods (like ProcessRemaining) take a CancellationToken as well?


var result = new SourceGeneratorText(codeGen.GetCSharpDocument().Text);

Expand Down Expand Up @@ -145,8 +143,8 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
.Combine(hasRazorFiles)
.WithLambdaComparer(static (a, b) =>
{
var ((compilationA, razorSourceGeneratorOptionsA), hasRazorFilesA) = a;
var ((compilationB, razorSourceGeneratorOptionsB), hasRazorFilesB) = b;
var (compilationA, razorSourceGeneratorOptionsA, hasRazorFilesA) = a;
var (compilationB, razorSourceGeneratorOptionsB, hasRazorFilesB) = b;

// When using the generator cache in the compiler it's possible to encounter metadata references that are different instances
// but ultimately represent the same underlying assembly. We compare the module version ids to determine if the references are the same
Expand Down Expand Up @@ -194,7 +192,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
})
.Select(static (pair, _) =>
{
var ((compilation, razorSourceGeneratorOptions), hasRazorFiles) = pair;
var (compilation, razorSourceGeneratorOptions, hasRazorFiles) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.

if (!hasRazorFiles)
{
// If there's no razor code in this app, don't do anything.
Expand Down Expand Up @@ -233,67 +231,60 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
.WithLambdaComparer((old, @new) => old.Left.Equals(@new.Left) && old.Right.SequenceEqual(@new.Right))
.Combine(razorSourceGeneratorOptions);

// Currently unused. See https://github.com/dotnet/roslyn/issues/71024.
var razorHostOutputsEnabled = analyzerConfigOptions.CheckGlobalFlagSet("EnableRazorHostOutputs");
var withOptionsDesignTime = withOptions.EmptyOrCachedWhen(razorHostOutputsEnabled, false);

IncrementalValuesProvider<(string, SourceGeneratorRazorCodeDocument)> processed(bool designTime)
{
return (designTime ? withOptionsDesignTime : withOptions)
.Select((pair, _) =>
{
var ((sourceItem, imports), razorSourceGeneratorOptions) = pair;
var processed =
withOptions
.Select((pair, _) =>
{
var (sourceItem, imports, razorSourceGeneratorOptions) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.


RazorSourceGeneratorEventSource.Log.ParseRazorDocumentStart(sourceItem.RelativePhysicalPath);
RazorSourceGeneratorEventSource.Log.ParseRazorDocumentStart(sourceItem.RelativePhysicalPath);

var projectEngine = GetGenerationProjectEngine(sourceItem, imports, razorSourceGeneratorOptions);
var projectEngine = GetGenerationProjectEngine(sourceItem, imports, razorSourceGeneratorOptions);

var document = projectEngine.ProcessInitialParse(sourceItem, designTime);
var document = projectEngine.ProcessInitialParse(sourceItem, designTime: false);
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming document to codeDocument since it will be deconstructed into codeDocument later.


RazorSourceGeneratorEventSource.Log.ParseRazorDocumentStop(sourceItem.RelativePhysicalPath);
return (projectEngine, sourceItem.RelativePhysicalPath, document);
})
RazorSourceGeneratorEventSource.Log.ParseRazorDocumentStop(sourceItem.RelativePhysicalPath);
return (projectEngine, sourceItem.RelativePhysicalPath, document);
})

// Add the tag helpers in, but ignore if they've changed or not, only reprocessing the actual document changed
.Combine(allTagHelpers)
.WithLambdaComparer((old, @new) => old.Left.Equals(@new.Left))
.Select(static (pair, _) =>
{
var ((projectEngine, filePath, codeDocument), allTagHelpers) = pair;
RazorSourceGeneratorEventSource.Log.RewriteTagHelpersStart(filePath);
// Add the tag helpers in, but ignore if they've changed or not, only reprocessing if the actual document changed
.Combine(allTagHelpers)
.WithLambdaComparer((old, @new) => old.Item3.Equals(@new.Item3))
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it's always super hard for me to follow the data flow in RazorSourceGenerator. Left and Right were already a bit obfuscated, but the tuple Item1, Item2, Item3, etc. make it even more challenging to sort out what's happening. Consider adding a lambda body here to deconstruct old and @new, or creating a helper method that declares names for the tuple members.

.Select(static (pair, _) =>
{
var (projectEngine, filePath, codeDocument, allTagHelpers) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.

RazorSourceGeneratorEventSource.Log.RewriteTagHelpersStart(filePath);

codeDocument = projectEngine.ProcessTagHelpers(codeDocument, allTagHelpers, checkForIdempotency: false);
codeDocument = projectEngine.ProcessTagHelpers(codeDocument, allTagHelpers, checkForIdempotency: false);

RazorSourceGeneratorEventSource.Log.RewriteTagHelpersStop(filePath);
return (projectEngine, filePath, codeDocument);
})
RazorSourceGeneratorEventSource.Log.RewriteTagHelpersStop(filePath);
return (projectEngine, filePath, codeDocument);
})

// next we do a second parse, along with the helpers, but check for idempotency. If the tag helpers used on the previous parse match, the compiler can skip re-writing them
.Combine(allTagHelpers)
.Select(static (pair, _) =>
{
var ((projectEngine, filePath, document), allTagHelpers) = pair;
RazorSourceGeneratorEventSource.Log.CheckAndRewriteTagHelpersStart(filePath);
// next we do a second parse, along with the helpers, but check for idempotency. If the tag helpers used on the previous parse match, the compiler can skip re-writing them
.Combine(allTagHelpers)
.Select(static (pair, _) =>
{
var (projectEngine, filePath, document, allTagHelpers) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.

Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.

RazorSourceGeneratorEventSource.Log.CheckAndRewriteTagHelpersStart(filePath);

document = projectEngine.ProcessTagHelpers(document, allTagHelpers, checkForIdempotency: true);
document = projectEngine.ProcessTagHelpers(document, allTagHelpers, checkForIdempotency: true);

RazorSourceGeneratorEventSource.Log.CheckAndRewriteTagHelpersStop(filePath);
return (projectEngine, filePath, document);
})
.Select((pair, _) =>
{
var (projectEngine, filePath, document) = pair;
RazorSourceGeneratorEventSource.Log.CheckAndRewriteTagHelpersStop(filePath);
return (projectEngine, filePath, document);
})
.Select((pair, _) =>
{
var (projectEngine, filePath, document) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.

Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.


var kind = designTime ? "DesignTime" : "Runtime";
RazorSourceGeneratorEventSource.Log.RazorCodeGenerateStart(filePath, kind);
document = projectEngine.ProcessRemaining(document);
RazorSourceGeneratorEventSource.Log.RazorCodeGenerateStart(filePath, "Runtime");
document = projectEngine.ProcessRemaining(document);

RazorSourceGeneratorEventSource.Log.RazorCodeGenerateStop(filePath, kind);
return (filePath, document);
});
}
RazorSourceGeneratorEventSource.Log.RazorCodeGenerateStop(filePath, "Runtime");
return (filePath, document);
});

var csharpDocuments = processed(designTime: false)
var csharpDocuments = processed
.Select(static (pair, _) =>
{
var (filePath, document) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider renaming document to codeDocument for consistency and to connect it to the data that has flowed from earlier.

Expand All @@ -318,7 +309,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context)

context.RegisterImplementationSourceOutput(csharpDocumentsWithSuppressionFlag, static (context, pair) =>
{
var ((hintName, _, csharpDocument), isGeneratorSuppressed) = pair;
var (hintName, _, csharpDocument, isGeneratorSuppressed) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.


// When the generator is suppressed, we may still have a lot of cached data for perf, but we don't want to actually add any of the files to the output
if (!isGeneratorSuppressed)
Expand All @@ -344,7 +335,7 @@ public void Initialize(IncrementalGeneratorInitializationContext context)
context.RegisterHostOutput(hostOutputs, (context, pair) =>
#pragma warning restore RSEXPERIMENTAL004 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.
{
var ((documents, tagHelpers), isGeneratorSuppressed) = pair;
var (documents, tagHelpers, isGeneratorSuppressed) = pair;
Copy link
Member

Choose a reason for hiding this comment

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

Consider a name other than pair, since it now contains more than two pieces of data.


if (!isGeneratorSuppressed)
{
Expand Down