skills/dotnet__skills
1
0
Fork 0
mirror of https://github.com/dotnet/skills.git synced 2026-09-20 09:49:54 +08:00
Code Issues Packages Projects Releases Wiki Activity

Trim csharp-refactoring SKILL.md to lean v3 (proportional rigor)

Browse Source 
Replaces the shipped skill body with the validated v3 variant: judgment-first,
rigor proportional to blast radius, redundant catalog/list scaffolding removed.
~53% smaller (12,811->6,039 chars) with equal or better cross-family eval quality
and no measured regression. Description (929 chars) and all safety rules retained.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c9f9823-7f2f-4f7d-9d1b-f2b9e7a20c60
This commit is contained in:
Abhitej John
2026-07-21 00:35:22 -07:00
parent becad3451d
commit a3806f7d5f
1 changed files with 62 additions and 170 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/dotnet/skills/csharp-refactoring/SKILL.md
+62 -170
View File
@@ -6,188 +6,80 @@ license: MIT
# C# Refactoring (behavior-preserving)
Refactor C#/.NET code so the structure improves but the observable behavior does **not** change. Every
refactor is a sequence of small, named, Roslyn-aware transformations, each followed by a build/test
gate. If the gate fails, stop and revert — a refactor that changes behavior is a bug, not a refactor.
A refactor changes **structure**, never observable **behavior**. Do the edit with binding-aware tools,
then confirm behavior held with a build + the relevant tests. Keep the effort proportional to the change:
a one-line local rename does not need the ceremony a public multi-targeted change does.
## Operation catalog (what "refactoring" means in C#)
## First, is this actually a refactor?
The canonical C# refactoring operations, aligned with Roslyn's IDE refactoring providers:
The most valuable thing this skill does is *not* restructure code you were told to restructure — it is
catching a request that is **not** behavior-preserving before you run it through a refactor's contract.
When the request changes results, decline the refactor framing and handle it honestly:
- **Rename** a symbol / type / file
- **Move** a type / member / file (to another file, namespace, or project)
- **Consolidate / de-duplicate** copy-pasted code
- **Modernize / simplify** to current C# idioms (the repo's analyzers decide the idiom, not taste)
- **Split** a large class / file / assembly
- **Extract** a method / class / interface
- **Enable nullable** reference annotations
- **Inline** a method / local / constant
- **Pull up / push down** a member
- **Sync namespace** to folder
- **Framework / NuGet version bump** → not a refactor; redirect to the `dotnet-upgrade` skills.
- **New feature** (e.g. add a pricing tier, a flag, an endpoint) → a feature, not a refactor. If asked,
build it as a feature *with its own tests*; don't claim behavior is preserved.
- **Bug fix or "simplification" that changes output** (e.g. always charge shipping, bump a discount) →
a behavior **change**. It is a legitimate task — do it as an explicit, tested change and update the
tests that lock in the new behavior — but keep it **separate** from any refactor and never label it
behavior-preserving. Do not stall or report "nothing to change."
- **A rename/move with a behavior tweak smuggled in** ("rename X, and while you're there bump the rate")
→ split it: do the rename as a behavior-preserving refactor, and treat the tweak as its own tested
change, or flag it and defer.
Each is a single named operation; compose them one step at a time (see Procedure). For the Roslyn
provider mapping and representative real PRs, see
Only when the request is genuinely structure-only do you proceed as a refactor.
## Rename / move by bindings, not text
The #1 way a "rename" silently corrupts code is editing textual matches (comments, strings, unrelated
overloads) instead of real **bindings**. Find every binding reference first, then edit semantically. Use
the strongest tool available: an IDE/Roslyn workspace refactoring, then the C# LSP the
[`dotnet` plugin declares](https://github.com/dotnet/skills/blob/main/plugins/dotnet/lsp.json)
(`findReferences`, `goToDefinition`, `incomingCalls`, `rename` code action), then analyzer code-fixes /
Roslynator, then compiler-validated edits (edit the true bindings, rebuild, let the compiler flag misses).
Plain find/replace only when scope is provably tiny and every hit is verified. Include **every** `partial`
declaration, and edit the generator input, never generated (`*.g.cs`) output.
For the operation → Roslyn-provider mapping and representative PRs, see
[references/operation-catalog.md](references/operation-catalog.md).
## Tooling — C# LSP (Roslyn language server)
## Verify proportionally
A headless CLI agent usually can't invoke IDE code actions, but it **can** get Roslyn-quality
_semantic navigation_ from the C# language server the [`dotnet` plugin in `dotnet/skills`](https://github.com/dotnet/skills/blob/main/plugins/dotnet/lsp.json)
declares. It launches through the .NET CLI (`dnx roslyn-language-server --yes --prerelease -- --stdio
--autoLoadProjects`) over `.cs`, `.razor`, and `.cshtml` files (prerequisite: a .NET 10 SDK with
`dotnet` on `PATH`).
Confirm behavior is preserved after the edit — scaled to blast radius, not a fixed ceremony:
When the LSP is available, prefer its binding-aware operations over textual grep/glob for every
reference-finding step in a refactor:
- **Local / private** (method-local or `private` member, one file, single target framework, no public
surface, no `partial`/generated/`#if`): build once and run the **relevant** tests once after the edit.
If the tree is already known-green, don't burn a second full "before" baseline — rely on the post-edit
gate. Let the compiler catch missed references.
- **Cross-boundary** (public/shipped symbol, multi-targeted project, `#if`/platform branches, or
`partial`/generated code): build/test **each** target framework (a green default build can hide a break
on another TFM), and run the hazards check below.
- Where a symbol is defined → **goToDefinition**
- All _binding_ references to a symbol → **findReferences** (the reliable rename/move safety net)
- What calls a method → **incomingCalls**
- Find symbols by name across the workspace → **workspaceSymbol**
- A symbol's type / signature / docs → **hover**
Use the repo's own build/test workflow when it documents one (`README`/`CONTRIBUTING`, `build.*`, `eng/`,
`global.json`, `.github/workflows`); its instructions win over any generic command. Otherwise:
These are exact, binding-aware answers — unlike grep they don't match comments, string literals, or
unrelated overloads. Reach for the LSP first; fall back to grep only when it is unavailable.
```bash
dotnet build # 0 errors
dotnet test # stays green; same pass count as before
git restore . # if the gate fails, revert THIS step and reassess
```
## .NET compatibility hazards — inspect first, then load `dotnet-breaking-changes`
One operation per step; never mix a refactor and a behavior change in the same step. On red, revert — a
refactor that changes behavior is a bug, not a refactor.
Behavior-preserving edits fail in _.NET-specific_ ways a green test run won't catch. Before you edit,
**search the repo** for the surfaces that govern the symbol — don't assume or recite:
## Cross-boundary hazards (only when it touches a boundary)
- Public-API gate: `PublicAPI.Shipped/Unshipped.txt` (PublicApiAnalyzers) and/or `ApiCompat` /
`<EnablePackageValidation>` — these are _not_ interchangeable.
- `<TargetFrameworks>` and `#if` / platform branches.
- `partial` declarations and generated (`*.g.cs`) files.
- `InternalsVisibleTo` friend/test assemblies.
If — and only if — the change touches a **public** symbol, a **multi-targeted** project, or
`partial`/generated code, some breaks won't show up as a failing test. Search the repo for the surface
that governs the symbol (don't assume): the public-API gate (`PublicAPI.Shipped/Unshipped.txt` for
PublicApiAnalyzers, and/or `ApiCompat`/`<EnablePackageValidation>` — not interchangeable),
`<TargetFrameworks>`/`#if` branches, and `InternalsVisibleTo`. Move a public type via a `[TypeForwardedTo]`
forwarder; a *rename* needs an `[Obsolete]` shim, not a forwarder. For depth on any of these surfaces,
load the companion **dotnet-breaking-changes** skill. For a provably local/private change, skip this.
The four bullets above (and the reminders below) are the **floor**: apply them even if the guide skill
is not installed. When it _is_ available, **load the `dotnet-breaking-changes` skill** for the full
per-surface playbook (it applies to any change, not just refactors, and holds the depth on each
surface); it _adds_ compatibility analysis but does **not** replace this skill's safety contract.
Refactor-specific reminders: move a public type across assemblies via a `[TypeForwardedTo]` forwarder
(a _rename_ needs an `[Obsolete]` shim, not a forwarder); include _every_ `partial` declaration in a
rename/move; edit the generator input, not generated output; and treat friend-assembly `internal`
members with public-API care.
## Stop and ask when
## Stop and escalate (do not silently proceed)
Halt and ask the user before continuing when:
- The **baseline is red** (build or tests already failing) — you can't prove you preserved behavior.
- A **public/shipped API** would change and you cannot verify compatibility (no type-forwarder/shim
path, and no PublicApiAnalyzers/ApiCompat/package-validation gate available to catch a break).
- The request is **not actually behavior-preserving** — it asks you to upgrade a framework/package,
add a feature, or fix a bug. Do **not** carry it out under this skill, and **never label such a
change "behavior-preserving."** Say it is out of scope and redirect (upgrades → `dotnet-upgrade`;
features/fixes → normal dev flow). If a refactor is genuinely a prerequisite, do only that, as a
separate step, and stop.
- The edit touches **generated, designer, or migration files** (`*.g.cs`, `*.Designer.cs`, EF
migrations, source-generator output) — hand-edits there are **overwritten on the next build**;
change the source of generation (template/generator input), not the output.
- Semantic equivalence depends on **runtime behavior not covered by tests** (reflection, DI wiring,
serialization, `dynamic`, P/Invoke) — flag the risk; tests alone won't catch a regression.
- The operation can't be done with any tool on the fallback ladder and would require a wide,
unverifiable text replace.
## When to use
- "Rename this method/type everywhere safely" / "rename across the solution"
- Short requests such as "rename X to Y" or "rename this class"
- "Extract this block into a method" / "extract an interface from this class"
- "Move this type into its own file / into project X / into namespace Y"
- "This class is too big — split it" / "consolidate these two near-identical implementations"
- "Modernize this file to current C#" / "use file-scoped namespaces and primary constructors"
- "Turn on nullable annotations for this project and fix the warnings"
- Any request to **restructure / clean up / reorganize** code while keeping behavior identical
## When NOT to use
- The change is meant to alter behavior, add a feature, or fix a bug (not a refactor)
- Framework/SDK/NuGet version upgrades → `dotnet-upgrade`
- Pure formatting/whitespace → `dotnet format`
- Writing brand-new tests → `writing-mstest-tests` / `code-testing-agent`
## Procedure (the safety contract)
1. **Establish a green baseline.** Build the affected projects and run the relevant tests. Never
refactor on a red baseline — you won't be able to tell what you broke.
2. **Pick ONE operation from the catalog.** Refactors compose, but each step is a single named
operation. Never mix a refactor with a behavior change in the same step.
3. **Find the true references first (for rename/move/inline).** Before editing, locate every _binding_
reference to the symbol — not textual matches. Prefer the C# LSP: **findReferences** /
**goToDefinition** / **incomingCalls** (see **Tooling**) return only true bindings. If the LSP is
unavailable, a headless grep finds candidates but also false positives (test method names, comments,
strings, unrelated overloads); treat grep as a _candidate list_, then keep only true bindings and let
the compiler catch any you missed. Skipping this is the #1 way a "rename" silently corrupts code.
4. **Prefer semantics-aware edits over text edits.** The principle is _semantic verification_, not a
specific API — use the strongest tool actually available, in this order (fallback ladder):
1. An IDE / Roslyn workspace refactoring (rename, move, extract, inline, pull-up) when available —
updates all references, `using`s, and partial declarations correctly.
2. **The C# LSP (Roslyn language server) from `dotnet/skills`** (see **Tooling**) for a headless
agent: drive edits from **findReferences** / **goToDefinition** / **incomingCalls** /
**workspaceSymbol** so every touched reference is a true binding, not a textual guess. Some
language-server builds also expose a `textDocument/rename` code action that rewrites all
references at once — use it when present.
3. Analyzer code-fixes / `dotnet format analyzers` / **Roslynator** for idiom modernization and
de-duplication, when installed.
4. **Compiler-validated, reference-tracked edits** (the common headless fallback): use the LSP (or
grep, if no LSP) to enumerate candidates → edit only the true binding references from step 3 →
rebuild. The compiler is your safety net: any missed or wrongly-edited reference becomes a build
error, not a silent bug.
5. Plain find/replace ONLY when scope is provably tiny and every reference is verified — it
silently corrupts strings, comments, and unrelated overloads.
A CLI agent often won't have a loaded MSBuildWorkspace, but it **can** load the C# LSP above; prefer
its semantic answers over grep, and never skip the build/test gate to compensate.
5. **Re-gate after every step — across every target framework.** Rebuild + re-run tests; if red, revert
this step and reassess. The diff must be behavior-neutral: tests still green, no new warnings (nullable
work is the deliberate exception), no new analyzer/API-compat diagnostics, and the intended public
contract unchanged. On a multi-targeted project, build/test **each** TFM — a green default build can
still be broken on another target.
```bash
dotnet build # 0 errors, on every TargetFramework (add -f <tfm> to check one)
dotnet test # must stay green; same pass count as baseline
# if the gate fails, revert THIS step and reassess:
git restore . # or: git checkout -- <changed files>
```
6. **Keep the relevant contract stable — and which contract depends on the project type:**
- **Library / shipped package:** preserve the .NET public API; move public types across assembly
boundaries via type-forwarders or `[Obsolete]` shims, not breaking moves.
- **Application / service:** preserve the _external_ contract (HTTP routes, config keys, DB schema,
CLI args); internal type surface can move freely.
- **Small / private codebase:** preserve behavior + tests; commit granularity can relax.
7. **One refactor per commit** for libraries/large repos (keeps review + `git bisect` meaningful);
relax for small private codebases.
## Inputs
- Target scope: a symbol, file, type, project, or directory.
- The operation (from the catalog) — or infer it from the request.
- How to build and test the affected projects (solution/proj path, test command).
## Outputs
- The applied refactoring as a minimal, behavior-preserving diff.
- Proof of safety: before/after build + test results.
- A short rationale naming the operation(s) applied.
## Anti-patterns to avoid
- Renaming via text replace (hits strings, comments, unrelated overloads).
- "Refactor + small fix" in one step (a behavior change masquerading as a refactor).
- Moving a public type without a forwarder/shim (silent breaking change) — or refactoring a public
surface with no PublicApiAnalyzers/ApiCompat gate to catch a break.
- Editing only the TFM/`#if` branch your editor shows, leaving other targets broken.
- Renaming a `partial` type without its other declarations, or editing generated (`*.g.cs`) output
instead of the generator input.
- Skipping the test gate "because it's just a rename."
## Reference Files
- **[references/operation-catalog.md](references/operation-catalog.md)** — the full operation taxonomy
with Roslyn providers and representative real PRs.
**Load when** you need the provider for an operation or more detail on the catalog.
For .NET compatibility hazards (public API, multi-targeting/`#if`, source-generated/partial code,
`InternalsVisibleTo`), load the companion `dotnet-breaking-changes` skill and its `references/` — it
holds the depth on each surface and applies to any change, not just refactors.
- The baseline is already red (you can't prove you preserved behavior).
- A public/shipped API would change and there is no forwarder/shim path and no analyzer/ApiCompat gate.
- Equivalence depends on runtime behavior tests don't cover (reflection, DI, serialization, `dynamic`,
P/Invoke) — flag it.