Add analyzer to prevent local functions accessing RenderTreeBuilder from parent scope in Blazor components#63826
Conversation
|
Greetings human! You've submitted a PR that modifies code that is shared with https://github.com/dotnet/runtime . Please make sure you synchronize this code with the changes in that repo! |
Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
…der analyzer Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
| context.RegisterSyntaxNodeAction(context => | ||
| { | ||
| var localFunction = (LocalFunctionStatementSyntax)context.Node; | ||
|
|
||
| // Check if this local function contains any RenderTreeBuilder method calls | ||
| if (ContainsRenderTreeBuilderCalls(wellKnownTypes, localFunction, context.SemanticModel)) | ||
| { | ||
| context.ReportDiagnostic(Diagnostic.Create( | ||
| DiagnosticDescriptors.DoNotUseLocalFunctionsInMarkup, | ||
| localFunction.Identifier.GetLocation(), | ||
| localFunction.Identifier.ValueText)); | ||
| } | ||
| }, SyntaxKind.LocalFunctionStatement); |
There was a problem hiding this comment.
Rather than analyze every single local function declaration, scope your analysis to components first. Then check only BuildRenderTree if the method extends component base.
There was a problem hiding this comment.
Refactored in commit 36e3eba. The analyzer now scopes analysis to BuildRenderTree methods within ComponentBase-derived classes, significantly improving efficiency and avoiding false positives in non-component code.
|
@copilot chop chop |
…Base classes Co-authored-by: javiercn <6995051+javiercn@users.noreply.github.com>
|
@copilot Update this draft onto current |
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Updated through |
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
|
@copilot Please revise the current draft at
<Reference Include="Microsoft.AspNetCore.Razor.Language" />Do not add a direct
var alias = builder;
while (ShouldReplace())
{
alias = new RenderTreeBuilder();
}
void LocalFunction()
{
alias.OpenElement(0, "div");
alias.CloseElement();
}The exact regression is red at the current head: 23 total, 22 pass, expected ASP0039 absent. A targeted loop-entry/body join makes it green and the wider analyzer project passes 734/734, but that proof patch is not a production solution: it mishandles
Preserve all existing fresh-builder, owning-alias, both reassignment directions, chained assignment, nested local/lambda, method-group, and real |
|
@copilot Please revise this draft at The straight-line provenance fixes look good, but three issues remain:
Keep the existing alias, reassignment, nested local/lambda, method-group, and real |
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
|
@copilot Please revise this draft at The loop provenance and SDK Razor fixture changes look much better, but two blockers remain:
Please add the returning-branch no-diagnostic regression and scope Keep the current straight-line, loop-family, switch, nested local/lambda, method-group, Please rerun the ASP0039 class, full analyzer project, and clean cross-platform CI. Also update the stale PR description from ASP0029 and the old test totals. |
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ASP.NET Core analyzer diagnostic (ASP0039) that flags local functions inside Blazor component BuildRenderTree(RenderTreeBuilder) overrides when they access a captured RenderTreeBuilder from an outer scope, which can corrupt rendering output in Razor-generated code.
Changes:
- Adds the
DoNotUseLocalFunctionsInMarkupAnalyzer(ASP0039) scoped toComponentBase+BuildRenderTree(RenderTreeBuilder)overrides. - Adds analyzer tests, including coverage for Razor-generated output via a test asset project and copied generated
.g.cs. - Updates diagnostic resources and the public diagnostics list to include ASP0039.
Show a summary per file
| File | Description |
|---|---|
| src/Framework/AspNetCoreAnalyzers/test/testassets/DoNotUseLocalFunctionsInMarkup/IssueSample.razor | Adds a Razor repro sample that generates the problematic local-function pattern. |
| src/Framework/AspNetCoreAnalyzers/test/testassets/DoNotUseLocalFunctionsInMarkup/FluentTreeItem.razor | Adds a minimal component used by the Razor repro sample. |
| src/Framework/AspNetCoreAnalyzers/test/testassets/DoNotUseLocalFunctionsInMarkup/DoNotUseLocalFunctionsInMarkup.csproj | Adds a Razor SDK test asset project that emits compiler-generated files for inspection. |
| src/Framework/AspNetCoreAnalyzers/test/Microsoft.AspNetCore.App.Analyzers.Test.csproj | Wires the test asset project and copies the generated .g.cs into test output; adds Razor.Language reference. |
| src/Framework/AspNetCoreAnalyzers/test/Components/DoNotUseLocalFunctionsInMarkupTest.cs | Adds comprehensive analyzer test coverage, including generated Razor code mapping assertions. |
| src/Framework/AspNetCoreAnalyzers/test/Components/DisallowNonLiteralSequenceNumbersTest.cs | Adds a regression-style test ensuring generated code scenarios work for existing analyzer infrastructure. |
| src/Framework/AspNetCoreAnalyzers/src/Analyzers/Resources.resx | Adds localized title/message strings for the new diagnostic. |
| src/Framework/AspNetCoreAnalyzers/src/Analyzers/RenderTreeBuilder/DoNotUseLocalFunctionsInMarkupAnalyzer.cs | Implements the new analyzer with flow/provenance tracking to detect captured owning-builder usage. |
| src/Framework/AspNetCoreAnalyzers/src/Analyzers/DiagnosticDescriptors.cs | Registers ASP0039 diagnostic descriptor. |
| docs/list-of-diagnostics.md | Updates the documented ASP diagnostic range and adds ASP0039 to the list. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
|
@copilot Two small review follow-ups are valid at
Please keep the PR ready for review and make only those resource/description updates. Don't add a null guard around |
|
@copilot Please pick up the two scoped follow-ups in #63826 (comment) on the current head. Only update the diagnostic title resource and PR description, keep the PR ready, and do not change |
Co-authored-by: PureWeen <5375137+PureWeen@users.noreply.github.com>
Updated the ASP0039 diagnostic title in |
Both scoped follow-ups are complete on current head |
Description
This PR adds a new analyzer diagnostic ASP0039 that detects local functions which access
RenderTreeBuildermethods from captured variables in their parent scope within Blazor components.Problem
Local functions defined within
@{ ... }blocks in Razor components'BuildRenderTreemethods can captureRenderTreeBuilderinstances from their parent scope, leading to incorrect rendering behavior. This pattern appears to work but actually corrupts the rendering output instead of properly writing to child component render fragments.The issue occurs because C# scoping rules cause the local function to capture the
RenderTreeBuilderfrom the parent context rather than using the builder that should be passed to theRenderFragment.Solution
The analyzer is efficiently scoped to only analyze local functions within:
BuildRenderTreemethods (by name)ComponentBaseThis scoping prevents false positives in non-component code and focuses analysis on the actual problematic pattern in Razor components.
New Diagnostic: ASP0039
Detection Logic
The analyzer intelligently identifies problematic patterns while allowing safe alternatives:
✅ Allowed (Safe Patterns):
RenderTreeBuilderas a parameterComponentBase-derived classesBuildRenderTreeRenderTreeBuilderat all❌ Detected (Problematic Patterns):
RenderTreeBuilderfrom captured variables withinBuildRenderTreemethodsExample
Before (causes runtime issues):
After (recommended approaches):
Testing
Added comprehensive test coverage across 40 targeted ASP0039 cases covering:
All 751 analyzer tests pass, including 40 targeted ASP0039 cases.
Performance
By scoping the analysis to only
BuildRenderTreemethods inComponentBase-derived classes (rather than all local functions), the analyzer is much more efficient and avoids false positives in non-component code.Impact
This change helps developers avoid a subtle but problematic pattern that can cause rendering corruption in Blazor applications. The analyzer provides clear, actionable feedback at compile time rather than allowing runtime failures.
Original prompt
This section details on the original issue you should resolve
<issue_title>Prevent use of local functions inside markup</issue_title>
<issue_description>[Edit by @SteveSandersonMS] This issue was originally reported by @verdie-g as follows below the line. On investigation the problem is that C# has added a new syntax that doesn't work in Razor.
The Razor compiler allows arbitrary C# code within
@{ ... }blocks. Unfortunately this means it allows the use of local functions in a way that confuses the parsing logic, causing it to use the wrong__builderinstance. Example:Here, the child content of
FluentTreeItemshould be compiled as aRenderFragmentthat acts on whateverRenderTreeBuilderis passed in. But because of C# scoping rules, theRenderFragmentactually acts on the__buildercaptured from its parent context, so it is simply corrupting the output instead of doing something useful.Possible solutions:
@{ ... }specifically. However that's probably impractical because Razor doesn't parse the contents of@{ ... }.RenderTreeBuilderis invoked. For example if the runtime set an "rendering in progress" flag on it before it starts rendering and synchronously unsets that flag at the end of rendering, then it would have caught this case because child components are rendered afterwards (not recursively), so when the child is rendered it would see it's trying to write to a builder that does not have the "rendering in progress" flag set.NullReferenceExceptioninstead of corrupt output. Obviously that's not super easy to understand but avoids any perf cost.Is there an existing issue for this?
Describe the bug
I'm rendering a blazor wasm component using a recursive C# method and while it's working fine using C# only (
OpenComponent,AddAttribute, etc.), it fails when returning HTML from that recursive method.Expected Behavior
I'm expecting a tree structure to be built and clicking on a line should expand its children but it seems like the children are not rendered and an error is thrown on click.
Steps To Reproduce
I was not able to reproduce the issue without the library fluentui-blazor.
dotnet new install Microsoft.FluentUI.AspNetCore.Templates dotnet new fluentblazorwasm --name aspnetcore-issue-53269 cd aspnetcore-issue-53269Then replace
Home.razorwithClick on the item generated and check the error in the console.
It could be an error with the library (initially reported here microsoft/fluentui-blazor#1289) but this code works fine:
Exceptions (if any)