Fix three rules that could not report, and add the checks that catch it - #8
Merged
Conversation
… fire. Three things, all found by running this package's own globalconfig against probe files rather than by reading it. CS8019 never worked. It has asked for unused usings to be reported for as long as it has been here, and a file with an unused using builds clean: CS8019 is emitted hidden by the compiler and a severity in config does not raise it. IDE0005 is the rule that reports; GenerateDocumentationFile, which it needs, was already set in build/Nota.CodeAnalysis.props. One consuming repository had forty-five unused directives behind this. dotnet_separate_import_directive_groups promised more than it delivered. SA1516 uses it for the blank line after the System group and that works; the per-vendor grouping the name suggests is IDE0055's job, and IDE0055 is off, so nothing checked it. Rather than enable IDE0055 - which would also start enforcing indentation and spacing, and reflow every consumer - UsingLayoutAnalyser now owns the layout: System, then third party, then the consuming solution's own namespaces, one run per vendor. SA1210 has to go off for it, because it sorts the whole list alphabetically and wants a solution's own root above a vendor whenever it sorts earlier. SA1208, SA1209, SA1211, SA1216 and SA1217 do not conflict and stay on. first_party_prefixes cannot live here - it differs per solution - so consumers set it in their own .editorconfig, and the scheme degrades sensibly when they do not. Nothing verified any of this, and pull requests did not build. That is how a dead CS8019 line survives: the product is configuration, and configuration fails silently. Nota.CodeAnalysis.Verification is a consumer that breaks five rules on purpose, and verify.sh asserts each one reported. Its samples compile only under -p:VerifyRules=true, because some of these rules are error severity and an ordinary solution build must not fail. Confirmed to work in both directions: switching IDE0005 back off makes verify.sh fail naming it. Pull requests now build and verify; pack and push stay conditioned on main so a pull request cannot publish. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The BOM requirement was added to get code building in Docker containers. It never did that. A file without a BOM compiles fine - Roslyn defaults to UTF-8 when no BOM is present - and the whole of the largest solution builds and passes with the marks stripped, on macOS and in the container. Whatever the original problem was, this was not its fix. SA1412 is what required it, and editing charset does not change that: SA1412 does not read charset at all, and demands a BOM whether it says utf-8 or utf-8-bom. Off is the only setting that moves it. charset is set to utf-8 to say what the convention is, and to be correct if this content is ever used as an .editorconfig. But the rule was mislabelled rather than useless, and dropping it alone would have left things worse. It was the only thing standing between the build and a file saved as Windows-1252 - measured: such a file compiles with no warning, and the compiler writes U+FFFD into the assembly. Silent, and it reaches the binary. No analyser can report it, because by the time an analyser runs the text has already been decoded; and it applies to .resx and .json as much as to .cs. So verify-encoding.sh, running in the same pipeline stage. It accepts BOM-marked UTF-16, which is what svcutil and EF migrations emit and what the compiler reads correctly - and those files must keep their BOM, since it is the only record of their encoding. It cannot catch a wrong encoding that happens to produce valid UTF-8, the "“" case, which is indistinguishable from someone writing those characters deliberately. Verified against a tree with spaces in its paths, a UTF-16 .resx, and a Latin-1 file - the clean tree passes and the bad file is caught. That combination is deliberate: an earlier draft passed paths through an unquoted variable, and every file under "Service References" was reported as corrupt because the name has a space in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It reads like a test project and is not one: nothing here ships, and it exists because a rule that cannot report looks exactly like a rule being obeyed. The CS8019 story is written down, because it is the argument for the whole project and it will otherwise be forgotten the first time someone wonders why a project full of deliberately broken files is in the solution. Also records the two things easiest to get wrong later: watch a check fail before trusting it, and remember this exercises the globalconfig's content rather than the package, so a wrong PackagePath would still ship silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing SA1412 left consumers with nothing. verify-encoding.sh guards this repository, but a package cannot ask every consumer to wire up a script - so the net effect of the previous commit, for anyone installing the package, was that the accidental guard went away and no replacement arrived. That is exactly the combination that leaves people worse off, and it shipped. build/Nota.CodeAnalysis.targets now validates every file being compiled, before CoreCompile, and fails with NOTA0001 naming the file. It has to happen there: a file saved as Windows-1252 compiles with no warning and reaches the assembly as U+FFFD, and an analyser cannot see it because by the time an analyser runs the text has been decoded and the bytes are gone. UTF-16 with a BOM passes - svcutil and EF migrations emit it, the compiler reads it correctly, and such a file must keep its BOM. Only @(Compile) is read, so .resx and .json are not covered; reading the whole tree on every build was not worth what it costs. NotaValidateSourceEncoding=false switches it off. Verified as a consumer sees it, not just in this solution: packed, installed from a local feed into a fresh project, clean source builds and one Windows-1252 file fails with NOTA0001. That path matters because it is the one the verification project does not cover - it imports the globalconfig directly, so a broken PackagePath would still ship silently. NOTA0001 is added to verify.sh, which needs a deliberately mis-encoded sample, so Samples/ is now excluded from verify-encoding.sh: those files are wrong on purpose and that check is for files that are wrong by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The readme was a single sentence, and it is not only the repository's front page: the csproj packs it to content/README.md and declares it as PackageReadmeFile, so it is what anyone browsing the package sees. It now says how to install it, what switches on, the one key a consumer has to set themselves, and which rules surprise people. Four things are worth a stranger's attention before their first build, and none were written down anywhere: IDE0008 and VSTHRD100 are errors rather than warnings, NOTA0001 is a build task and not a diagnostic so it has its own opt-out, and usinglayout.first_party_prefixes cannot be shipped by the package because it differs per solution - unset, the using layout quietly stops distinguishing your code from a vendor's. GenerateDocumentationFile gets a line of its own. It looks like an unrelated documentation setting and is the switch that makes IDE0005 report at all; someone will eventually decide it is redundant. Nota.CodeAnalysis/README.md is deleted. It was a near-duplicate of the first line of this one, nothing referenced it, and it was not the file that reaches the package - two readmes saying almost the same thing is how one of them goes stale without anyone noticing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The using layout needs to know which namespaces are the consumer's own, and the previous commit left that as the one thing every repository had to remember. A setting every repository must remember is a setting most repositories will not have - and unset, the layout quietly stops distinguishing Nota code from a vendor's, which is the whole point of it. Nota is right for nearly everything here, so it is the default. A consumer whose code is called something else overrides it in their own .editorconfig, and getting it wrong is not fatal: their namespaces are sorted as one more vendor rather than last. The precedence this relies on was verified rather than assumed - the built package installed into a project whose .editorconfig disagreed with it, and the .editorconfig won. That holds for every severity in the globalconfig too, so the readme now says so as a fact rather than an expectation. The comment above the setting had said this key could not live here. It is now the line directly beneath it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pipeline installed SDK 8 and would have packed a release whose using rules did nothing. An analyser cannot reference a newer Roslyn than the compiler running it: UsingLayoutAnalyser is built against 4.14, SDK 8.0.423 runs 4.11, and the compiler answers with CS9057 - a warning, not an error - then carries on without it. Verified on that exact SDK, where StyleCop reported normally and UA1000 and UA1001 were simply absent. Which is this package's own failure mode one level out: a rule that is configured, believed, and incapable of reporting. Nothing about the build looks wrong. The verification project moves to net10.0 for the same reason. On net8.0 it would still pass while proving less, because the analyser it is meant to be verifying could not be loaded at all. Worth noting the guard worked: pinned to SDK 8, verify.sh fails with "UA1000 did not report - it is configured but not reaching consumers". Had this been missed, the pipeline would have gone red rather than shipping. Consumers building on SDK 8 still get CS9057 and no using rules. That is acceptable while everything here is on .NET 10; the durable fix is to build UsingLayoutAnalyser against an older Roslyn, which costs nothing it currently uses. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The repository is on GitHub and the packages go to GitHub Packages, so the build was the only part living elsewhere - and it paid for that with a variable group holding a personal access token and a secure NuGet.config downloaded on every run. GITHUB_TOKEN replaces the first. The second turns out to have been unnecessary all along: every dependency here is public, and the solution restores from nuget.org alone, which was checked rather than assumed. It also settles a question the Azure version left open. Those verification steps were Bash@3 tasks against self-hosted agents whose operating system I never established; on ubuntu-latest the question does not arise. Same shape as before: build, verify the rules report, verify the encoding, pack, and publish only from main. Pull requests do everything except publish, and the package is uploaded as an artifact either way so a packaging mistake is visible without merging. SDK 10 rather than 8, for the reason recorded in the previous commit: on 8 the using rules answer CS9057 and then silently do not run. Actions are pinned to current majors. upload-artifact@v4 was written first and replaced with v7 - v4 runs on Node 20, which is the deprecation this pipeline would have started warning about immediately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… again. 0.2.1 is built against Roslyn 4.8 rather than 4.14, so the using rules load on every SDK from .NET 8 onwards. On 0.2.0 an SDK 8 consumer got CS9057 - a warning - and no UA1000 or UA1001 at all. Microsoft.VisualStudio.Threading.Analyzers loses the PrivateAssets that Rider added when its version was bumped. That is the right default for an ordinary project and wrong for this one: PrivateAssets stops a reference reaching consumers, and for an analyser that means the rules never arrive. The globalconfig went on declaring VSTHRD100 an error while no consumer could receive the analyser that reports it - configured, believed, and incapable of firing, which is the failure this package has now produced three times. Verified through the built package rather than the solution, because the solution cannot show it: the verification project references the analysers directly, so VSTHRD100 fired there throughout while consumers got nothing. Packed, installed from a local feed, and an async void method now fails the build with VSTHRD100. The nuspec declares all four analysers again. None of these references may carry PrivateAssets, and the item group now says so, because the next version bump will offer to add it back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onfigured. verify.sh runs inside the solution, where the globalconfig is imported directly and the analysers are referenced by the project itself. A rule can pass there while no consumer receives it, and that has now happened twice: UsingLayoutAnalyser built against a newer Roslyn than the SDK running it, skipped with CS9057; and the threading analyser acquiring PrivateAssets in a version bump, so VSTHRD100 went on firing here while consumers got nothing at all. Neither was visible from inside. verify-package.sh packs, installs into a throwaway project from a local feed, and compiles a file breaking one rule per analyser - IDE0008 for the globalconfig and props, NOTA0001 for the targets, SA1208, VSTHRD100, UA1000 and Serilog003 for the four analysers reaching a consumer at all. CS9057 fails it outright, being a warning nothing else would notice. It packs under a throwaway version rather than the real one. That is the whole check, not a detail: NuGet extracts a package once per version into the global cache, so re-packing 2.2.0 and installing 2.2.0 gets whatever was extracted first and the change under test never arrives. Written the obvious way, this script passed cleanly against the exact regression it exists to catch. It was only found by putting PrivateAssets back and watching what happened - which is what the project's own readme tells everyone to do, and which I had not done. Both states are now confirmed: with the regression it fails naming VSTHRD100, without it, all rules survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merging and releasing were the same act, which made the version number a thing someone had to remember to change before merging - and forgetting it published nothing while looking like it had, because --skip-duplicate silently declines a version already in the registry. A v* tag now publishes, and the tag is the version: nothing in the repository can disagree with what shipped. Pushes to main and pull requests still build and run all three verifications; they simply stop before publishing. <Version> in the csproj stays as a local default for anyone packing by hand, and is now labelled as one. On a tag it is overridden for both build and pack, so the assembly and the package agree. Both paths were run locally: with no tag the pack is 2.2.0 from the csproj, and with VERSION_ARG set as the workflow sets it, 2.3.0. The unset case matters - an undefined variable in a run step expands to nothing rather than erroring, which is what lets one command serve both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were described as things that had cost this repository, which reads as shipped bugs found in the package. They were not. UsingLayoutAnalyser is added by this branch and its CS9057 problem never existed anywhere else; the threading analyser's PrivateAssets was added by an editor during a version bump on this same branch. Neither reached a consumer. CS8019 is the one that was genuinely there for years, and the distinction matters to anyone reading this later to judge how much the checks are worth: two of the three failures were created and caught inside an afternoon, which is a better argument for them than a history of shipped mistakes would have been, and it happens to be the true one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nanchen2483
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
dotnet_diagnostic.CS8019.severity = warninghas been in the globalconfig asking for unused usings to be reported. It has never reported anything: CS8019 is emitted hidden by the compiler, and a severity set in config does not raise it. One consuming solution had 45 unused directives behind it, across 36 files, and every build was green.Nothing in this repository could have told us. What we ship is configuration, and configuration fails quietly: a rule that cannot report looks exactly like a rule being obeyed, because the build is silent either way.
That is the argument for the verification added here — and it earned its place immediately, by catching two more failures of the same shape in changes made on this branch, before any of them merged:
UsingLayoutAnalyserwas built against a newer Roslyn than the SDK running itCS9057, a warning; the using rules simply did not runverify.shgo redPrivateAssetsto the threading analyserVSTHRD100could never fire for anyoneverify-package.shNeither reached the package. Both are the same failure as
CS8019: configured, believed, incapable of reporting. One had been in the repository for years; the other two took an afternoon to create.So this PR fixes the dead rule, and adds the checks that catch the shape.
Rules
IDE0005replaces the deadCS8019.GenerateDocumentationFile, which it needs, was already set.CS8019stays with a comment saying it is inert, so nobody sets it again expecting an effect.SA1412is off, andNOTA0001replaces it. The byte-order-mark requirement was added believing it was needed for container builds. It is not — a file without a BOM compiles fine. But it was, by accident, the only thing catching a file saved as Windows-1252, which compiles with no warning at all and reaches the assembly as U+FFFD.NOTA0001is an MSBuild task inbuild/Nota.CodeAnalysis.targets, because an analyser cannot see this: by the time one runs, the text has been decoded and the bytes are gone.NotaValidateSourceEncoding=falseopts out.UsingLayoutAnalyserowns the using layout — System, then third party, then the consuming solution's own namespaces, one run per vendor.dotnet_separate_import_directive_groupspromised this and delivered half: SA1516 uses it for the System boundary only, and the per-vendor grouping is IDE0055's job, which is off.SA1210has to go off for the new layout; SA1208, SA1209, SA1211, SA1216 and SA1217 are unaffected.usinglayout.first_party_prefixesdefaults toNota. A consumer whose code is called something else overrides it in their own.editorconfig— verified that this takes precedence over the package's global config, rather than assumed.Verification
Nota.CodeAnalysis.Verificationis a consumer built against files that break the rules on purpose, plus three scripts:verify.sh— the six rules report. Confirmed to fail correctly by switchingIDE0005off.verify-encoding.sh— every source file is valid UTF-8, or BOM-marked UTF-16 (svcutil and EF migrations emit that, and those files must keep their BOM).verify-package.sh— packs, installs into a throwaway project, and checks each rule survived. This is the only one that catches a package which installs cleanly and does nothing.That last one was itself broken when first written: it passed against the exact regression it exists to catch, because NuGet extracts a package once per version and re-packing
2.2.0served the cached copy. It packs under a throwaway version now, and both states are confirmed — it fails namingVSTHRD100with the regression present, and passes without it.Build
The Azure pipeline is replaced by a GitHub Action.
GITHUB_TOKENreplaces the variable-group PAT, and the secureNuGet.configturns out never to have been needed — every dependency is public, checked rather than assumed.Two behaviour changes worth noting:
CS9057and silently do not run.v*supplies the version. Previously, forgetting to bump<Version>published nothing while looking successful, because--skip-duplicatedeclines a version already present.Pull requests build and run all three verifications without publishing.
For the reviewer
Everything asserted here was measured against a built package rather than reasoned about — including the claims that turned out wrong on the first attempt. The commit messages carry the evidence and the failures, and are worth reading individually.
Consumers of
2.1.4will see new diagnostics on upgrade: unused usings, the using layout, and any mis-encoded file.dotnet format analyzers --diagnostics UA1000 UA1001 --severity warnhandles the layout in one pass.Co-Authored-By: Claude Opus 5 noreply@anthropic.com