diff --git a/Nota.CodeAnalysis.Verification/README.md b/Nota.CodeAnalysis.Verification/README.md index 17d91ba..83e1275 100644 --- a/Nota.CodeAnalysis.Verification/README.md +++ b/Nota.CodeAnalysis.Verification/README.md @@ -59,6 +59,27 @@ looks redundant and is not: without it `IDE0005` silently stops reporting. | `UA1001` | no blank line between using blocks | `Unseparated.cs` | | `SA1516` | no blank line between members | `Unseparated.cs` | +Two more come from `build/Nota.CodeAnalysis.targets` rather than an analyser, and so are the only +rules here that cannot be confirmed by reading a severity out of the globalconfig - they have to +actually run: + +| Rule | What it catches | Severity | Sample | +|------------|-------------------------------------|----------|--------------------| +| `NOTA0001` | a file that is not valid UTF-8 | error | `Latin1Encoded.cs` | +| `NOTA0002` | a file with a UTF-8 byte order mark | warning | `BomMarked.cs` | + +The severities differ on purpose. `NOTA0001` is corruption and stops the build outright. A mark costs +nothing at runtime, so `NOTA0002` reports on a developer's build and becomes an error in a pipeline +built with `-warnaserror` - an MSBuild engine switch, so unlike `TreatWarningsAsErrors` it promotes a +task-logged warning. The greps here accept `warning` or `error` for that reason, and so keep working +whichever way a build is invoked. + +This pipeline does not pass that switch, and does not need to: `verify-encoding.sh` fails it on a +mark anywhere in the tree, including the files no compiler opens. + +`BomMarked.cs` is otherwise unremarkable on purpose: the fault is its first three bytes, and it must +not also be mis-encoded, or `NOTA0001` would cover for `NOTA0002` never firing. + `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 @@ -69,7 +90,7 @@ separation, which was always its own job. ## What `verify-encoding.sh` asserts -Every source file is valid UTF-8, or UTF-16 carrying a BOM. +Every source file is valid UTF-8 without a byte order mark, or UTF-16 carrying one. This is the guard that let `SA1412` be switched off. SA1412 demanded a byte order mark, which was never what anyone wanted, but it was the only thing standing between the build and a file saved as @@ -80,6 +101,12 @@ already been decoded. It is also a property of `.resx` and `.json` files, which BOM-marked UTF-16 is accepted rather than flagged. svcutil and EF migrations emit it, the compiler reads it correctly, and those files must keep their BOM - it is the only record of their encoding. +A UTF-8 mark fails instead - here it stops the pipeline, where `NOTA0002` only warns a consumer. This +repository ships that rule, so its own tree is the first place that has to be clean, and nothing in +it should ever need the grace period a warning buys someone else. It is also wider than +`NOTA0002` can be: the rule only ever sees `@(Compile)`, while this reads the `.md`, `.json` and +`.targets` files no compiler opens. + It cannot catch a wrong encoding that happens to produce valid UTF-8, the classic `“` mojibake, which is indistinguishable from someone writing those characters on purpose. @@ -105,8 +132,9 @@ 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 -warning nothing else would notice. +`build/Nota.CodeAnalysis.targets` was packed and imported, and a marked one for `NOTA0002`, which is +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 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 diff --git a/Nota.CodeAnalysis.Verification/Samples/BomMarked.cs b/Nota.CodeAnalysis.Verification/Samples/BomMarked.cs new file mode 100644 index 0000000..44e6d0b --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/BomMarked.cs @@ -0,0 +1,8 @@ +namespace Nota.Verification; + +/// Saved with a UTF-8 byte order mark on purpose. See verify.sh. +public static class BomMarked +{ + /// Nothing wrong with the text - it is the first three bytes of the file. + public const string Text = "marked"; +} diff --git a/Nota.CodeAnalysis.Verification/verify-encoding.sh b/Nota.CodeAnalysis.Verification/verify-encoding.sh index 3bfe7e1..c514398 100755 --- a/Nota.CodeAnalysis.Verification/verify-encoding.sh +++ b/Nota.CodeAnalysis.Verification/verify-encoding.sh @@ -1,6 +1,7 @@ #!/usr/bin/env sh # -# Fails if any source file is neither valid UTF-8 nor a BOM-marked UTF-16 file. +# Fails if any source file is neither valid UTF-8 without a byte order mark, nor a BOM-marked UTF-16 +# file. # # This is the guard that has to exist before SA1412 is switched off. SA1412 required a byte order # mark, which was never the point - but it was, by accident, the only thing standing between the @@ -12,6 +13,11 @@ # No analyser can cover this anyway - it is a property of bytes on disk, and it applies to .json and # .resx as much as to .cs. Hence a script. # +# A UTF-8 mark is a failure, not an acceptance. It records nothing - UTF-8 is what the compiler +# assumes when there is no mark - and it comes back on its own: an editor that opened a file with one +# writes one back on every later save. NOTA0002 says the same thing to consumers, but only about +# files the compiler sees; here it covers the .md, .json and .targets files as well. +# # UTF-16 is accepted when it carries a BOM. Generated output - svcutil service references, EF # migrations - is often UTF-16, the compiler reads it correctly from the BOM, and such a file must # keep that BOM: it is the only thing recording the encoding. @@ -36,6 +42,8 @@ set -eu root="${1:-.}" +# Each line is a tag and a path. Two different faults are being looked for in one pass over the +# bytes, and they want different advice: one is corruption, the other is noise that comes back. found="$(find "$root" \ \( -name '*.cs' -o -name '*.csproj' -o -name '*.json' -o -name '*.resx' -o -name '*.md' -o -name '*.props' -o -name '*.targets' \) \ -not -path '*/obj/*' -not -path '*/bin/*' -not -path '*/.git/*' \ @@ -45,17 +53,37 @@ found="$(find "$root" \ bom=$(head -c 3 "$f" | xxd -p) case "$bom" in fffe*|feff*) ;; - *) iconv -f UTF-8 -t UTF-8 "$f" >/dev/null 2>&1 || printf "%s\n" "$f" ;; + efbbbf) printf "bom %s\n" "$f" ;; + *) iconv -f UTF-8 -t UTF-8 "$f" >/dev/null 2>&1 || printf "invalid %s\n" "$f" ;; esac done ' sh {} +)" -if [ -n "$found" ]; then +invalid="$(printf '%s\n' "$found" | sed -n 's/^invalid //p')" +marked="$(printf '%s\n' "$found" | sed -n 's/^bom //p')" + +status=0 + +if [ -n "$invalid" ]; then printf 'Not valid UTF-8:\n' >&2 - printf '%s\n' "$found" | sed 's/^/ /' >&2 + printf '%s\n' "$invalid" | sed 's/^/ /' >&2 printf '\nThese compile without complaint and land in the assembly as U+FFFD.\n' >&2 printf 'Re-save them as UTF-8; check the tool or editor that last wrote them.\n' >&2 - exit 1 + status=1 fi -printf 'All source files are valid UTF-8 or BOM-marked UTF-16.\n' +# This repository ships NOTA0002, which fails a consumer build over exactly this. Its own tree is the +# first place that has to be clean - and unlike NOTA0002, which only ever sees @(Compile), this pass +# also covers the .md, .json and .targets files no compiler reads. +if [ -n "$marked" ]; then + [ "$status" -eq 0 ] || printf '\n' >&2 + printf 'Carrying a UTF-8 byte order mark:\n' >&2 + printf '%s\n' "$marked" | sed 's/^/ /' >&2 + printf '\nStrip them with tools/de-bom.sh, with the editor closed - one that opened a file with a\n' >&2 + printf 'mark writes the mark back on the next save, however the file on disk now looks.\n' >&2 + status=1 +fi + +[ "$status" -eq 0 ] || exit "$status" + +printf 'All source files are valid UTF-8 without a byte order mark, or BOM-marked UTF-16.\n' diff --git a/Nota.CodeAnalysis.Verification/verify-package.sh b/Nota.CodeAnalysis.Verification/verify-package.sh index ebcf4d5..158f004 100755 --- a/Nota.CodeAnalysis.Verification/verify-package.sh +++ b/Nota.CodeAnalysis.Verification/verify-package.sh @@ -130,15 +130,23 @@ printf 'namespace App;\n\n/// Saved in the wrong encoding on purpose.> "$app/MisEncoded.cs" printf '";\n}\n' >> "$app/MisEncoded.cs" +# A UTF-8 byte order mark, for NOTA0002. Separate from the file above on purpose: NOTA0001 stopping +# at the first byte of a mis-encoded file would hide it, and a mark on an otherwise clean file is the +# case that actually occurs. +printf '\357\273\277' > "$app/Marked.cs" +printf 'namespace App;\n\n/// Carries a UTF-8 byte order mark on purpose.\npublic static class Marked\n{\n /// Ordinary text.\n public const string T = "marked";\n}\n' >> "$app/Marked.cs" + output="$(cd "$app" && dotnet build --no-incremental -v:m 2>&1 || true)" # IDE0008 the globalconfig was packed, and the props file turned rule enforcement on # NOTA0001 build/Nota.CodeAnalysis.targets was packed and imported +# NOTA0002 the byte order mark check runs by default - it is a warning with an opt-out, so it is +# the rule here most easily lost without anything failing to say so # SA1208 StyleCop.Analyzers reached the consumer # 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" +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..c65e0a5 100755 --- a/Nota.CodeAnalysis.Verification/verify.sh +++ b/Nota.CodeAnalysis.Verification/verify.sh @@ -37,10 +37,13 @@ done # first party above the vendors and leaves --verify-no-changes failing permanently # SA1208 System usings not placed first # 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 UA1001 SA1208 SA1516 NOTA0001) +# NOTA0001 a source file that is not valid UTF-8, from build/Nota.CodeAnalysis.targets - one of +# two rules here logged by an MSBuild task rather than an analyser, and so the only ones +# that cannot be confirmed by reading a severity out of the globalconfig +# NOTA0002 a source file carrying a UTF-8 byte order mark - Samples/BomMarked.cs. Also from the +# targets, and the reason the samples are exempt from verify-encoding.sh. A warning +# where NOTA0001 is an error, which is why the grep below accepts either +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. diff --git a/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets index 6050bfd..5bdf78b 100644 --- a/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets +++ b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets @@ -2,7 +2,8 @@ true + false + = 3 && bytes[0] == 0xEF && bytes[1] == 0xBB && bytes[2] == 0xBF) + { + Log.LogWarning( + null, + "NOTA0002", + null, + path, + 1, + 1, + 0, + 0, + "File begins with a UTF-8 byte order mark. Re-save it as UTF-8 without one, or strip the tree with tools/de-bom.sh from Nota.CodeAnalysis - and close the editor while you do, since one that opened the file with a mark writes the mark back on the next save."); + } + try { new System.Text.UTF8Encoding(false, true).GetString(bytes); @@ -80,7 +120,7 @@ - + diff --git a/README.md b/README.md index 280ce01..7d792a7 100644 --- a/README.md +++ b/README.md @@ -64,6 +64,16 @@ 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` is a warning** on a source file carrying a UTF-8 byte order mark. Note that + `NOTA0001` says nothing about these: a BOM is legal UTF-8, so a marked file is a valid file, and + the two rules are looking for different things. The mark records nothing - UTF-8 is what the + compiler assumes when there is no mark at all - and it comes back on its own, which is the actual + reason this is a rule. An editor decides a file's encoding when it opens it: one that opened a file + with a mark writes a mark back on every later save, so a single stray file re-marks itself forever + and spreads to whatever else that editor touches. A warning rather than an error because a mark + 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`. - **`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`. @@ -87,21 +97,105 @@ decision you are stuck with: dotnet_diagnostic.IDE0008.severity = suggestion ``` -The encoding check is a build task rather than a diagnostic, so it has its own switch: +The encoding checks are a build task rather than diagnostics, so they have their own switches: ```xml -false +false +true ``` -## Upgrading from 2.1 +`NOTA0002` is only a warning, so a tree full of marks still builds; `NotaAllowUtf8Bom` is for +silencing it entirely while that tree waits its turn, and it leaves `NOTA0001` - the one that catches +actual corruption - in place. -`SA1412`, which required every source file to carry a byte order mark, is off. It never did anything -for the build - a file without a mark compiles fine, since the compiler assumes UTF-8 when none is -present - and what it was quietly protecting against is now `NOTA0001`'s job, which checks the bytes +### Making it an error, and the one that will not + +`NOTA0002` is logged by an MSBuild task, so it never passes through the compiler - and the property +everyone reaches for first is a compiler setting. All four measured: + +| Set this | `NOTA0002` becomes | +|-----------------------------------------|--------------------| +| `dotnet build -warnaserror` | an error | +| `NOTA0002` | an error | +| `true` | an error | +| `true` | **still a warning**| + +The first row is the one that matters, and it is the arrangement to want: `-warnaserror` is the +MSBuild engine's switch rather than the compiler's, so it takes task warnings with it, and a pipeline +already carrying that flag fails on a marked file without anyone configuring anything. That is the +split this severity is chosen for - a mark says so on a developer's build and stops it at the gate. +`-warnaserror:CS0168`, or any code list that omits `NOTA0002`, leaves it alone. + +The last row is the genuine surprise. `TreatWarningsAsErrors` is a compiler property, and this +warning never goes near the compiler, so a repository relying on that alone gets no gate - use the +switch or `WarningsAsErrors` if you want one. + +## Keeping marks out, rather than failing on them + +`NOTA0002` reports a file that already has a mark. What stops one being written in the first place is +a line in the consuming repository's own `.editorconfig`, which Rider and Visual Studio both honour: + +```ini +[*] +charset = utf-8 +``` + +That is `utf-8` meaning *without* a mark; `utf-8-bom` is the spelling that asks for one. Worth setting +even with `NOTA0002` on - the rule reports a file that has been re-marked, and this stops it being +written that way at all. + +It does not help a file that is already open. The encoding is decided when the editor opens a file +and kept for the buffer, so a tree is cleaned with the IDE closed and the setting keeps it clean +afterwards. + +That one line also puts `dotnet format` to work, which is the part worth knowing: + +| `.editorconfig` | `dotnet format whitespace` | `--verify-no-changes` on a marked file | +|-------------------------|-----------------------------------|----------------------------------------| +| `charset = utf-8` | strips the mark | exits 2 | +| `charset = utf-8-bom` | **adds** a mark to every file | exits 2 on an unmarked one | +| no `charset` key | leaves marks exactly as they are | exits 0 | + +All three measured, on `dotnet format whitespace` with nothing else wrong in the file - the mark is a +change in its own right, not something that has to ride along with a reformat. So with `utf-8` set, +a repository already running `dotnet format` in CI fails on a re-marked file without `NOTA0002` +needing to say anything, and fixes it by running the same command without `--verify-no-changes`. + +Two things follow from the second row. `utf-8-bom` is not merely the opposite setting - it will mark +files that never had a mark, and it fights `NOTA0002` on every build. And `charset` unset is why +marks survive a tree that formats itself religiously. + +`dotnet format` cannot fix `NOTA0002` as such. The rule is logged by an MSBuild task, so +`dotnet format analyzers` never sees it and no code fix exists for it; what strips the mark is the +`charset` key, working on its own. Nor does it leave UTF-16 alone the way `tools/de-bom.sh` does - +measured, it transcodes such a file to UTF-8, which preserves the text and still compiles, but +arrives as a whole-file diff, and a regenerated service reference or migration will be UTF-16 again +next time the generator runs. + +## Upgrading + +The two encoding changes came one version apart and land in the same place, so they are described +together. Where 2.1 demanded a mark and 2.2 stopped caring, 2.3 fails the build on one. + +**On 2.1**, `SA1412` required every source file to carry a byte order mark. It never did anything for +the build - a file without a mark compiles fine, since the compiler assumes UTF-8 when none is +present - and what it was quietly protecting against became `NOTA0001`'s job, which checks the bytes rather than the mark. -Nothing forces you to remove the marks you have. If you want to, `tools/de-bom.sh` does it a tree at -a time: +**On 2.2**, `SA1412` is off and nothing objects to a mark either way. That is the version to strip +them on, and the marks left behind are why 2.3 exists: with no rule pointing at them they come back, +one editor buffer at a time. + +**On 2.3**, `NOTA0002` warns on any compiled file that has one, and a pipeline building with +`-warnaserror` fails on it - which is the point of the severity rather than a side effect of it. A +developer's build says which files are marked and carries on; CI declines to merge them. + +So the marks have to go before the first build that matters, not eventually. Strip them, set +`charset = utf-8` in the same commit, and the tree stays clean on its own. `NotaAllowUtf8Bom` is +there for a repository that needs to upgrade today and sweep next week, and it leaves `NOTA0001` +running while it waits. + +`tools/de-bom.sh` strips a tree at a time: ```sh tools/de-bom.sh /path/to/repo # report, change nothing @@ -118,10 +212,10 @@ git diff --numstat | awk '$1 != 1 || $2 != 1' Silence means nothing but marks moved. -Two things in that order, and both bite if you get them wrong. +Three things in that order, and each bites if you get it wrong. -**Take 2.2 first.** On 2.1.x `SA1412` still demands a mark, so stripping them before upgrading breaks -the build on every file. +**Take 2.2 or later first.** On 2.1.x `SA1412` still demands a mark, so stripping them before +upgrading breaks the build on every file. **Then close the IDE while you strip them.** Visual Studio and Rider decide a file's encoding when they open it and keep that decision for the buffer. A file that was opened with a mark gets one @@ -129,6 +223,10 @@ written back on the next save, whatever the file on disk now looks like - so an quietly undoes the script, file by file, as you touch them. Closing it and reopening afterwards is enough; the encoding is re-detected from what is actually there. +**Then set `charset = utf-8` before reopening it.** Stripping the marks is a one-off; the setting is +what stops them being written again. Do it in the other order and the first save of the first file +you touch has already put one back. + ## Working on this repository The product here is configuration, and configuration fails silently: a rule that cannot report looks @@ -142,7 +240,7 @@ before changing rules. ```sh ./Nota.CodeAnalysis.Verification/verify.sh # the rules report -./Nota.CodeAnalysis.Verification/verify-encoding.sh # source is valid UTF-8 +./Nota.CodeAnalysis.Verification/verify-encoding.sh # source is UTF-8, and unmarked ``` Both run on pull requests as well as on `main`.