From b2b34b084a48547c7ffb9201b67ecb741a91f360 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Otte=20S=C3=B8rensen?= Date: Wed, 12 Aug 2026 20:27:33 +0200 Subject: [PATCH 1/4] feat: Version the package with MinVer instead of a manual CI step. MinVer computes Version from the nearest git tag (v-prefixed, per Directory.Build.props) plus commit height, so packing by hand and CI publishing can no longer disagree, and the workflow's own "take the version from the tag" step is now unnecessary. verify-package.sh switches from -p:Version to -p:MinVerVersionOverride: MinVer recalculates and overwrites Version itself, so -p:Version was silently ignored and every verification run would have collided on the same package version - the exact bug that comment already warns about. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/build-and-publish.yml | 19 ++++++++++--------- Directory.Build.props | 7 +++++++ .../verify-package.sh | 6 +++++- Nota.CodeAnalysis/Nota.CodeAnalysis.csproj | 8 +++++--- 4 files changed, 27 insertions(+), 13 deletions(-) create mode 100644 Directory.Build.props diff --git a/.github/workflows/build-and-publish.yml b/.github/workflows/build-and-publish.yml index 087bc42..8a8c687 100644 --- a/.github/workflows/build-and-publish.yml +++ b/.github/workflows/build-and-publish.yml @@ -32,6 +32,11 @@ jobs: steps: - uses: actions/checkout@v7 + with: + # MinVer derives the version from git tags and the commit count above them, so it needs the + # actual history rather than actions/checkout's default shallow clone - which contains no + # tags to find. + fetch-depth: 0 - uses: actions/setup-dotnet@v6 with: @@ -41,17 +46,13 @@ jobs: # build with, and testing on what people use beats testing on the oldest thing that works. dotnet-version: '10.x' - # On a tag the tag is the version, so nothing in the repository can disagree with what shipped. - # Off a tag this is unset and expands to nothing, leaving the csproj's own - which is - # then only ever a local default, never the thing that gets published. - - name: Take the version from the tag - if: github.ref_type == 'tag' - run: echo "VERSION_ARG=-p:Version=${GITHUB_REF_NAME#v}" >> "$GITHUB_ENV" - # No NuGet.config and no secure file: every dependency is public on nuget.org. The Azure # pipeline this replaced downloaded a secure NuGet.config it did not need. + # + # No -p:Version here: MinVer sets it from the git tag (and the commit count above one, off a + # tag), so nothing in the repository can disagree with what shipped. - name: Build - run: dotnet build Nota.CodeAnalysis.sln --configuration Release $VERSION_ARG + run: dotnet build Nota.CodeAnalysis.sln --configuration Release - name: Verify the rules report run: ./Nota.CodeAnalysis.Verification/verify.sh @@ -67,7 +68,7 @@ jobs: run: ./Nota.CodeAnalysis.Verification/verify-package.sh - name: Pack - run: dotnet pack Nota.CodeAnalysis.sln --configuration Release --no-build --output artifacts $VERSION_ARG + run: dotnet pack Nota.CodeAnalysis.sln --configuration Release --no-build --output artifacts # Kept even on a pull request, so a packaging mistake is visible without merging. - name: Upload the package diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 0000000..77a55bd --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,7 @@ + + + + v + + diff --git a/Nota.CodeAnalysis.Verification/verify-package.sh b/Nota.CodeAnalysis.Verification/verify-package.sh index 791be3a..ebcf4d5 100755 --- a/Nota.CodeAnalysis.Verification/verify-package.sh +++ b/Nota.CodeAnalysis.Verification/verify-package.sh @@ -40,10 +40,14 @@ mkdir -p "$feed" "$app" # whatever 2.2.0 was extracted first - the change under test never reaches the consumer. That is not # hypothetical; this script gave a clean pass against a regression it was written to catch, until the # version was made unique. +# +# MinVerVersionOverride, not -p:Version - MinVer computes Version itself from the git tag and +# overwrites whatever was passed in, so -p:Version here would be silently ignored and every run +# would collide on the same version, reviving the exact bug this comment describes. version="0.0.0-verify.$(date +%Y%m%d%H%M%S)" printf 'Packing %s...\n' "$version" -dotnet pack "$root/Nota.CodeAnalysis.sln" --configuration Release --output "$feed" -p:Version="$version" >"$work/pack.log" 2>&1 || { +dotnet pack "$root/Nota.CodeAnalysis.sln" --configuration Release --output "$feed" -p:MinVerVersionOverride="$version" >"$work/pack.log" 2>&1 || { printf 'pack failed:\n' >&2 cat "$work/pack.log" >&2 exit 1 diff --git a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj index 492b3f0..c5c5cc9 100644 --- a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj +++ b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj @@ -4,9 +4,6 @@ true https://github.com/Notalib/Nota.CodeAnalysis true - - 2.2.1 content/README.md @@ -25,6 +22,11 @@ + + + From 1028bbf6aef8bcd9f8320e4006a33e5f2273dc03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Otte=20S=C3=B8rensen?= Date: Wed, 12 Aug 2026 20:43:36 +0200 Subject: [PATCH 2/4] docs: Explain MinVer-based versioning in the README. The "Working on this repository" section still described the version living in the tag and a local-default in the csproj - true before this change, wrong after it now that MinVer computes Version from the tag on every build and the property is gone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 53040a5..7416aa1 100644 --- a/README.md +++ b/README.md @@ -144,8 +144,12 @@ Releases are cut by tagging: git tag -a v2.3.0 -m "Nota.CodeAnalysis 2.3.0" && git push origin v2.3.0 ``` -The tag is the version, so nothing in the repository can disagree with what shipped. `` in -the csproj is only a local default, for anyone packing by hand. +[MinVer](https://github.com/adamralph/minver) derives the package version from that tag on every +build, so nothing in the repository can disagree with what shipped - there is no `` to edit +or forget to bump. Building straight off a tagged commit gets that tag's version exactly; anything +else gets the next patch as a pre-release with the commit count above the tag, e.g. `2.3.1-alpha.0.4`. +CI checks out full history (`fetch-depth: 0`) so MinVer can see the tags at all - a shallow clone has +none, and would fall back to `0.0.0-alpha.0`. Merging to `main` builds and verifies but does not publish, and neither do pull requests - releasing is a separate act from merging. From c06a76fbd1c1ab339fb91bc63e160f3946affe0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Thu, 13 Aug 2026 16:17:39 +0200 Subject: [PATCH 3/4] fix: Drop the two import keys that wedge dotnet format. dotnet_sort_system_directives_first and dotnet_separate_import_directive_groups are gone from the globalconfig. Not set to false - absent. Their presence at any value arms the organize-imports stage of "dotnet format style", which reports as "error IMPORTS: Fix imports ordering" and sorts flat-alphabetically after System, putting Nota above the vendors. That is the inverse of what UA1000 requires. The result is a loop rather than a wrong file. The style stage reorders, the analyzers stage puts it back, the file on disk never changes - so dotnet format reports nothing to do while dotnet format --verify-no-changes exits 2 forever, and a consuming repository cannot make CI green by any means available to it. The old note here reasoned that the per-vendor grouping was "checked by IDE0055 only, which is off here, so nothing reports it". That is true and beside the point: the imports stage is not a diagnostic, has no severity, and setting dotnet_diagnostic.IDE0055.severity = none does not reach it. It checked what reports, not what enforces. Rider is unaffected, which is why this only ever showed up for whoever formats from the command line. Measured against a real consumer with both of its .editorconfig files, not reasoned about: as shipped, three consecutive runs left the file byte-identical and --verify-no-changes exited 2 every time; with the keys removed, one pass produces the canonical layout and it exits 0. verify.sh asserted SA1516 for the blank line after the System group, and that only worked because the key was set - so removing it broke the suite, which is the suite doing its job. UA1001 makes that check now, and knows first party from vendor where SA1516 only ever saw first-level namespaces. Samples/Unseparated.cs carries both faults so SA1516 stays asserted on member separation, which was always its own job. Co-Authored-By: Claude Opus 5 --- Nota.CodeAnalysis.Verification/README.md | 27 +++++++++++------ .../Samples/Unseparated.cs | 26 +++++++++++++++++ Nota.CodeAnalysis.Verification/verify.sh | 8 +++-- .../content/Nota.CodeAnalysis.globalconfig | 29 ++++++++++++++----- README.md | 9 ++++++ 5 files changed, 80 insertions(+), 19 deletions(-) create mode 100644 Nota.CodeAnalysis.Verification/Samples/Unseparated.cs diff --git a/Nota.CodeAnalysis.Verification/README.md b/Nota.CodeAnalysis.Verification/README.md index 9ecbb11..17d91ba 100644 --- a/Nota.CodeAnalysis.Verification/README.md +++ b/Nota.CodeAnalysis.Verification/README.md @@ -48,15 +48,24 @@ looks redundant and is not: without it `IDE0005` silently stops reporting. ## What `verify.sh` asserts -`Samples/Broken.cs` breaks each of these deliberately. - -| Rule | What it catches | -|-----------|----------------------------------------------| -| `IDE0005` | an unused using - what CS8019 never did | -| `IDE0008` | `var` instead of an explicit type | -| `UA1000` | using directives out of order | -| `SA1208` | System usings not placed first | -| `SA1516` | no blank line after the System group | +`Samples/Broken.cs` and `Samples/Unseparated.cs` break each of these deliberately. + +| Rule | What it catches | Sample | +|-----------|--------------------------------------------------|------------------| +| `IDE0005` | an unused using - what CS8019 never did | `Broken.cs` | +| `IDE0008` | `var` instead of an explicit type | `Broken.cs` | +| `UA1000` | using directives out of order | `Broken.cs` | +| `SA1208` | System usings not placed first | `Broken.cs` | +| `UA1001` | no blank line between using blocks | `Unseparated.cs` | +| `SA1516` | no blank line between members | `Unseparated.cs` | + +`SA1516` used to be the one asserting the blank line after the System group, and that worked only +because `dotnet_separate_import_directive_groups` was set. The key had to go - at *any* value, +including `false`, its presence arms the organize-imports stage of `dotnet format style`, which sorts +first party above the vendors and leaves `dotnet format --verify-no-changes` failing permanently with +nothing a consumer can do about it. `UA1001` makes that check now, and distinguishes first party from +vendor where `SA1516` only ever saw first-level namespaces. `SA1516` stays asserted on member +separation, which was always its own job. ## What `verify-encoding.sh` asserts diff --git a/Nota.CodeAnalysis.Verification/Samples/Unseparated.cs b/Nota.CodeAnalysis.Verification/Samples/Unseparated.cs new file mode 100644 index 0000000..f1c4579 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/Unseparated.cs @@ -0,0 +1,26 @@ +using System; +using System.Xml; +using Nota.Vendor; + +namespace Nota.Verification; + +/// +/// Two deliberate faults, both asserted by verify.sh. +/// +/// The using directives are in the right order and have no blank line between the blocks, which +/// UA1001 reports. SA1516 used to report that, but only because +/// dotnet_separate_import_directive_groups was set - and setting that key at any value arms the +/// organize-imports stage of "dotnet format style", which sorts first party above the vendors and +/// leaves "dotnet format --verify-no-changes" failing forever. The key is gone; UA1001 makes the +/// check now, and knows first party from vendor where SA1516 only ever saw first-level namespaces. +/// +/// The two members below have no blank line between them either. That is SA1516's own job and +/// nothing to do with usings, so it stays asserted here rather than leaving with the key. They carry +/// no documentation comments on purpose: a comment between them would be reported by SA1514 instead, +/// and SA1600 is off here, so their absence costs nothing. +/// +public static class Unseparated +{ + public static string First => typeof(Thing).Name; + public static string Second => typeof(XmlDocument).Name + Environment.NewLine; +} diff --git a/Nota.CodeAnalysis.Verification/verify.sh b/Nota.CodeAnalysis.Verification/verify.sh index 2f7a395..529f38b 100755 --- a/Nota.CodeAnalysis.Verification/verify.sh +++ b/Nota.CodeAnalysis.Verification/verify.sh @@ -31,12 +31,16 @@ done # IDE0005 unused using directive the one CS8019 was supposed to cover # IDE0008 var instead of an explicit type # UA1000 using directives out of order UsingLayoutAnalyser +# UA1001 no blank line between using blocks - see Samples/Unseparated.cs. SA1516 used to cover +# this, but only because dotnet_separate_import_directive_groups was set, and that key +# had to go: at any value it arms dotnet format's organize-imports stage, which sorts +# first party above the vendors and leaves --verify-no-changes failing permanently # SA1208 System usings not placed first -# SA1516 no blank line after the System group +# SA1516 no blank line between members - its own job, not the using one it lost # NOTA0001 a source file that is not valid UTF-8, from build/Nota.CodeAnalysis.targets - the one # rule here that is a build error rather than an analyser diagnostic, and so the only # one that cannot be confirmed by reading a severity out of the globalconfig -expected=(IDE0005 IDE0008 UA1000 SA1208 SA1516 NOTA0001) +expected=(IDE0005 IDE0008 UA1000 UA1001 SA1208 SA1516 NOTA0001) # VerifyRules is what pulls Samples/ into the compilation. Without it the project builds empty, which # is what every other build of this solution wants. diff --git a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig index e604344..8ddd9a8 100644 --- a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig +++ b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig @@ -7,14 +7,27 @@ end_of_line = crlf # Stylecop settings ## Using Directives -dotnet_sort_system_directives_first = true - -# Half of this is enforced and half is not, so do not read it as a promise. SA1516 uses it to require -# the blank line after the System group, and that part works. The per-vendor grouping the name -# suggests - Microsoft together, Serilog together - is checked by IDE0055 only, which is off here, so -# nothing reports it. UsingLayoutAnalyser below is what actually enforces the grouping; this stays -# for the System boundary, and because turning it off would silence that too. -dotnet_separate_import_directive_groups = true +# +# dotnet_sort_system_directives_first and dotnet_separate_import_directive_groups are deliberately +# absent, and must stay absent. Not set to false - absent. Their presence at any value arms the +# organize-imports stage of "dotnet format style", which reports as "error IMPORTS: Fix imports +# ordering" and sorts flat-alphabetically after System. That puts Nota above the vendors, which is +# the inverse of the layout UA1000 enforces below. +# +# The result is a loop rather than a wrong file. The style stage reorders, the analyzers stage puts +# it back, the file on disk never changes - so "dotnet format" reports nothing to do while +# "dotnet format --verify-no-changes" exits 2 forever, and CI cannot be made green. Measured against +# a real consumer, not reasoned about. +# +# This stage is not a diagnostic. It has no ID and no severity, and setting +# dotnet_diagnostic.IDE0055.severity = none does not reach it - which is what the earlier note here +# got wrong: it checked what reports, not what enforces. Rider is unaffected, so this only ever bites +# whoever formats from the command line. +# +# Cost of their absence, measured: SA1516 no longer reports a missing blank line between using +# groups, which dotnet_separate_import_directive_groups = true did give us. UA1001 already requires +# that blank line and knows first party from vendor, where SA1516 only saw first-level namespaces, so +# what goes is the blunter duplicate of a check we still have. ## Using layout - UsingLayoutAnalyser (UA1000, UA1001) # System, then third party, then the consuming solution's own namespaces, as blocks separated by a diff --git a/README.md b/README.md index 7416aa1..280ce01 100644 --- a/README.md +++ b/README.md @@ -67,6 +67,15 @@ Most of this is unsurprising. These are the ones that catch people out: - **`UA1000` and `UA1001`** enforce the using layout: System, then third party, then yours, as blocks separated by a blank line, one run per vendor. An existing repository is converted in one pass with `dotnet format analyzers --diagnostics UA1000 UA1001 --severity warn`. +- **Do not set `dotnet_sort_system_directives_first` or `dotnet_separate_import_directive_groups`.** + Not even to `false`. This package leaves both keys out on purpose, and an `.editorconfig` entry + beats a global analyzer config entry, so putting one back is the one override here that breaks + something. Their presence at any value arms the organize-imports stage of `dotnet format style`, + which sorts flat-alphabetically after System and so puts your namespaces above the vendors - + the inverse of what `UA1000` requires. The two then take turns: the style stage reorders, the + analyzers stage puts it back, the file on disk never changes, and `dotnet format` reports nothing + to do while `dotnet format --verify-no-changes` exits 2 forever. It is not a diagnostic and no + severity setting reaches it. Rider is unaffected, so this only bites the command line and CI. ## Turning things off From f8dd79cf07898db480930f9016a4cf59dee1d8fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Wed, 7 Oct 2026 19:23:00 +0200 Subject: [PATCH 4/4] feat!: Report operators that lead a wrapped line (NOTA0002). A new analyser, a code fix that moves only the operator, and their unit tests. Both assemblies ship inside the package under analyzers/dotnet/cs, built against Roslyn 4.8 so SDK 8 consumers do not skip them with CS9057. The convention flips to match: dotnet_style_operator_placement_when_wrapping is end_of_line, and Rider's three csharp_wrap_before_* keys are set the same way. IDE0024 is also raised to a warning, so operator overloads get block bodies. Breaking: existing leading operators and expression-bodied operators become warnings. dotnet format converts both; the README's upgrade section has the commands. verify.sh and verify-package.sh now assert NOTA0002 reports, and that nothing reports the trailing form the fix produces. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/build-and-publish.yml | 6 + ...ta.CodeAnalysis.Analysers.CodeFixes.csproj | 28 ++ .../OperatorPlacementCodeFix.cs | 67 +++++ .../Nota.CodeAnalysis.Analysers.Test.csproj | 30 +++ .../OperatorPlacementAnalyserTests.cs | 128 +++++++++ .../OperatorPlacementTests.cs | 250 ++++++++++++++++++ .../AnalyzerReleases.Shipped.md | 3 + .../AnalyzerReleases.Unshipped.md | 8 + .../LeadingOperator.cs | 95 +++++++ .../Nota.CodeAnalysis.Analysers.csproj | 34 +++ .../OperatorPlacement.cs | 116 ++++++++ .../OperatorPlacementAnalyser.cs | 55 ++++ .../Nota.CodeAnalysis.Verification.csproj | 6 + Nota.CodeAnalysis.Verification/README.md | 34 ++- .../Samples/LeadingOperator.cs | 31 +++ .../verify-package.sh | 10 +- Nota.CodeAnalysis.Verification/verify.sh | 15 +- Nota.CodeAnalysis.sln | 42 +++ Nota.CodeAnalysis/Nota.CodeAnalysis.csproj | 26 ++ .../content/Nota.CodeAnalysis.globalconfig | 18 +- README.md | 44 ++- 21 files changed, 1029 insertions(+), 17 deletions(-) create mode 100644 Nota.CodeAnalysis.Analysers.CodeFixes/Nota.CodeAnalysis.Analysers.CodeFixes.csproj create mode 100644 Nota.CodeAnalysis.Analysers.CodeFixes/OperatorPlacementCodeFix.cs create mode 100644 Nota.CodeAnalysis.Analysers.Test/Nota.CodeAnalysis.Analysers.Test.csproj create mode 100644 Nota.CodeAnalysis.Analysers.Test/OperatorPlacementAnalyserTests.cs create mode 100644 Nota.CodeAnalysis.Analysers.Test/OperatorPlacementTests.cs create mode 100644 Nota.CodeAnalysis.Analysers/AnalyzerReleases.Shipped.md create mode 100644 Nota.CodeAnalysis.Analysers/AnalyzerReleases.Unshipped.md create mode 100644 Nota.CodeAnalysis.Analysers/LeadingOperator.cs create mode 100644 Nota.CodeAnalysis.Analysers/Nota.CodeAnalysis.Analysers.csproj create mode 100644 Nota.CodeAnalysis.Analysers/OperatorPlacement.cs create mode 100644 Nota.CodeAnalysis.Analysers/OperatorPlacementAnalyser.cs create mode 100644 Nota.CodeAnalysis.Verification/Samples/LeadingOperator.cs diff --git a/.github/workflows/build-and-publish.yml b/.github/workflows/build-and-publish.yml index 8a8c687..9135e11 100644 --- a/.github/workflows/build-and-publish.yml +++ b/.github/workflows/build-and-publish.yml @@ -54,6 +54,12 @@ jobs: - name: Build run: dotnet build Nota.CodeAnalysis.sln --configuration Release + # Unit tests for this repository's own analysers (NOTA0002): what they report, and that the + # code fix moves operators without changing anything but whitespace. The scripts below prove + # the rule reaches a consumer; these prove it is right when it gets there. + - name: Test the analysers + run: dotnet test Nota.CodeAnalysis.sln --configuration Release --no-build + - name: Verify the rules report run: ./Nota.CodeAnalysis.Verification/verify.sh diff --git a/Nota.CodeAnalysis.Analysers.CodeFixes/Nota.CodeAnalysis.Analysers.CodeFixes.csproj b/Nota.CodeAnalysis.Analysers.CodeFixes/Nota.CodeAnalysis.Analysers.CodeFixes.csproj new file mode 100644 index 0000000..1956d89 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers.CodeFixes/Nota.CodeAnalysis.Analysers.CodeFixes.csproj @@ -0,0 +1,28 @@ + + + + + netstandard2.0 + latest + enable + false + true + true + + + + + + + + + + + + diff --git a/Nota.CodeAnalysis.Analysers.CodeFixes/OperatorPlacementCodeFix.cs b/Nota.CodeAnalysis.Analysers.CodeFixes/OperatorPlacementCodeFix.cs new file mode 100644 index 0000000..9acdd2e --- /dev/null +++ b/Nota.CodeAnalysis.Analysers.CodeFixes/OperatorPlacementCodeFix.cs @@ -0,0 +1,67 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Text; + +namespace Nota.CodeAnalysis.Analysers.CodeFixes; + +/// +/// Fixes NOTA0002 by moving the operator to the end of the line before it. +/// +[ExportCodeFixProvider(Microsoft.CodeAnalysis.LanguageNames.CSharp, Name = nameof(OperatorPlacementCodeFix))] +[Shared] +public sealed class OperatorPlacementCodeFix : CodeFixProvider +{ + private const string Title = "Move the operator to the end of the line before"; + + /// + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(OperatorPlacementAnalyser.DiagnosticId); + + /// + public override FixAllProvider GetFixAllProvider() + { + return FixAllProvider.Create(FixAllAsync); + } + + /// + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + foreach (Diagnostic diagnostic in context.Diagnostics) + { + context.RegisterCodeFix( + CodeAction.Create(Title, + cancellationToken => FixAsync(context.Document, [diagnostic], cancellationToken), + nameof(OperatorPlacementCodeFix)), + diagnostic); + } + + return Task.CompletedTask; + } + + private static async Task FixAllAsync(FixAllContext context, Document document, ImmutableArray diagnostics) + { + return await FixAsync(document, diagnostics, context.CancellationToken).ConfigureAwait(false); + } + + private static async Task FixAsync(Document document, IEnumerable diagnostics, CancellationToken cancellationToken) + { + SyntaxNode? root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + SourceText text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false); + + if (root is null) + { + return document; + } + + HashSet positions = [.. diagnostics.Select(d => d.Location.SourceSpan.Start)]; + + return document.WithText(OperatorPlacement.Apply(root, text, positions)); + } +} diff --git a/Nota.CodeAnalysis.Analysers.Test/Nota.CodeAnalysis.Analysers.Test.csproj b/Nota.CodeAnalysis.Analysers.Test/Nota.CodeAnalysis.Analysers.Test.csproj new file mode 100644 index 0000000..d36f018 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers.Test/Nota.CodeAnalysis.Analysers.Test.csproj @@ -0,0 +1,30 @@ + + + + net10.0 + enable + false + Exe + + + + + + + + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + + + + + + + + + + + + + diff --git a/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementAnalyserTests.cs b/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementAnalyserTests.cs new file mode 100644 index 0000000..d0ce7ba --- /dev/null +++ b/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementAnalyserTests.cs @@ -0,0 +1,128 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Threading.Tasks; + +using AwesomeAssertions; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Nota.CodeAnalysis.Analysers.Test; + +public class OperatorPlacementAnalyserTests +{ + /// + /// Runs NOTA0002 over , placed inside a method, and returns the operators it + /// reported. + /// + internal static async Task> ReportedOperatorsAsync(string body) + { + IReadOnlyList diagnostics = await DiagnosticsAsync(Wrap(body)); + + return diagnostics.Select(d => d.Location.SourceTree!.GetText().ToString(d.Location.SourceSpan)).ToList(); + } + + internal static async Task> DiagnosticsAsync(string source) + { + SyntaxTree tree = CSharpSyntaxTree.ParseText(source, path: "Test.cs"); + CSharpCompilation compilation = CSharpCompilation.Create("Test", [tree]); + ImmutableArray diagnostics = await compilation.WithAnalyzers([new OperatorPlacementAnalyser()]) + .GetAnalyzerDiagnosticsAsync(TestContext.Current.CancellationToken); + + return diagnostics.Where(d => d.Id == OperatorPlacementAnalyser.DiagnosticId).ToList(); + } + + internal static string Wrap(string body) + { + return "class C\n{\n object M(bool a, bool b, int x, int y, string? s, object o)\n {\n" + body + "\n }\n}\n"; + } + + [Theory] + [InlineData("return a\n && b;", "&&")] + [InlineData("return a\n || b;", "||")] + [InlineData("return x\n + y;", "+")] + [InlineData("return x\n - y;", "-")] + [InlineData("return x\n * y;", "*")] + [InlineData("return x\n | y;", "|")] + [InlineData("return x\n != y;", "!=")] + [InlineData("return x\n < y;", "<")] + [InlineData("return s\n ?? \"\";", "??")] + [InlineData("return o\n as string;", "as")] + [InlineData("return o\n is int;", "is")] + [InlineData("return o\n is int i;", "is")] + [InlineData("return o is 1\n or 2;", "or")] + [InlineData("return o is not null\n and not 2;", "and")] + public async Task ReportsAnOperatorThatLeadsItsLine(string body, string op) + { + IReadOnlyList reported = await ReportedOperatorsAsync(body); + + reported.Should().Equal(op); + } + + [Fact] + public async Task ReportsBothHalvesOfAConditional() + { + IReadOnlyList reported = await ReportedOperatorsAsync("return a\n ? x\n : y;"); + + reported.Should().Equal("?", ":"); + } + + [Fact] + public async Task ReportsEachOperatorOfAChain() + { + IReadOnlyList reported = await ReportedOperatorsAsync("return a\n && b\n && a\n || b;"); + + reported.Should().HaveCount(3); + } + + [Theory] + [InlineData("return a &&\n b;")] + [InlineData("return a ?\n x :\n y;")] + [InlineData("return o is 1 or\n 2;")] + [InlineData("return a && b;")] + [InlineData("return a ? x : y;")] + [InlineData("return a\n .ToString();")] + [InlineData("System.Func f = ()\n => 1;\nreturn f;")] + [InlineData("return @\"x\n && y\";")] + [InlineData("// a\n// && b\nreturn a;")] + public async Task LeavesEverythingElse(string body) + { + IReadOnlyList reported = await ReportedOperatorsAsync(body); + + reported.Should().BeEmpty(); + } + + /// SA1128 wants a constructor initializer on its own line, and this must not argue. + [Fact] + public async Task LeavesAConstructorInitializerAndABaseList() + { + const string source = "class B\n{\n public B(int x)\n {\n }\n}\n\nclass D\n : B\n{\n public D()\n : base(1)\n {\n }\n}\n"; + + IReadOnlyList diagnostics = await DiagnosticsAsync(source); + + diagnostics.Should().BeEmpty(); + } + + [Fact] + public async Task LeavesGeneratedCode() + { + const string source = "// \nclass C\n{\n bool M(bool a, bool b)\n {\n return a\n && b;\n }\n}\n"; + + IReadOnlyList diagnostics = await DiagnosticsAsync(source); + + diagnostics.Should().BeEmpty(); + } + + [Fact] + public async Task ReportsAtTheOperator() + { + IReadOnlyList diagnostics = await DiagnosticsAsync(Wrap("return a\n && b;")); + + Diagnostic diagnostic = diagnostics.Should().ContainSingle().Subject; + diagnostic.Location.GetLineSpan().StartLinePosition.Should().Be(new Microsoft.CodeAnalysis.Text.LinePosition(5, 4)); + diagnostic.GetMessage(System.Globalization.CultureInfo.InvariantCulture).Should().StartWith("'&&' leads its line"); + diagnostic.Severity.Should().Be(DiagnosticSeverity.Warning); + } +} diff --git a/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementTests.cs b/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementTests.cs new file mode 100644 index 0000000..784b0c4 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers.Test/OperatorPlacementTests.cs @@ -0,0 +1,250 @@ +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using System.Threading.Tasks; + +using AwesomeAssertions; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Text; + +using Nota.CodeAnalysis.Analysers.CodeFixes; + +namespace Nota.CodeAnalysis.Analysers.Test; + +/// +/// Moving an operator: where it goes, where the operand it leaves behind ends up, and that nothing but +/// whitespace changes. +/// +public class OperatorPlacementTests +{ + private static string Rewrite(string source) + { + SyntaxTree tree = CSharpSyntaxTree.ParseText(source); + SourceText rewritten = OperatorPlacement.Apply(tree.GetRoot(), tree.GetText(), positions: null); + SyntaxTree after = CSharpSyntaxTree.ParseText(rewritten); + + SyntaxFactory.AreEquivalent(tree.GetRoot(), after.GetRoot(), topLevel: false) + .Should().BeTrue("moving an operator changes whitespace and nothing else"); + + return rewritten.ToString(); + } + + [Fact] + public void AnOperatorAlignedUnderTheFirstOperandLeavesTheOperandThere() + { + const string before = """ + if (a + && b + && c) + """; + const string after = """ + if (a && + b && + c) + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void AnOperatorHungInTheMarginLeavesTheOperandWhereItIs() + { + const string before = """ + x = "one" + + "two" + + "three"; + """; + const string after = """ + x = "one" + + "two" + + "three"; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void AConditionalIndentedUnderItsStatementKeepsTheIndent() + { + const string before = """ + var x = a + ? b + : c; + """; + const string after = """ + var x = a ? + b : + c; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void APatternCombinatorMoves() + { + const string before = """ + bool r = s is A + or B + or C; + """; + const string after = """ + bool r = s is A or + B or + C; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void TheOperatorGoesBeforeACommentEndingTheLine() + { + const string before = """ + bool r = a // why a + && b; + """; + const string after = """ + bool r = a && // why a + b; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void ACommentLineBetweenStaysWhereItIs() + { + const string before = """ + bool r = a + // why b + && b; + """; + const string after = """ + bool r = a && + // why b + b; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void AnOperatorAloneOnItsLineTakesTheLineWithIt() + { + const string before = """ + bool r = a + && + b; + """; + const string after = """ + bool r = a && + b; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void NestedOperatorsAllMove() + { + const string before = """ + var x = (a + || b) + ? p + + q + : r; + """; + const string after = """ + var x = (a || + b) ? + p + + q : + r; + """; + + Rewrite(before).Should().Be(after); + } + + [Fact] + public void AnOperatorBehindADirectiveIsLeftForAPerson() + { + const string before = """ + bool r = a + #if true + && b + #endif + ; + """; + + Rewrite(before).Should().Be(before); + } + + [Fact] + public void AnOperatorFollowedByACommentIsLeftForAPerson() + { + const string before = """ + bool r = a + && /* why */ b; + """; + + Rewrite(before).Should().Be(before); + } + + [Fact] + public void OnlyTheRequestedOperatorsMove() + { + const string before = """ + bool r = a + && b; + bool s = c + && d; + """; + SyntaxTree tree = CSharpSyntaxTree.ParseText(before, cancellationToken: TestContext.Current.CancellationToken); + int second = before.LastIndexOf("&&", System.StringComparison.Ordinal); + + string rewritten = OperatorPlacement.Apply(tree.GetRoot(TestContext.Current.CancellationToken), + tree.GetText(TestContext.Current.CancellationToken), + new HashSet { second }).ToString(); + + rewritten.Should().Be(""" + bool r = a + && b; + bool s = c && + d; + """); + } + + /// + /// The fix end to end, through the provider Rider calls: the diagnostic the analyser raises is one + /// the fix accepts, and once applied the analyser has nothing left to say. + /// + [Fact] + public async Task TheCodeFixClearsTheDiagnostic() + { + string source = OperatorPlacementAnalyserTests.Wrap("return a\n && b\n && a;"); + using AdhocWorkspace workspace = new(); + Document document = workspace.AddProject("Test", LanguageNames.CSharp).AddDocument("Test.cs", source); + IReadOnlyList diagnostics = await OperatorPlacementAnalyserTests.DiagnosticsAsync(source); + OperatorPlacementCodeFix fix = new(); + List actions = []; + + Diagnostic first = diagnostics.OrderBy(d => d.Location.SourceSpan.Start).First(); + + CodeFixContext context = new(document, first, (action, _) => actions.Add(action), TestContext.Current.CancellationToken); + await fix.RegisterCodeFixesAsync(context); + ImmutableArray operations = await actions.Should().ContainSingle().Subject + .GetOperationsAsync(TestContext.Current.CancellationToken); + Solution fixedSolution = operations.OfType().Single().ChangedSolution; + SourceText fixedText = await fixedSolution.GetDocument(document.Id)!.GetTextAsync(TestContext.Current.CancellationToken); + + fixedText.ToString().Should().Contain("return a &&\n b\n && a;", + "only the operator the diagnostic names moves"); + fix.FixableDiagnosticIds.Should().Equal(OperatorPlacementAnalyser.DiagnosticId); + fix.GetFixAllProvider().Should().NotBeNull(); + } +} diff --git a/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Shipped.md b/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Shipped.md new file mode 100644 index 0000000..60c1edf --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Shipped.md @@ -0,0 +1,3 @@ +; Shipped analyzer releases +; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md + diff --git a/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Unshipped.md b/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Unshipped.md new file mode 100644 index 0000000..46984d8 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/AnalyzerReleases.Unshipped.md @@ -0,0 +1,8 @@ +; Unshipped analyzer release +; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|------- +NOTA0002 | Layout | Warning | OperatorPlacementAnalyser diff --git a/Nota.CodeAnalysis.Analysers/LeadingOperator.cs b/Nota.CodeAnalysis.Analysers/LeadingOperator.cs new file mode 100644 index 0000000..259c1d6 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/LeadingOperator.cs @@ -0,0 +1,95 @@ +using System.Collections.Generic; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Text; + +namespace Nota.CodeAnalysis.Analysers; + +/// +/// An operator that begins a wrapped line instead of ending the line before it. +/// +public readonly struct LeadingOperator +{ + /// + /// Initializes a new instance of the struct. + /// + /// The operator token. + /// The first token of the leftmost operand the operator joins. + public LeadingOperator(SyntaxToken op, SyntaxToken firstOperand) + { + Operator = op; + FirstOperand = firstOperand; + } + + /// Gets the operator token. + public SyntaxToken Operator { get; } + + /// + /// Gets the first token of the leftmost operand the operator joins - where a hand-aligned + /// expression lines its operands up. + /// + public SyntaxToken FirstOperand { get; } + + /// + /// Every operator in that is the first token on its line. + /// + /// + /// Binary operators, the pattern combinators and and or, is, and both halves + /// of a conditional. Not the : of a constructor initializer or a base list, which SA1128 + /// puts on its own line, nor the . of a call chain or =>, which are not + /// operators joining two operands. + /// + /// The syntax tree's root. + /// The text was parsed from. + /// The leading operators, in document order within each expression. + public static IEnumerable FindAll(SyntaxNode root, SourceText text) + { + foreach (SyntaxNode node in root.DescendantNodes()) + { + switch (node) + { + case BinaryExpressionSyntax binary when LeadsItsLine(binary.OperatorToken, text): + yield return new LeadingOperator(binary.OperatorToken, binary.Left.GetFirstToken()); + break; + + case BinaryPatternSyntax pattern when LeadsItsLine(pattern.OperatorToken, text): + yield return new LeadingOperator(pattern.OperatorToken, pattern.Left.GetFirstToken()); + break; + + case IsPatternExpressionSyntax isPattern when LeadsItsLine(isPattern.IsKeyword, text): + yield return new LeadingOperator(isPattern.IsKeyword, isPattern.Expression.GetFirstToken()); + break; + + case ConditionalExpressionSyntax conditional: + SyntaxToken condition = conditional.Condition.GetFirstToken(); + + if (LeadsItsLine(conditional.QuestionToken, text)) + { + yield return new LeadingOperator(conditional.QuestionToken, condition); + } + + if (LeadsItsLine(conditional.ColonToken, text)) + { + yield return new LeadingOperator(conditional.ColonToken, condition); + } + + break; + } + } + } + + private static bool LeadsItsLine(SyntaxToken token, SourceText text) + { + SyntaxToken previous = token.GetPreviousToken(); + + if (previous.RawKind == 0 || token.IsMissing) + { + return false; + } + + int previousLine = text.Lines.GetLineFromPosition(previous.Span.End).LineNumber; + + return previousLine < text.Lines.GetLineFromPosition(token.SpanStart).LineNumber; + } +} diff --git a/Nota.CodeAnalysis.Analysers/Nota.CodeAnalysis.Analysers.csproj b/Nota.CodeAnalysis.Analysers/Nota.CodeAnalysis.Analysers.csproj new file mode 100644 index 0000000..3a477ec --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/Nota.CodeAnalysis.Analysers.csproj @@ -0,0 +1,34 @@ + + + + + netstandard2.0 + latest + enable + false + true + true + + + + + + + + + + + + + diff --git a/Nota.CodeAnalysis.Analysers/OperatorPlacement.cs b/Nota.CodeAnalysis.Analysers/OperatorPlacement.cs new file mode 100644 index 0000000..4b5406e --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/OperatorPlacement.cs @@ -0,0 +1,116 @@ +using System.Collections.Generic; +using System.Linq; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Text; + +namespace Nota.CodeAnalysis.Analysers; + +/// +/// Moves a leading operator to the end of the line before it. +/// +/// +/// +/// Here rather than in the code fix so that it needs nothing beyond the compiler: the fix and a +/// whole-tree rewrite call the same code. +/// +/// +/// The operand left behind takes the operator's column, which keeps an operator that was aligned +/// under the first operand aligned. The exception is an operator hung in the margin so that the +/// operands line up - return a over + b - where the operand is already where it +/// belongs and stays there. +/// +/// +public static class OperatorPlacement +{ + /// + /// Moves every leading operator in whose position is in + /// , or every one when is null. + /// + /// + /// An operator with a comment or directive where the edits would go is left for a person, and the + /// analyser goes on reporting it. + /// + /// The syntax tree's root. + /// The text was parsed from. + /// Operator start positions to move, or null for all of them. + /// The rewritten text; itself when nothing could be moved. + public static SourceText Apply(SyntaxNode root, SourceText text, ISet? positions) + { + List changes = []; + + // How far each rewritten line's operand moved left. An operand lined up under one that has + // itself moved has to move with it, and the first operand is always on an earlier line. + Dictionary shifts = []; + + foreach (LeadingOperator leading in LeadingOperator.FindAll(root, text) + .Where(l => positions is null || positions.Contains(l.Operator.SpanStart)) + .OrderBy(l => l.Operator.SpanStart)) + { + SyntaxToken op = leading.Operator; + SyntaxToken previous = op.GetPreviousToken(); + SyntaxToken next = op.GetNextToken(); + TextLine line = text.Lines.GetLineFromPosition(op.SpanStart); + + if (op.LeadingTrivia.Any(IsDirective) || + op.TrailingTrivia.Any(t => !IsSpace(t)) || + previous.TrailingTrivia.Any(IsDirective) || + !string.IsNullOrWhiteSpace(text.ToString(TextSpan.FromBounds(line.Start, op.SpanStart)))) + { + continue; + } + + changes.Add(new TextChange(new TextSpan(previous.Span.End, 0), " " + op.Text)); + + if (text.Lines.GetLineFromPosition(next.SpanStart).LineNumber != line.LineNumber) + { + changes.Add(new TextChange(line.SpanIncludingLineBreak, string.Empty)); + continue; + } + + int opColumn = op.SpanStart - line.Start; + int nextColumn = next.SpanStart - line.Start; + int firstColumn = Column(text, leading.FirstOperand.SpanStart); + int firstLine = text.Lines.GetLineFromPosition(leading.FirstOperand.SpanStart).LineNumber; + int inherited = 0; + + if (shifts.TryGetValue(firstLine, out (int operandStart, int shift) moved) && + leading.FirstOperand.SpanStart >= moved.operandStart) + { + inherited = moved.shift; + } + + int column = opColumn; + + if (nextColumn == firstColumn) + { + column = nextColumn - inherited; + } + else if (opColumn == firstColumn) + { + column = opColumn - inherited; + } + + changes.Add(new TextChange(TextSpan.FromBounds(line.Start, next.SpanStart), new string(' ', column))); + shifts[line.LineNumber] = (next.SpanStart, nextColumn - column); + } + + return changes.Count == 0 ? text : text.WithChanges(changes.OrderBy(c => c.Span.Start)); + } + + private static int Column(SourceText text, int position) + { + return position - text.Lines.GetLineFromPosition(position).Start; + } + + private static bool IsDirective(SyntaxTrivia trivia) + { + return trivia.IsDirective || trivia.IsKind(SyntaxKind.DisabledTextTrivia); + } + + private static bool IsSpace(SyntaxTrivia trivia) + { + return trivia.IsKind(SyntaxKind.WhitespaceTrivia) || trivia.IsKind(SyntaxKind.EndOfLineTrivia); + } +} diff --git a/Nota.CodeAnalysis.Analysers/OperatorPlacementAnalyser.cs b/Nota.CodeAnalysis.Analysers/OperatorPlacementAnalyser.cs new file mode 100644 index 0000000..52bcc13 --- /dev/null +++ b/Nota.CodeAnalysis.Analysers/OperatorPlacementAnalyser.cs @@ -0,0 +1,55 @@ +using System.Collections.Immutable; + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Text; + +namespace Nota.CodeAnalysis.Analysers; + +/// +/// NOTA0002: an operator that wraps a line ends the line it belongs to rather than leading the next. +/// +/// +/// dotnet_style_operator_placement_when_wrapping states the rule for Roslyn's formatter, and +/// Rider's csharp_wrap_before_* settings state it for Rider, but neither reports code that +/// already breaks it - IDE0055 steers a rewrap and does not audit what is there. This does. +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class OperatorPlacementAnalyser : DiagnosticAnalyzer +{ + /// The diagnostic id. + public const string DiagnosticId = "NOTA0002"; + + private static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + "Operator leads a wrapped line", + "'{0}' leads its line; move it to the end of the line before and leave the rest of the expression as it is", + "Layout", + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: "When an expression wraps, the operator ends the line it follows and the next line starts with the operand. " + + "Moving the operator is the whole fix: a wrapped condition or ternary is fine as it is and does not need " + + "rewriting as an if/else."); + + /// + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + /// + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxTreeAction(AnalyseTree); + } + + private static void AnalyseTree(SyntaxTreeAnalysisContext context) + { + SyntaxNode root = context.Tree.GetRoot(context.CancellationToken); + SourceText text = context.Tree.GetText(context.CancellationToken); + + foreach (LeadingOperator leading in LeadingOperator.FindAll(root, text)) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, leading.Operator.GetLocation(), leading.Operator.Text)); + } + } +} diff --git a/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj b/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj index 69286f4..17b2179 100644 --- a/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj +++ b/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj @@ -54,4 +54,10 @@ + + + + + diff --git a/Nota.CodeAnalysis.Verification/README.md b/Nota.CodeAnalysis.Verification/README.md index 17d91ba..3068f58 100644 --- a/Nota.CodeAnalysis.Verification/README.md +++ b/Nota.CodeAnalysis.Verification/README.md @@ -48,16 +48,27 @@ looks redundant and is not: without it `IDE0005` silently stops reporting. ## What `verify.sh` asserts -`Samples/Broken.cs` and `Samples/Unseparated.cs` break each of these deliberately. - -| Rule | What it catches | Sample | -|-----------|--------------------------------------------------|------------------| -| `IDE0005` | an unused using - what CS8019 never did | `Broken.cs` | -| `IDE0008` | `var` instead of an explicit type | `Broken.cs` | -| `UA1000` | using directives out of order | `Broken.cs` | -| `SA1208` | System usings not placed first | `Broken.cs` | -| `UA1001` | no blank line between using blocks | `Unseparated.cs` | -| `SA1516` | no blank line between members | `Unseparated.cs` | +The files in `Samples/` break each of these deliberately. + +| Rule | What it catches | Sample | +|------------|--------------------------------------------------|----------------------| +| `IDE0005` | an unused using - what CS8019 never did | `Broken.cs` | +| `IDE0008` | `var` instead of an explicit type | `Broken.cs` | +| `UA1000` | using directives out of order | `Broken.cs` | +| `SA1208` | System usings not placed first | `Broken.cs` | +| `UA1001` | no blank line between using blocks | `Unseparated.cs` | +| `SA1516` | no blank line between members | `Unseparated.cs` | +| `NOTA0001` | a source file that is not valid UTF-8 | `Latin1Encoded.cs` | +| `NOTA0002` | an operator leading a wrapped line | `LeadingOperator.cs` | + +`LeadingOperator.cs` also wraps the same expression the way `NOTA0002` wants, and `verify.sh` fails if +anything at all reports on it. That is what its code fix produces, and a fix that some other rule - +StyleCop's `SA1003`, say, or `IDE0055` - wants undone is a loop rather than a convention. Nothing does +today; this is what would say so if a StyleCop upgrade changed that. + +`NOTA0002` comes from `Nota.CodeAnalysis.Analysers` in this repository, referenced as an analyser +rather than through the package. Its own behaviour is covered by the unit tests in +`Nota.CodeAnalysis.Analysers.Test`; this only proves it loads and reports. `SA1516` used to be the one asserting the blank line after the System group, and that worked only because `dotnet_separate_import_directive_groups` was set. The key had to go - at *any* value, @@ -105,7 +116,8 @@ while this project was being written, both times caught before merging: So it packs, installs into a throwaway project from a local feed, and compiles a file that breaks one rule per analyser - plus a deliberately mis-encoded file for `NOTA0001`, which proves -`build/Nota.CodeAnalysis.targets` was packed and imported. It fails on `CS9057` too, since that is a +`build/Nota.CodeAnalysis.targets` was packed and imported. `NOTA0002` proves the package's own +analyser assembly was packed under `analyzers/dotnet/cs`, which nothing inside the solution can see. It fails on `CS9057` too, since that is a warning nothing else would notice. It packs under a throwaway version like `0.0.0-verify.20260802143000`. That is not cosmetic: NuGet diff --git a/Nota.CodeAnalysis.Verification/Samples/LeadingOperator.cs b/Nota.CodeAnalysis.Verification/Samples/LeadingOperator.cs new file mode 100644 index 0000000..7b4d3fd --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/LeadingOperator.cs @@ -0,0 +1,31 @@ +namespace Nota.Verification; + +/// +/// An operator leading a wrapped line, which NOTA0002 reports, asserted by verify.sh. +/// +/// The second method wraps the same expression the way the rule wants, and must stay quiet - from +/// NOTA0002 and from StyleCop alike, since a rule whose fix another rule reports is a loop rather +/// than a convention. +/// +public static class LeadingOperator +{ + /// The operator leads the second line. + /// The first operand. + /// The second operand. + /// Both. + public static bool Leading(bool a, bool b) + { + return a + && b; + } + + /// The operator ends the first line. + /// The first operand. + /// The second operand. + /// Both. + public static bool Trailing(bool a, bool b) + { + return a && + b; + } +} diff --git a/Nota.CodeAnalysis.Verification/verify-package.sh b/Nota.CodeAnalysis.Verification/verify-package.sh index ebcf4d5..2f02b21 100755 --- a/Nota.CodeAnalysis.Verification/verify-package.sh +++ b/Nota.CodeAnalysis.Verification/verify-package.sh @@ -113,6 +113,13 @@ public static class Broken /// An unbound property is Serilog003, from SerilogAnalyzer. public static void LogIt() => Log.Information("Hello {Name}"); + + /// A leading operator is NOTA0002, from the package's own analyser. + public static bool Both(bool a, bool b) + { + return a + && b; + } } EOF @@ -138,7 +145,8 @@ output="$(cd "$app" && dotnet build --no-incremental -v:m 2>&1 || true)" # VSTHRD100 Microsoft.VisualStudio.Threading.Analyzers reached the consumer # UA1000 UsingLayoutAnalyser reached the consumer, and loaded on this Roslyn # Serilog003 SerilogAnalyzer reached the consumer -expected="IDE0008 NOTA0001 SA1208 VSTHRD100 UA1000 Serilog003" +# NOTA0002 the package's own analyser was packed under analyzers/dotnet/cs, and loaded on this Roslyn +expected="IDE0008 NOTA0001 NOTA0002 SA1208 VSTHRD100 UA1000 Serilog003" missing="" for rule in $expected; do diff --git a/Nota.CodeAnalysis.Verification/verify.sh b/Nota.CodeAnalysis.Verification/verify.sh index 529f38b..39a3161 100755 --- a/Nota.CodeAnalysis.Verification/verify.sh +++ b/Nota.CodeAnalysis.Verification/verify.sh @@ -40,7 +40,9 @@ done # NOTA0001 a source file that is not valid UTF-8, from build/Nota.CodeAnalysis.targets - the one # rule here that is a build error rather than an analyser diagnostic, and so the only # one that cannot be confirmed by reading a severity out of the globalconfig -expected=(IDE0005 IDE0008 UA1000 UA1001 SA1208 SA1516 NOTA0001) +# NOTA0002 an operator leading a wrapped line - see Samples/LeadingOperator.cs. From this +# repository's own analyser, so it also proves the analyser project loads at all +expected=(IDE0005 IDE0008 UA1000 UA1001 SA1208 SA1516 NOTA0001 NOTA0002) # VerifyRules is what pulls Samples/ into the compilation. Without it the project builds empty, which # is what every other build of this solution wants. @@ -53,6 +55,17 @@ for rule in "${expected[@]}"; do fi done +# 3. Samples/LeadingOperator.cs wraps the same expression both ways. The leading operator is the one +# NOTA0002 reports; the trailing one is what its fix produces, and nothing may report that - not +# NOTA0002, and not a StyleCop or formatting rule wanting the operator back where it was. A rule +# whose fix another rule reports is a loop, not a convention. +leading="$(grep 'LeadingOperator\.cs(' <<<"$output" | sed 's/ \[.*//' | sort -u || true)" +unexpected="$(grep -v 'LeadingOperator\.cs(19,13): warning NOTA0002:' <<<"$leading" || true)" + +if [ -n "$unexpected" ]; then + failures+=("Samples/LeadingOperator.cs reported more than the one leading operator:"$'\n'"$unexpected") +fi + if [ ${#failures[@]} -ne 0 ]; then printf 'Verification failed:\n' >&2 printf ' - %s\n' "${failures[@]}" >&2 diff --git a/Nota.CodeAnalysis.sln b/Nota.CodeAnalysis.sln index 10f67ea..51e3909 100644 --- a/Nota.CodeAnalysis.sln +++ b/Nota.CodeAnalysis.sln @@ -7,6 +7,12 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Nota.CodeAnalysis", "Nota.C EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Nota.CodeAnalysis.Verification", "Nota.CodeAnalysis.Verification\Nota.CodeAnalysis.Verification.csproj", "{1053892B-03AC-49E2-8E7F-4E83D05A835D}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Nota.CodeAnalysis.Analysers", "Nota.CodeAnalysis.Analysers\Nota.CodeAnalysis.Analysers.csproj", "{C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Nota.CodeAnalysis.Analysers.CodeFixes", "Nota.CodeAnalysis.Analysers.CodeFixes\Nota.CodeAnalysis.Analysers.CodeFixes.csproj", "{E18676A5-1BBE-46B6-89C7-9D998958BADC}" +EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Nota.CodeAnalysis.Analysers.Test", "Nota.CodeAnalysis.Analysers.Test\Nota.CodeAnalysis.Analysers.Test.csproj", "{60A323E5-8CBC-4896-993B-2DE191673226}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -41,6 +47,42 @@ Global {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x64.Build.0 = Release|Any CPU {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x86.ActiveCfg = Release|Any CPU {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x86.Build.0 = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|Any CPU.Build.0 = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|x64.ActiveCfg = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|x64.Build.0 = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|x86.ActiveCfg = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Debug|x86.Build.0 = Debug|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|Any CPU.ActiveCfg = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|Any CPU.Build.0 = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|x64.ActiveCfg = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|x64.Build.0 = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|x86.ActiveCfg = Release|Any CPU + {C8110B1B-6C3D-4536-8DFA-C32A0DC1CBD5}.Release|x86.Build.0 = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|Any CPU.Build.0 = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|x64.ActiveCfg = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|x64.Build.0 = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|x86.ActiveCfg = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Debug|x86.Build.0 = Debug|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|Any CPU.ActiveCfg = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|Any CPU.Build.0 = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|x64.ActiveCfg = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|x64.Build.0 = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|x86.ActiveCfg = Release|Any CPU + {E18676A5-1BBE-46B6-89C7-9D998958BADC}.Release|x86.Build.0 = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|Any CPU.Build.0 = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|x64.ActiveCfg = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|x64.Build.0 = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|x86.ActiveCfg = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Debug|x86.Build.0 = Debug|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|Any CPU.ActiveCfg = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|Any CPU.Build.0 = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|x64.ActiveCfg = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|x64.Build.0 = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|x86.ActiveCfg = Release|Any CPU + {60A323E5-8CBC-4896-993B-2DE191673226}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj index c5c5cc9..c820265 100644 --- a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj +++ b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj @@ -29,6 +29,32 @@ + + + + + + + + $(TargetsForTfmSpecificContentInPackage);NotaPackAnalysers + + + + + + + + + + + + diff --git a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig index 8ddd9a8..e82dd61 100644 --- a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig +++ b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig @@ -107,7 +107,18 @@ dotnet_naming_style.pascal_case.required_prefix = dotnet_naming_style.pascal_case.required_suffix = dotnet_naming_style.pascal_case.word_separator = dotnet_naming_style.pascal_case.capitalization = pascal_case -dotnet_style_operator_placement_when_wrapping = beginning_of_line +# A wrapped expression ends its line with the operator, and the next line starts with the operand. +# dotnet_style_operator_placement_when_wrapping steers Roslyn's formatter when it rewraps, and the +# csharp_wrap_before_* keys say the same to Rider's. Neither reports code that is already wrapped +# the other way; NOTA0002, below, is what reports, and carries a code fix. +# +# The Rider keys are here in case Rider reads them from a global analyzer config, which is +# unverified - Rider certainly reads them from .editorconfig, and a repository whose Rider still +# wraps before the operator should copy these three lines there. Roslyn ignores them either way. +dotnet_style_operator_placement_when_wrapping = end_of_line +csharp_wrap_before_binary_opsign = false +csharp_wrap_before_binary_pattern_op = false +csharp_wrap_before_ternary_opsigns = false dotnet_style_coalesce_expression = true:suggestion dotnet_style_null_propagation = true:suggestion dotnet_style_prefer_is_null_check_over_reference_equality_method = true:suggestion @@ -150,7 +161,7 @@ csharp_style_prefer_top_level_statements = true:silent csharp_style_prefer_primary_constructors = true:suggestion csharp_style_expression_bodied_methods = false:silent csharp_style_expression_bodied_constructors = false:silent -csharp_style_expression_bodied_operators = false:silent +csharp_style_expression_bodied_operators = false:warning csharp_style_expression_bodied_properties = true:silent csharp_style_expression_bodied_indexers = true:silent csharp_style_expression_bodied_accessors = true:silent @@ -557,6 +568,9 @@ dotnet_diagnostic.IDE0063WithoutSuggestion.severity = none dotnet_diagnostic.IDE0066.severity = none dotnet_diagnostic.IDE0066WithoutSuggestion.severity = none dotnet_diagnostic.IDE0290.severity = none +# An operator leading a wrapped line, from this package's own analyser. Stated although warning is +# already its default, so that what it is set to can be read here like every other rule. +dotnet_diagnostic.NOTA0002.severity = warning dotnet_diagnostic.SA0000.severity = warning dotnet_diagnostic.SA1101.severity = none dotnet_diagnostic.SA1116.severity = none diff --git a/README.md b/README.md index 280ce01..7e3af82 100644 --- a/README.md +++ b/README.md @@ -35,8 +35,9 @@ Four properties are switched on, in `build/Nota.CodeAnalysis.props`: | `ImplicitUsings` | disabled - usings are stated, not inherited | Four analyser packages come as dependencies: StyleCop.Analyzers, Microsoft.VisualStudio.Threading.Analyzers, -SerilogAnalyzer, and UsingLayoutAnalyser. Around 400 rule severities are set in -`content/Nota.CodeAnalysis.globalconfig`. +SerilogAnalyzer, and UsingLayoutAnalyser. One more, Nota's own (`NOTA0002`, with its code fix), is +carried inside the package itself, built from `Nota.CodeAnalysis.Analysers`. Around 400 rule +severities are set in `content/Nota.CodeAnalysis.globalconfig`. ## Configuring it @@ -64,6 +65,12 @@ Most of this is unsurprising. These are the ones that catch people out: no warning at all and reaches the assembly as U+FFFD replacement characters. UTF-16 with a byte order mark passes, since the compiler reads it correctly - and such a file must keep its BOM, which is the only record of its encoding. +- **`NOTA0002`**: when an expression wraps, the operator ends the line and the next line starts + with the operand - `a &&` then `b`, not `a` then `&& b`. Binary operators, `is`/`as`, the pattern + combinators `and`/`or`, and both halves of `?:`; not `.`, `=>`, or the `:` of a base list or + constructor initializer. The code fix moves only the operator and leaves the expression alone - a + wrapped condition or ternary is fine as it is. An existing repository is converted in one pass + with `dotnet format analyzers --diagnostics NOTA0002 --severity warn`. - **`UA1000` and `UA1001`** enforce the using layout: System, then third party, then yours, as blocks separated by a blank line, one run per vendor. An existing repository is converted in one pass with `dotnet format analyzers --diagnostics UA1000 UA1001 --severity warn`. @@ -93,6 +100,32 @@ The encoding check is a build task rather than a diagnostic, so it has its own s false ``` +## Upgrading from 2.3 + +The operator placement convention has flipped. `dotnet_style_operator_placement_when_wrapping` was +`beginning_of_line` and is now `end_of_line`, and `NOTA0002` reports every operator that still leads +a wrapped line - as a warning, so a repository with `TreatWarningsAsErrors` will fail to build until +they are moved. One command moves them all, and changes nothing but whitespace: + +```sh +dotnet format analyzers --diagnostics NOTA0002 --severity warn +``` + +`IDE0024` is now a warning too: an operator overload has a block body, not `=>`. It was always the +preference, just never reported. `dotnet format style --diagnostics IDE0024 --severity warn` converts +them. + +The package also sets Rider's wrap keys, but whether Rider reads them from a global analyzer config +rather than `.editorconfig` is unverified. If Rider still puts the operator first when it wraps - +producing fresh warnings - add them to your `.editorconfig`: + +```ini +[*.cs] +csharp_wrap_before_binary_opsign = false +csharp_wrap_before_binary_pattern_op = false +csharp_wrap_before_ternary_opsigns = false +``` + ## Upgrading from 2.1 `SA1412`, which required every source file to carry a byte order mark, is off. It never did anything @@ -147,6 +180,13 @@ before changing rules. Both run on pull requests as well as on `main`. +Nota's own analysers live in `Nota.CodeAnalysis.Analysers`, their code fixes in +`Nota.CodeAnalysis.Analysers.CodeFixes` - a separate assembly, because a fix needs Workspaces and the +command-line compiler does not carry it. Both reference Roslyn 4.8 and must stay there or below: an +analyser built against a newer compiler than the consumer's SDK is skipped with `CS9057`, a warning, +and its rules silently vanish. `dotnet test` runs their unit tests in +`Nota.CodeAnalysis.Analysers.Test`; a new rule goes in `AnalyzerReleases.Unshipped.md` as well. + Releases are cut by tagging: ```sh