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`.