Skip to content

Run Razor decl C# lowering before tag-helper discovery (classify-first component split) - #84605

Merged
chsienki merged 8 commits into
dotnet:features/sonicfrom
chsienki:sonic/decl-precomp-fastpath
Jul 24, 2026
Merged

Run Razor decl C# lowering before tag-helper discovery (classify-first component split)#84605
chsienki merged 8 commits into
dotnet:features/sonicfrom
chsienki:sonic/decl-precomp-fastpath

Conversation

@chsienki

@chsienki chsienki commented Jul 23, 2026

Copy link
Copy Markdown
Member

What

Reorder the Razor engine phases so a component's markup split and decl C# lowering run before tag-helper discovery, and rewrite the split to partition the classified class body. The decl half — the markup-free tag-helper-descriptor surface (base type, interfaces, type parameters, parameters/fields/methods) — is produced from user source alone, independent of tag-helper resolution. The markup-bearing impl half (the render method plus any markup-bearing methods) flows through the rest of the pipeline.

Every component now splits: a markup-free @code sends its whole body to the decl half; only a component the analysis can't route safely (a markup property, @inject, a preprocessor directive, a header/arity directive combined with markup, or unrecoverable syntax) falls back to a single document.

Why

The resolution-independent decl half is the enabler for the Sonic incremental source generator: a follow-up feeds it into the SG's input compilation so tag-helper discovery stays incremental without paying for full tag-helper resolution per keystroke. This PR establishes the classify-first pipeline that produces the decl early; it does not yet consume it.

Commits

  1. Add decl-half baselines to the integration-test harness — a DeclDocumentNode slot and .decl.ir.txt / .decl.codegen.cs baseline emission, so the partition is reviewable at the IR and C# level. A document with no decl half asserts no decl baseline lingers.
  2. Emit RZ9978 during IR lowering instead of tag-helper discovery — with discovery moving after IR lowering, the "@addTagHelper/@removeTagHelper/@tagHelperPrefix are not valid in a component" diagnostic is emitted while lowering the directive's CSharpStatementLiteral, keyed on the chunk-generator type.
  3. Split the component classify-first: reorder phases and rewrite the split — the core change (phase reorder in RazorProjectEngine, the DefaultRazorMarkupSplitPhase rewrite, decl lowering simplification, MarkupSplitter tweak, ComponentWhitespacePass → optimization). Class-decoration nodes (@layout, @attribute, @page) decorate the decl partial only, so they are never emitted on both halves. The SG's incremental replay restarts at IR lowering on a tag-helper change (rather than at resolution) so that fresh unresolved IR is rebuilt and re-bound — because resolution binds nodes in place, replaying from resolution would find nothing left to rebind.
  4. Remove the superseded impl-derivation fallback — the split produces the impl half directly, so the final C# lowering always writes the working node; the old TryWriteImplDocument path and IsSplitImplDocument flag are unreachable and removed.
  5. Regenerate Razor baselines — derived data (regenerated via GenerateBaselines).

Perf

The decl half is produced but not yet consumed (the SG pre-compilation wiring is the follow-up), so this adds a small amount of currently-unused work per component. On an incremental tag-helper change the SG re-lowers the document (as it did before Sonic) — caching the resolution-independent decl across tag-helper changes so it is not re-lowered is a deliberate follow-up (the SG pre-compilation rewire), not part of this PR.

Testing

  • Microsoft.AspNetCore.Razor.Language.UnitTests: green (3901 pass, 2 skipped).
  • Microsoft.NET.Sdk.Razor.SourceGenerators.UnitTests: green (218 pass), including the incremental tag-helper rebinding scenarios.
Microsoft Reviewers: Open in CodeFlow

chsienki and others added 5 commits July 22, 2026 20:25
The markup split produces a separate "decl" C# document -- a component's
resolution-independent public surface -- alongside the impl document. Record it
in the integration-test harness: a DeclDocumentNode slot on the document node,
and baseline emission/verification of the decl IR (.decl.ir.txt) and decl C#
(.decl.codegen.cs / .decl.mappings.txt). A document with no decl half asserts
that no decl baseline exists, so a document that produces none never leaves an
orphaned baseline behind.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
The "@addTagHelper / @removeTagHelper / @tagHelperPrefix are not valid in a
component" diagnostic (RZ9978) is emitted while lowering the component's
CSharpStatementLiteral nodes, keyed on the chunk-generator type, rather than by
the component tag-helper discovery visitor. The diagnostic rides on the directive
content token, so its span covers the directive argument.

Emitting it during lowering keeps it correct independent of the tag-helper
discovery phase's position in the pipeline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Run the markup split and decl C# lowering after directive classification but
before tag-helper discovery, so the decl half is produced from user source alone
-- markup-free and independent of tag-helper resolution.

- RazorProjectEngine orders MarkupSplit + DeclCSharpLowering ahead of discovery.
- DefaultRazorMarkupSplitPhase partitions the classified primary class body: it
  stashes the markup-free decl subtree on the document node, rewrites the working
  node into the impl half in place, and strips class-decoration nodes (@layout,
  @Attribute, @page) from the impl so they decorate the decl partial only.
- DefaultRazorDeclCSharpLoweringPhase lowers the stashed decl subtree.
- MarkupSplitter treats a statement-position element (still unresolved at this
  point) as routable markup.
- ComponentWhitespacePass moves to the optimization phase, after the split.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
With the split producing the impl half directly, the final C# lowering phase
always writes the working document node. Drop the alternate path that rebuilt an
impl spine from a classified single tree (TryWriteImplDocument) and the
IsSplitImplDocument flag that selected it; both are unreachable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Regenerated with the integration-test harness (GenerateBaselines). Markup-free
components split, moving their @code to the new .decl.* baselines; @page
components emit their route only on the decl baseline; components that fall back
to a single document carry no decl baselines.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Copilot AI review requested due to automatic review settings July 23, 2026 17:15
@chsienki
chsienki requested a review from a team as a code owner July 23, 2026 17:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

@chsienki

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).

The decl C# document is lowered before the tag-helper rewrite phase runs, so the
RazorCodeDocument it back-references has no rewritten syntax tree. Workspaces-layer
tooling reads GetRequiredTagHelperRewrittenSyntaxTree() off the decl document --
for example, cohost diagnostic filtering that decides whether an unused-using
directive is actually consumed by a component tag -- and throws when it is null.

The decl is markup-free, so its rewritten tree is just the canonical syntax tree.
Seed it on the decl document's snapshot at lowering time, leaving the main document
for the rewrite phase to update, so the back-reference is complete for every consumer.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Copilot AI review requested due to automatic review settings July 23, 2026 21:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

ImplementInterface_Explicitly_PartialBaseImplementations_AddsDerivedMembers
regresses under the decl/impl split: the cohost runs the code action on the impl
document, where Roslyn groups the generated members after the matching existing
IBase member, and the single per-@code-block source mapping translates those
mid-C# insertions to a mid-@code-block Razor position. A proper fix needs
compiler codegen / source-mapping or cohost edit changes, tracked by
dotnet#84609.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Copilot AI review requested due to automatic review settings July 24, 2026 00:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@davidwengier davidwengier left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, though I can't say I understood a lot of it :)

""", item.GetMessage(CultureInfo.CurrentCulture));
Assert.Equal(0, item.Span.LineIndex);
Assert.Equal(0, item.Span.CharacterIndex);
Assert.Equal(14, item.Span.CharacterIndex);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this mean the squiggles are just for the erroneous @addTagHelper content, not the directive itself? Not a big deal I guess, just wondering.

Are there tests for empty directives, to ensure we still have something to report on?

(I'm reviewing commit-at-a-time so haven't seen baselines yet, which might answer these questions)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh yeah, good catch. I'll update it so it walks up the tree to find the actual directive its attached to so we squiggle the whole thing (and add an empty test too)

#nullable restore
#line (7,13)-(9,1) "TestFiles/IntegrationTests/CodeGenerationIntegrationTest/BasicComponent.cshtml"

void IDisposable.Dispose(){ }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a surprsing change? Is this a bug fix? Or is there just a change to the test that I can't see (because GitHub hates the commit this came in with :) )

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, this is actually a fix from #83887 that I had missed, where we were incorrectly dropping code. This is when baseline-ing the changes for testing sucks. I plan to go over the merge to main with a fine tooth comb to make sure that they are as we expect.

}

[Fact]
[Fact(Skip = "PROTOTYPE(sonic): cohost ImplementInterface positions members mid-@code-block with the decl/impl split; see https://github.com/dotnet/roslyn/issues/84609")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mentioned offline, but if you update the baseline of this test, and it remains stable, then I'm fine with it. I made similar member ordering changes in baselines when I did the sonic tooling work anyway.

…content

RZ9978 (@addTagHelper/@removeTagHelper/@tagHelperPrefix are not valid in a
component) is raised during IR lowering so it no longer depends on tag-helper
discovery's position in the pipeline. It rode on the directive's content node,
which shrank the squiggle to the directive argument and gave an empty directive a
zero-width span after the keyword. Walk up from the content node to the directive
and span the whole directive -- matching the original discovery-phase behavior --
and add a test covering the empty-directive case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
Copilot AI review requested due to automatic review settings July 24, 2026 04:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

@chsienki
chsienki enabled auto-merge (squash) July 24, 2026 04:36
@chsienki
chsienki merged commit 0285ca0 into dotnet:features/sonic Jul 24, 2026
24 of 25 checks passed
chsienki added a commit that referenced this pull request Jul 29, 2026
#84670)

Builds on the decl/impl split now in `features/sonic` (#84577, #84605).

## What this does

Wires the resolution-independent **declaration half** of a splittable
Razor component through `RegisterPreCompilationSourceOutput`, so
tag-helper discovery runs over the real compilation instead of a
separate declaration compilation parsed from generated text. This is the
step that unlocks reusing the standard compilation's tag helpers.

Components the markup split can't partition (a header/arity directive
like `@inherits`/`@implements`/`@typeparam`, or class-body markup it
can't route) still go through the separate declaration engine. To keep
their **type** resolvable in the pre-compilation compilation — so a
split component that references one in C# (e.g. `[Parameter] public
Widget Child`) doesn't bind to an error type and fail consumers with
CS0246 — the split phase emits a bodiless **type shell** for them.

## Commits

1. **Consume the split declaration through pre-compilation source
output** — the core rewire: fast/slow tag-helper discovery split, decl
`#pragma checksum` suppression so markup-only edits keep discovery
cached, set-based test matching.
2. **Strip the generated-code banner from files a delegated code action
creates** — cohost fix: generate-type-in-a-new-file no longer copies the
`// <auto-generated/>` banner (from the decl's suppressed-checksum first
line) onto authored `.cs` files.
3. **Emit a type-shell declaration for fallback components** — the
bodiless type shell (namespace / class / modifiers / type-parameter
names only) so fallback types resolve without contributing a
discoverable surface.
4. **Add decl-baseline files for fallback component code-generation
tests** — `.decl.codegen.cs` / `.decl.ir.txt` baselines.
5. **Distinguish a fallback type-shell declaration from a real
declaration half** — `RazorCSharpDocument.IsStubDocument`, so tooling
(diagnostic translation) treats the member-less shell as absent; fixes
dropped IDE0005 unused-usings for fallback components.
6. **Add a skipped test for the fallback nested-delegate limitation** —
a known gap (a delegate nested in a fallback binds to an error type →
CS1503), tracked by #84646 and marked `PROTOTYPE(sonic)` so it can't
reach `main` unfixed.

## Testing

- Razor SG unit suite: **218 passed / 1 skipped** (`net10.0`).
- Cohost pull-diagnostics: **19/19**.
- Each commit builds in isolation (bisect-safe).

## Known follow-ups

- #84646 — nested delegate in a fallback component (the skipped test
above).

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/84670)

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: c45f67d5-6fe0-4f6b-8c68-c268dc7c0621
chsienki added a commit that referenced this pull request Aug 4, 2026
Retires the `PROTOTYPE(sonic)` markers under `src/Razor`, in four
independent commits.

### Un-skip two rendermode tests
`RenderMode_GenericComponent_CSharp10` and
`LanguageVersion_BreakingChange_8_0` were skipped as Sonic bugs, but
neither is a regression -- the behavior matches `main`. The generic case
is the known `@typeparam` + `@rendermode` limitation
(dotnet/razor#9683); the pre-11 case is the intentional "don't map the
expression, to avoid breaking hot reload" behavior. Un-skipped,
asserting current behavior with a comment explaining why.

### Un-skip the ImplementInterface cohost test -- Closes #84609
On `main`, ImplementInterface already interleaves the new members with
the existing ones, and the test expects that. A mid-branch baseline
change (during the declaration-engine era, when `@implements` split with
a render-only impl) flipped the expected order to append-at-end; the
classify-first reorder (#84605) then made `@implements` fall back to
unsplit, matching `main` again, so the stale baseline started failing
and got skipped. Restored `main`'s baseline.

### Retire the SONICDEV document-getter guardrails
The `#if SONICDEV` `[Obsolete]` markers nudged callers onto
`GetCSharpDocument(bool)` / `GetRequiredCSharpDocument(bool)`. The only
product callers left were in the source generator, so they move to the
bool overloads. Four convenience wrappers had zero callers and are
deleted. The three impl/decl getters were test-only, so they move to a
`RazorCodeDocumentTestExtensions` in the
`Microsoft.AspNetCore.Razor.Test.Common` friend assembly; with the last
markers gone, the now-inert SONICDEV build toggle comes out too.

### Not addressed
The fallback-component nested-delegate metadata loss (#84646) stays
skipped as a known issue -- it's narrow and fails loudly at compile
time. Both fix options are sized in a comment on that issue.

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/84743)

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b8188d9c-18d2-4e12-8b25-4c8d99114193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants