Skip to content

Always pass C# formatting options to Razor functions - #84795

Merged
davidwengier merged 7 commits into
dotnet:mainfrom
davidwengier:thread-razor-csharp-formatting-options
Aug 7, 2026
Merged

Always pass C# formatting options to Razor functions#84795
davidwengier merged 7 commits into
dotnet:mainfrom
davidwengier:thread-razor-csharp-formatting-options

Conversation

@davidwengier

@davidwengier davidwengier commented Aug 6, 2026

Copy link
Copy Markdown
Member

We already supported C# formatting options in Razor formatting and auto insert, this completes the picture with completion and code action resolve. It also makes things non-nullable, since it now covers all features and there is only one editor to worry about.

Should make future editorconfig work a bit more sensible too. Maybe.

Microsoft Reviewers: Open in CodeFlow

davidwengier and others added 5 commits August 6, 2026 15:18
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Default the options and remove obsolete null checks and fallback reconstruction.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Copilot AI review requested due to automatic review settings August 6, 2026 21:57
@davidwengier
davidwengier requested a review from a team as a code owner August 6, 2026 21:57
@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.

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 encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
Copilot AI review requested due to automatic review settings August 6, 2026 22:58

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.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 10957fef-33ae-4f41-962c-6e56de4b0369
@davidwengier
davidwengier requested a review from a team as a code owner August 7, 2026 00:26
Copilot AI review requested due to automatic review settings August 7, 2026 00:26

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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/Razor/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/CSharpFormattingOptionsHelper.cs:15

  • GetCSharpSyntaxFormattingOptions dereferences services.ExportProvider.GetService<IGlobalOptionService>() without a null check. GetService<T> is used elsewhere as an optional lookup and can return null, which would make this method throw a NullReferenceException in environments missing the global option service (e.g., some tests/host compositions). Consider falling back to CSharpSyntaxFormattingOptions.Default (or using a required-get API) to make the failure mode deterministic and avoid NREs.
    internal static CSharpSyntaxFormattingOptions GetCSharpSyntaxFormattingOptions(SolutionServices services)
        => (CSharpSyntaxFormattingOptions)services.ExportProvider.GetService<IGlobalOptionService>()
            .GetSyntaxFormattingOptions(services.GetLanguageServices(LanguageNames.CSharp));

.github/copilot-instructions.md:68

  • The added guidance claims “CS1998 is not active for this repository”, but the repo contains many #pragma warning disable 1998 suppressions, which indicates the warning is active in at least some projects. This line is likely to mislead contributors/agents into introducing new CS1998 warnings. Consider rewording to reflect the actual policy (e.g., whether CS1998 is tolerated but still produced, or only suppressed in specific layers/tests).
- It is acceptable to have async methods with no awaits. CS1998 is not active for this repository.

@davidwengier
davidwengier enabled auto-merge August 7, 2026 00:36
@davidwengier
davidwengier merged commit 59d60a4 into dotnet:main Aug 7, 2026
24 of 25 checks passed
@davidwengier
davidwengier deleted the thread-razor-csharp-formatting-options branch August 7, 2026 01:49
JoeRobich pushed a commit that referenced this pull request Aug 11, 2026
Backs the `features/sonic` decl/impl split out of `main` to unblock
insertion. Reverts the merge (#84778) and the commits that stacked on
top of it and cannot stand without it.

## What reverts

- **#84778** `Merge/sonic into main` -- the whole feature body (reverted
with `-m 1`, keeping pre-sonic `main`).
- **#84818** fallback child-content discovery fix -- patches sonic's
fast/slow discovery, which no longer exists.
- **#84813** restore `GetCSharpDocument()`/`GeneratedCode` -- these were
re-added to shim sonic's API removal; reverting the merge restores the
originals, so the shim would double-define them.
- **#84795** always pass C# formatting options -- edits
`CSharpFormattingOptionsHelper.cs`, a file that only exists because of
sonic.

## What is kept

The other post-merge commits are independent of sonic and stay: arcade
bumps, SDL param removal, CS8802, #84758 (syntax-tree sharing), and the
two Razor features that merely shared files -- **#84760**
(unbound-attribute warnings) and **#84764** (empty-outer-tag warning).

Preserving #84760 needed one manual resolution in
`UnresolvedAttributeIntermediateNode.cs`: sonic added the
`CloneNode()`/`Clone()` overrides and #84760 later added one property
assignment inside `CloneNode`. The overrides are sonic infrastructure
and go with the revert; #84760's `IsDirectiveAttributeCandidate`
property and all its real usages are untouched.

## Validation

- `Razor.slnf` builds clean (compiler + all tooling projects, including
the ones #84795 touched).
- `Microsoft.NET.Sdk.Razor.SourceGenerators.UnitTests`: 215 passed.
- `Microsoft.AspNetCore.Razor.Language.UnitTests` (component / tag
helper / bind / unbound-attribute): 1593 passed -- confirms #84760's
kept feature still works.

## Note

Reverting the merge means re-landing `features/sonic` later will need
this revert itself reverted first -- expected for a back-out.

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/dotnet/roslyn/pull/84831)
chsienki added a commit that referenced this pull request Aug 21, 2026
…#84844)

Re-lands the sonic decl/impl split (backed out in #84831 to unblock
insertions) together with the slow-discovery perf work (#84808) and a
new fix that removes the remaining cold-allocation regression on
fallback components.

## Why this is safe to re-land now

The split was reverted because a VS insertion showed a cold-allocation
regression on fallback-heavy scenarios (Component Classification,
cohosting completion) with no offsetting speed gain. This branch
addresses the root cause rather than just re-applying the revert.

## What's here

1. **Un-revert** of #84831 -- restores the sonic merge and the fixes
that landed on top of it (#84818 fallback child-content discovery,
#84813 RZC public-API compat, #84795 formatting options).
2. **#84808** -- slow discovery resolves fallback types via the
declaration table (`GetSymbolsWithName`) instead of a per-tree semantic
model, and discovers only the fallback types rather than re-walking the
whole augmented compilation. (Removed the +535 MB semantic-model
regression.)
3. **Reuse the initial parse for fallback discovery** (new) -- the
remaining regression.

## The new fix

A fallback component (an `@inherits`/`@implements`/`@typeparam` header,
or body markup the analysis can't route) can't be split, so its
descriptor comes from discovery over an augmented compilation. Each
fallback component was being processed **twice**: the generation
engine's initial parse, then a freshly created declaration engine that
**re-parsed the same source** just to produce a discoverable decl. For
an app where nearly every component `@inherits` a base (MudBlazor,
OrchardCore) every component is a fallback, so that second parse
dominates cold discovery allocation.

The split phase already builds the discoverable declaration surface for
a *split* component from the nodes it just classified. This builds the
same surface for a *fallback* component and stashes it on the document
node; the generator lowers it in place -- reusing the initial parse --
instead of re-parsing through a separate engine. It's never emitted to
pre-compilation (the bodiless type shell still fills that role for C#
type resolution); only its syntax tree feeds slow discovery. The rare
shapes with no render method or namespace keep the re-parse path.

## Measurements

Cold source-generator allocation over 100 fallback components (in-memory
A/B):

| | Allocated |
|---|---|
| Pre-split baseline | 17.1 MB |
| Sonic + #84808 (before this fix) | 19.6 MB |
| **This branch** | **13.6 MB** |

The fallback path is now **below** the pre-split baseline -- a genuine
win, not break-even.

## Validation

- `Microsoft.NET.Sdk.Razor.SourceGenerators.UnitTests`: 219 passed / 1
skipped (incl. #84818's fallback child-content test).
- `Microsoft.AspNetCore.Razor.Language.UnitTests` (component / tag
helper / split / bind / discovery / clone): 1672 passed.
- Razor compiler builds clean.

Draft pending the RPS + Speedometer insertion run.

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

---

## Update: warm/interactive path fix (html-span completion)

A matched-baseline Speedometer re-run showed the cold-discovery
regressions gone but a smaller residual on **html-span completion in a
component** (`CLR_BytesAllocated_devenv`) -- the interactive/warm path,
a different signature than the cold source-generator work above.

Root cause: the fallback decl incremental pipeline compared decls by
**instance**, while the split-decl (`DeclSources`) fast path compares by
**text**. A fallback component's decl is markup-free and
checksum-suppressed, so a markup-only edit (every keystroke during
completion) yields a byte-identical decl -- but the instance changed
each run, invalidating the parse and, through the `Collect`, re-running
slow discovery over *every* fallback component on each edit.

Fix: compare fallback decls by text (`Text.ContentEquals`), mirroring
the fast path. A markup-only edit now leaves the fallback decl
`Unchanged`, keeping its parse and slow discovery cached.

Measured (100 fallback components, warm re-run on a markup edit):

| | warm alloc |
|---|---|
| Pre-split baseline | 0.41 MB |
| Before this fix | 3.2 MB (~8x) |
| **After this fix** | **0.42 MB** |

Guarded by
`IncrementalCompilation_WhenFallbackComponentMarkupChanges_SlowDiscoveryStaysCached`
(verified to fail without the comparer). Full SG suite 220 passed / 1
skipped.
@jjonescz jjonescz added this to the 18.11 milestone Aug 25, 2026
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.

4 participants