From 65304619b28b7f97bb0d4838246c43e4508d1040 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20O=2E=20S=C3=B8rensen?= Date: Tue, 18 Aug 2026 22:00:23 +0200 Subject: [PATCH] fix: Stop NOTA0001 and NOTA0002 reporting files nobody can fix. Microsoft.NET.Test.Sdk contributes a source file of its own to @(Compile) - Microsoft.NET.Test.Sdk.Program.cs, the generated test entry point - and it carries a UTF-8 byte order mark. It lives in the read-only NuGet cache. So NOTA0002 fired on every test project that installed this package, naming a file the consumer did not write, cannot re-save, and which is restored again the moment it is touched. That is worse than a missing check. The only escape was NotaValidateSourceEncoding=false, which switches off NOTA0001 as well - so a rule against corruption was talking people into turning off the one guard against it. NotaAllowUtf8Bom=true would have kept NOTA0001, but silences the mark check everywhere rather than in the one place it is wrong. The task now skips anything under the NuGet package root or the intermediate output path. Both are the same idea: sources compiled into the project that the project does not own. Generated files under obj carry no mark today, and are included because they are rewritten on the next build, so a report on one is noise that cannot be acted on either. The roots are passed in as items rather than matched by wildcard - globbing the package cache to exclude it would read every file in it - and NuGetPackageRoot is only passed when it has a value, since an empty root is a prefix of every path and would skip the entire check. Not caught before because the verification project is not a test project and so never installed the SDK that carries the file. It does now, in the throwaway consumer verify-package.sh builds, referenced for what it drags in rather than anything it does - and the script asserts NOTA000x names no file under the cache. Confirmed against the unfixed targets, where it fails with the file named. Co-Authored-By: Claude Opus 5 --- Nota.CodeAnalysis.Verification/README.md | 12 +++++ .../verify-package.sh | 17 +++++++ .../build/Nota.CodeAnalysis.targets | 44 ++++++++++++++++++- README.md | 7 +++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/Nota.CodeAnalysis.Verification/README.md b/Nota.CodeAnalysis.Verification/README.md index 83e1275..e82f821 100644 --- a/Nota.CodeAnalysis.Verification/README.md +++ b/Nota.CodeAnalysis.Verification/README.md @@ -136,6 +136,18 @@ rule per analyser - plus a deliberately mis-encoded file for `NOTA0001`, which p the only rule with an opt-out that defaults to enforcing and so the only one where a wrong default would ship as silence. It fails on `CS9057` too, since that is a warning nothing else would notice. +It also asserts the opposite of everything above: that `NOTA0001` and `NOTA0002` report the +consumer's own files and **nothing else**. The throwaway project references `Microsoft.NET.Test.Sdk` +purely for what that drags in - a source file of its own, from the read-only NuGet cache, carrying a +UTF-8 byte order mark. Every test project on earth compiles that file, and the encoding check +reported it on all of them until it learned to skip what the consumer did not write. + +A false positive there is worse than a missing check, which is why it is asserted rather than left to +notice: the file cannot be re-saved, it is restored the moment it is touched, and it is shared by +every project on the machine. The only way out was `NotaValidateSourceEncoding=false`, which throws +away `NOTA0001` as well - so a rule meant to catch corruption talked people into switching off the +one thing standing between them and it. + It packs under a throwaway version like `0.0.0-verify.20260802143000`. That is not cosmetic: 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 reaches the consumer. This diff --git a/Nota.CodeAnalysis.Verification/verify-package.sh b/Nota.CodeAnalysis.Verification/verify-package.sh index 158f004..f7642b2 100755 --- a/Nota.CodeAnalysis.Verification/verify-package.sh +++ b/Nota.CodeAnalysis.Verification/verify-package.sh @@ -80,6 +80,11 @@ cat > "$app/App.csproj" < + + EOF @@ -155,6 +160,18 @@ for rule in $expected; do fi done +# NOTA0001 and NOTA0002 must report the consumer's own files and nothing else. A file inside the +# NuGet cache is not the consumer's to fix - it is shared, read-only, and restored again the moment +# it is touched - so reporting one leaves switching the whole check off as the only way to a clean +# build. Checked by name rather than by count: the rules are expected to fire above, so a count says +# nothing about which file they fired on. +foreign="$(printf '%s' "$output" | grep -E '(warning|error) NOTA000[0-9]' | grep -iE '[/\\]\.nuget[/\\]|[/\\]packages[/\\]' || true)" +if [ -n "$foreign" ]; then + printf '\nThe encoding check reported a file the consumer cannot fix:\n%s\n' "$foreign" >&2 + printf 'It lives in the NuGet cache. Skip it rather than asking anyone to re-save it.\n\n' >&2 + exit 1 +fi + # CS9057 is never acceptable: it means an analyser referenced a newer compiler than the one running, # and was skipped. It is a warning, so nothing else would fail. if printf '%s' "$output" | grep -q "CS9057"; then diff --git a/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets index 5bdf78b..faec1cf 100644 --- a/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets +++ b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets @@ -48,9 +48,22 @@ + (); + foreach (var root in SkipRoots ?? new Microsoft.Build.Framework.ITaskItem[0]) + { + var full = root.GetMetadata("FullPath"); + if (!string.IsNullOrEmpty(full)) + { + skip.Add(full.TrimEnd(System.IO.Path.DirectorySeparatorChar) + System.IO.Path.DirectorySeparatorChar); + } + } + foreach (var item in Files) { var path = item.GetMetadata("FullPath"); @@ -60,6 +73,27 @@ continue; } + // A package's own sources are compiled into this project but belong to whoever shipped + // them: Microsoft.NET.Test.Sdk contributes a marked file from the NuGet cache, and no + // consumer can re-save a file in a shared read-only cache. Generated files under obj go + // the same way - they are rewritten on the next build, so a report is noise that cannot + // be acted on. Reporting either would leave switching the whole check off as the only + // way to get a clean build, which is worse than not checking those files. + var skipped = false; + foreach (var root in skip) + { + if (path.StartsWith(root, System.StringComparison.OrdinalIgnoreCase)) + { + skipped = true; + break; + } + } + + if (skipped) + { + continue; + } + var bytes = System.IO.File.ReadAllBytes(path); // UTF-16 carrying a BOM is fine: the compiler reads it correctly, and svcutil and EF @@ -120,7 +154,15 @@ - + + + + + + + diff --git a/README.md b/README.md index 191661f..346541b 100644 --- a/README.md +++ b/README.md @@ -74,6 +74,13 @@ Most of this is unsurprising. These are the ones that catch people out: costs nothing at runtime, so a tree that has them should say so on every build rather than be unbuildable until someone sweeps it. Which of the several ways to promote a warning actually promotes this one is not obvious - see below. UTF-16 is exempt, as under `NOTA0001`. +- **Both only look at files you wrote.** Sources under the NuGet cache and generated sources under + `obj` are compiled into your project but are not yours to re-save, so neither rule reports them. + `Microsoft.NET.Test.Sdk` is the one that made this necessary: it contributes a marked file of its + own to every test project, from a read-only shared cache that restores it the moment it is touched. + Reporting that left `NotaValidateSourceEncoding=false` as the only route to a clean build, which + throws away `NOTA0001` too - a rule against corruption talking people into switching off the only + guard against it. - **`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`.