Merge pull request #13 from henrikottesorensen/claude/utf8-bom-enforcement-c44583

Warn on UTF-8 byte order marks with NOTA0002

Author: henrikottesorensen

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

Nota.CodeAnalysis.Verification/Samples/BomMarked.cs

@@ -0,0 +1,8 @@
+namespace Nota.Verification;
+
+/// <summary>Saved with a UTF-8 byte order mark on purpose. See verify.sh.</summary>
+public static class BomMarked
+{
+    /// <summary>Nothing wrong with the text - it is the first three bytes of the file.</summary>
+    public const string Text = "marked";
+}

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'

Nota.CodeAnalysis.Verification/verify-package.sh

@@ -130,15 +130,23 @@ printf 'namespace App;\n\n/// <summary>Saved in the wrong encoding on purpose.</
 printf '\346\370\345' >> "$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/// <summary>Carries a UTF-8 byte order mark on purpose.</summary>\npublic static class Marked\n{\n    /// <summary>Ordinary text.</summary>\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

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.

Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets

@@ -2,7 +2,8 @@
 <Project ToolsVersion="17.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
 
   <!--
-    Fails the build on a source file that is not valid UTF-8.
+    Fails the build on a source file that is not valid UTF-8 (NOTA0001), and warns on one carrying a
+    UTF-8 byte order mark (NOTA0002).
 
     Nothing else can do this. A file saved as Windows-1252 compiles with no warning and no error, and
     the compiler writes U+FFFD replacement characters into the assembly - measured, not assumed. An
@@ -15,18 +16,38 @@
     standing between a build and a mis-encoded file. Removing it without this would have left every
     consumer worse off than before.
 
-    Set NotaValidateSourceEncoding to false to switch it off.
+    NOTA0002 is the other half of that: with SA1412 gone nothing objected to a mark either way, and
+    marks come back on their own. An editor that opened a file with one writes one back on every
+    subsequent save, so a single stray file re-BOMs itself indefinitely and spreads to whatever else
+    that editor touches. The bytes have to be looked at to see it, which is this task's job already.
+
+    A warning where NOTA0001 is an error, and the difference is deliberate. NOTA0001 is corruption:
+    the file is already wrong and the build should stop. A mark costs nothing at runtime - it
+    compiles, it runs, it is only noise - so it says so on a developer's build and stops the one that
+    counts, because dotnet build -warnaserror is an MSBuild engine switch and takes task warnings
+    with it. A pipeline already carrying that flag fails on a marked file having configured nothing.
+    WarningsAsErrors=NOTA0002 (the SDK feeds it into MSBuildWarningsAsErrors) and
+    MSBuildTreatWarningsAsErrors do the same.
+
+    TreatWarningsAsErrors does not, however emphatically it is set - it is a compiler property, and
+    this warning is logged by a task and never passes through the compiler. A repository relying on
+    that one alone has no gate. All four measured.
+
+    Set NotaValidateSourceEncoding to false to switch both off, or NotaAllowUtf8Bom to true to keep
+    NOTA0001 while permitting marks.
   -->
 
   <PropertyGroup>
     <NotaValidateSourceEncoding Condition="'$(NotaValidateSourceEncoding)' == ''">true</NotaValidateSourceEncoding>
+    <NotaAllowUtf8Bom Condition="'$(NotaAllowUtf8Bom)' == ''">false</NotaAllowUtf8Bom>
   </PropertyGroup>
 
   <UsingTask TaskName="NotaValidateUtf8" TaskFactory="RoslynCodeTaskFactory"
              AssemblyFile="$(MSBuildToolsPath)\Microsoft.Build.Tasks.Core.dll"
              Condition="'$(NotaValidateSourceEncoding)' == 'true'">
     <ParameterGroup>
       <Files ParameterType="Microsoft.Build.Framework.ITaskItem[]" Required="true" />
+      <AllowByteOrderMark ParameterType="System.Boolean" Required="false" />
     </ParameterGroup>
     <Task>
       <Code Type="Fragment" Language="cs"><![CDATA[
@@ -50,6 +71,25 @@
                 continue;
             }
 
+            // EF BB BF, and only EF BB BF. A UTF-8 mark records nothing - UTF-8 is what the compiler
+            // assumes when there is no mark at all - so it is pure noise that editors keep putting
+            // back. Checked before the decode, not instead of it: a file can have a mark and still be
+            // mis-encoded further in, and both are worth saying.
+            if (!AllowByteOrderMark
+                && bytes.Length >= 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 @@
   <Target Name="NotaValidateSourceEncoding"
           BeforeTargets="CoreCompile"
           Condition="'$(NotaValidateSourceEncoding)' == 'true' AND '@(Compile)' != ''">
-    <NotaValidateUtf8 Files="@(Compile)" />
+    <NotaValidateUtf8 Files="@(Compile)" AllowByteOrderMark="$(NotaAllowUtf8Bom)" />
   </Target>
 
 </Project>