diff --git a/.github/workflows/build-and-publish.yml b/.github/workflows/build-and-publish.yml new file mode 100644 index 0000000..087bc42 --- /dev/null +++ b/.github/workflows/build-and-publish.yml @@ -0,0 +1,89 @@ +# Builds, verifies, and publishes to GitHub Packages. +# +# Verification is the point rather than a formality. What this repository ships is configuration, and +# configuration fails silently: a rule that cannot report looks exactly like a rule being obeyed, +# because the build is quiet either way. Both scripts below exist to make that noisy, and they run on +# pull requests as well as on main, because a rule that stopped reporting should not be discovered +# after it has merged. +name: build and publish + +on: + push: + branches: [main] + tags: ['v*'] + pull_request: + branches: [main] + workflow_dispatch: + +# Only a version tag publishes. Pushes to main and pull requests build and verify and stop there, +# so merging is not the same act as releasing. +permissions: + contents: read + packages: write + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + build: + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-dotnet@v6 + with: + # 10, not 8. An analyser cannot reference a newer Roslyn than the compiler running it, and + # UsingLayoutAnalyser is the rule set most likely to be affected: on SDK 8 it answers + # CS9057 - a warning - and its rules are then silently absent. This is also what consumers + # build with, and testing on what people use beats testing on the oldest thing that works. + dotnet-version: '10.x' + + # On a tag the tag is the version, so nothing in the repository can disagree with what shipped. + # Off a tag this is unset and expands to nothing, leaving the csproj's own - which is + # then only ever a local default, never the thing that gets published. + - name: Take the version from the tag + if: github.ref_type == 'tag' + run: echo "VERSION_ARG=-p:Version=${GITHUB_REF_NAME#v}" >> "$GITHUB_ENV" + + # No NuGet.config and no secure file: every dependency is public on nuget.org. The Azure + # pipeline this replaced downloaded a secure NuGet.config it did not need. + - name: Build + run: dotnet build Nota.CodeAnalysis.sln --configuration Release $VERSION_ARG + + - name: Verify the rules report + run: ./Nota.CodeAnalysis.Verification/verify.sh + + - name: Verify source encoding + run: ./Nota.CodeAnalysis.Verification/verify-encoding.sh + + # The two checks above run inside the solution and cannot see whether the package delivers + # anything. This one packs, installs into a throwaway project, and compiles a file that breaks + # a rule from each analyser. It is the only check that fails when a package installs cleanly + # and does nothing, which has happened twice. + - name: Verify the package delivers the rules + run: ./Nota.CodeAnalysis.Verification/verify-package.sh + + - name: Pack + run: dotnet pack Nota.CodeAnalysis.sln --configuration Release --no-build --output artifacts $VERSION_ARG + + # Kept even on a pull request, so a packaging mistake is visible without merging. + - name: Upload the package + uses: actions/upload-artifact@v7 + with: + name: nupkg + path: artifacts/*.nupkg + + # GITHUB_TOKEN rather than a personal access token: publishing to this repository's own package + # registry needs nothing else, which is the whole reason to be here rather than in Azure. + # --skip-duplicate so re-running a failed job is not itself an error; the registry keeps a + # version forever, and pushing the same one twice is worth ignoring rather than failing on. + - name: Publish to GitHub Packages + if: github.ref_type == 'tag' + run: > + dotnet nuget push artifacts/*.nupkg + --source https://nuget.pkg.github.com/Notalib/index.json + --api-key ${{ secrets.GITHUB_TOKEN }} + --skip-duplicate diff --git a/.gitignore b/.gitignore index 63bc327..4a453c3 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,12 @@ .vs obj bin + +# macOS Finder metadata. +.DS_Store + +# Rider / IntelliJ project metadata. +.idea/ + +# Local pack output. +artifacts/ diff --git a/DevOps/Azure-Pipelines/nuget.yml b/DevOps/Azure-Pipelines/nuget.yml deleted file mode 100644 index a10d020..0000000 --- a/DevOps/Azure-Pipelines/nuget.yml +++ /dev/null @@ -1,59 +0,0 @@ -trigger: - - main -pr: none - -variables: - - group: github-packages - -pool: - name: "Nota OnPremise Agents" - -stages: - - stage: Build - jobs: - - job: Build - displayName: Build - variables: - - name: solution - value: "Nota.CodeAnalysis.sln" - - name: buildPlatform - value: "Any CPU" - - name: buildConfiguration - value: "Release" - steps: - - task: UseDotNet@2 - displayName: 'Install .NET Core SDK' - inputs: - version: 8.x - - task: DownloadSecureFile@1 - name: "nuget" - displayName: "Download NuGet.config from secure storage" - inputs: - secureFile: "NuGet.config" - - task: DotNetCoreCLI@2 - displayName: Nuget restore solution - inputs: - command: "restore" - restoreSolution: "$(solution)" - feedsToUse: "config" - nugetConfigPath: $(nuget.secureFilePath) - - task: DotNetCoreCLI@2 - displayName: 'Build' - inputs: - command: 'build' - arguments: '--configuration $(buildConfiguration)' - projects: "$(solution)" - - task: DotNetCoreCLI@2 - displayName: "Generate Nuget package" - inputs: - command: 'pack' - arguments: '--configuration $(buildConfiguration)' - packagesToPack: "$(solution)" - nobuild: true - versioningScheme: 'off' - - task: DotNetCoreCLI@2 - displayName: Publish nuget packages - inputs: - command: 'custom' - custom: 'nuget' - arguments: 'push "$(Build.ArtifactStagingDirectory)/**/*.nupkg" --source https://nuget.pkg.github.com/Notalib/index.json --skip-duplicate --api-key "$(github_token)"' diff --git a/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj b/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj new file mode 100644 index 0000000..e316aaa --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Nota.CodeAnalysis.Verification.csproj @@ -0,0 +1,57 @@ + + + + + + net10.0 + false + + + false + + + true + true + true + disable + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Nota.CodeAnalysis.Verification/README.md b/Nota.CodeAnalysis.Verification/README.md new file mode 100644 index 0000000..9ecbb11 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/README.md @@ -0,0 +1,111 @@ +# Nota.CodeAnalysis.Verification + +Checks that the rules this repository ships actually report. Nothing here is shipped or consumed: +`IsPackable` is false, it is in no package, and nothing depends on it. + +## Why it exists + +The product of this repository is configuration, and configuration fails silently. A rule that is +misspelled, superseded, or simply incapable of reporting looks exactly like a rule that is being +obeyed - the build is quiet either way. + +That is not hypothetical. `dotnet_diagnostic.CS8019.severity = warning` asked for unused usings to be +reported, and reported nothing, for as long as anyone can tell: CS8019 is emitted hidden by the +compiler and a severity in config does not raise it. One consuming solution had forty-five unused +directives behind it. Every build was green. + +So this project is a consumer, built on purpose against files that break the rules, and a script that +fails if any of them stayed quiet. + +## Running it + +```sh +./verify.sh # the rules report +./verify-encoding.sh # source files are valid UTF-8 +./verify-package.sh # the rules survive being packaged +``` + +All three run in the pipeline, on pull requests as well as on `main`. All exit non-zero on failure +and name what went wrong. + +## How it is put together + +`Samples/` is excluded from compilation unless `VerifyRules` is set: + +```xml + + +``` + +Some of the rules are error severity - `IDE0008` is - so compiling the samples would fail any +ordinary build of the solution, including the one the pipeline runs before it gets here. `verify.sh` +passes `-p:VerifyRules=true`; every other build gets an empty assembly. + +The project also declares the same properties `build/Nota.CodeAnalysis.props` gives a consumer, and +`verify.sh` fails if the props file stops setting one of them. Otherwise this project could drift +into testing a configuration nobody actually receives - and `GenerateDocumentationFile` in particular +looks redundant and is not: without it `IDE0005` silently stops reporting. + +## What `verify.sh` asserts + +`Samples/Broken.cs` breaks each of these deliberately. + +| Rule | What it catches | +|-----------|----------------------------------------------| +| `IDE0005` | an unused using - what CS8019 never did | +| `IDE0008` | `var` instead of an explicit type | +| `UA1000` | using directives out of order | +| `SA1208` | System usings not placed first | +| `SA1516` | no blank line after the System group | + +## What `verify-encoding.sh` asserts + +Every source file is valid UTF-8, or UTF-16 carrying a BOM. + +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 +Windows-1252 - and such a file compiles with no warning at all, putting U+FFFD replacement characters +straight into the assembly. No analyser can report that: by the time an analyser runs, the text has +already been decoded. It is also a property of `.resx` and `.json` files, which no analyser reads. + +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. + +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. + +## Adding a rule + +Break it in `Samples/Broken.cs`, then add its id to `expected` in `verify.sh`. + +**Watch it fail before you trust it.** Set the rule's severity to `none` in the globalconfig and run +`verify.sh`; it should fail naming that rule. A check nobody has seen fail is not a check - which is +the whole reason this project exists. + +## What `verify-package.sh` asserts + +That the rules survive packaging, which the other two cannot see. They run inside the solution, where +the globalconfig is imported directly and the analysers are referenced by the project itself - so a +rule can be reported here while no consumer receives it. That is not theoretical - it happened twice +while this project was being written, both times caught before merging: + +- **UsingLayoutAnalyser was built against a newer Roslyn than the SDK running it.** The compiler + answered `CS9057`, a warning, and skipped the analyser. Green build, no using rules. +- **The threading analyser acquired `PrivateAssets` during a version bump**, which stops a reference + reaching consumers. `VSTHRD100` went on firing inside this solution while consumers got nothing. + +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. + +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 +script passed cleanly against a regression it was written to catch until the version was made unique. + +## What none of them cover + +The scripts are POSIX `sh` and run on `ubuntu-latest`. On a Windows agent they would need porting. + +`verify-package.sh` needs network on a cold cache, for the throwaway project's own dependencies. diff --git a/Nota.CodeAnalysis.Verification/Samples/Broken.cs b/Nota.CodeAnalysis.Verification/Samples/Broken.cs new file mode 100644 index 0000000..b0b996f --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/Broken.cs @@ -0,0 +1,17 @@ +using Nota.Vendor; +using System; +using System.Xml; + +namespace Nota.Verification; + +/// Deliberately wrong. Every diagnostic here is asserted by verify.sh. +public static class Broken +{ + /// Uses var, which IDE0008 forbids. + public static string Value() + { + var text = typeof(Thing).Name + Environment.NewLine; + + return text; + } +} diff --git a/Nota.CodeAnalysis.Verification/Samples/Latin1Encoded.cs b/Nota.CodeAnalysis.Verification/Samples/Latin1Encoded.cs new file mode 100644 index 0000000..4c76da2 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/Latin1Encoded.cs @@ -0,0 +1,8 @@ +namespace Nota.Verification; + +/// Saved as Windows-1252 on purpose. See verify.sh. +public static class Latin1Encoded +{ + /// Danish text in the wrong encoding. + public const string Text = "æøå"; +} diff --git a/Nota.CodeAnalysis.Verification/Samples/Vendor.cs b/Nota.CodeAnalysis.Verification/Samples/Vendor.cs new file mode 100644 index 0000000..12ece93 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/Samples/Vendor.cs @@ -0,0 +1,6 @@ +namespace Nota.Vendor; + +/// Stub, so the first using resolves. +public class Thing +{ +} diff --git a/Nota.CodeAnalysis.Verification/verify-encoding.sh b/Nota.CodeAnalysis.Verification/verify-encoding.sh new file mode 100755 index 0000000..3bfe7e1 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/verify-encoding.sh @@ -0,0 +1,61 @@ +#!/usr/bin/env sh +# +# Fails if any source file is neither valid UTF-8 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 +# build and a file saved as Windows-1252. Measured: such a file compiles with no warning and no +# error, and the compiler writes U+FFFD replacement characters into the assembly. The corruption is +# silent, it reaches the binary, and no analyser reports it, because by the time an analyser runs the +# text has already been decoded. +# +# 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. +# +# 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. +# +# What this cannot catch: a wrong encoding that happens to produce valid UTF-8 - the classic "“" +# mojibake - which is indistinguishable from someone deliberately writing those characters. What it +# does catch is any file containing a byte sequence that is not legal UTF-8, which is every ordinary +# case of an ANSI file with Danish, Cyrillic or CJK text in it. +# +# POSIX sh and BSD-safe, so it runs on a developer's Mac and in a Linux build container alike. +# +# The verification samples are excluded. They are wrong on purpose - one of them is Windows-1252, so +# that NOTA0001 has something to fire on - and this check exists to find files that are wrong by +# accident. +# +# Paths are passed to a child shell by find rather than through a variable and a for loop. That is +# not fussiness: word splitting on an unquoted expansion breaks every path containing a space, and +# reports each half as unreadable - which looks exactly like a corrupt file, in a whole directory of +# them at once, because "Service References" has a space in it. + +set -eu + +root="${1:-.}" + +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/*' \ + -not -path '*/Nota.CodeAnalysis.Verification/Samples/*' \ + -exec sh -c ' + for f do + 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" ;; + esac + done + ' sh {} +)" + +if [ -n "$found" ]; then + printf 'Not valid UTF-8:\n' >&2 + printf '%s\n' "$found" | 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 +fi + +printf 'All source files are valid UTF-8 or BOM-marked UTF-16.\n' diff --git a/Nota.CodeAnalysis.Verification/verify-package.sh b/Nota.CodeAnalysis.Verification/verify-package.sh new file mode 100755 index 0000000..791be3a --- /dev/null +++ b/Nota.CodeAnalysis.Verification/verify-package.sh @@ -0,0 +1,159 @@ +#!/usr/bin/env sh +# +# Asserts that the rules survive being packaged. +# +# verify.sh proves the globalconfig's content: it imports that file directly and declares the +# analyser references itself, standing in for a consumer. That is not the same as being one, and the +# difference has already mattered twice, both times in changes that had not yet merged. +# +# - UsingLayoutAnalyser was built against a Roslyn newer than the SDK running it. The compiler +# answered CS9057, a warning, and skipped the analyser. Green build, no rules. +# - Microsoft.VisualStudio.Threading.Analyzers acquired PrivateAssets during a version bump, which +# stops a reference reaching consumers. VSTHRD100 kept firing inside this solution, because the +# verification project references the analyser directly, while no consumer received it at all. +# +# Neither is visible from inside the solution. Both are obvious the moment something installs the +# package and compiles a file that breaks a rule, which is all this does: pack, install from a local +# feed into a throwaway project, and check each rule reported. +# +# It covers the parts verify.sh cannot reach - PackagePath, the build/ props and targets being +# imported at all, and whether a dependency actually flows - and it is the only check that fails when +# a package installs cleanly and does nothing. +# +# POSIX sh and BSD-safe. Needs network on a cold cache, for the throwaway project's own dependencies. + +set -eu + +here="$(cd "$(dirname "$0")" && pwd)" +root="$(cd "$here/.." && pwd)" + +work="$(mktemp -d)" +cleanup() { rm -rf "$work"; } +trap cleanup EXIT INT TERM + +feed="$work/feed" +app="$work/app" +mkdir -p "$feed" "$app" + +# A version nothing can already have. Without this the check silently passes: NuGet extracts a +# package once per version into the global cache, so re-packing 2.2.0 and installing 2.2.0 gets +# whatever 2.2.0 was extracted first - the change under test never reaches the consumer. That is not +# hypothetical; this script gave a clean pass against a regression it was written to catch, until the +# version was made unique. +version="0.0.0-verify.$(date +%Y%m%d%H%M%S)" + +printf 'Packing %s...\n' "$version" +dotnet pack "$root/Nota.CodeAnalysis.sln" --configuration Release --output "$feed" -p:Version="$version" >"$work/pack.log" 2>&1 || { + printf 'pack failed:\n' >&2 + cat "$work/pack.log" >&2 + exit 1 +} + +package="Nota.CodeAnalysis.$version.nupkg" +[ -f "$feed/$package" ] || { printf 'no package was produced at %s\n' "$feed/$package" >&2; exit 1; } + +printf 'Testing %s as a consumer would install it.\n' "$package" + +cat > "$app/nuget.config" < + + + + + + + +EOF + +cat > "$app/App.csproj" < + + net10.0 + + false + + + + + + + +EOF + +# Every line here breaks something on purpose. What is asserted below is that each rule survived +# packaging - not that the rule works, which verify.sh already covers. +cat > "$app/Broken.cs" <<'EOF' +using Nota.Vendor; +using System; + +using Serilog; + +namespace App; + +/// Breaks one rule per analyser, on purpose. +public static class Broken +{ + /// var is IDE0008, from the globalconfig. + public static string Value() + { + var text = typeof(Thing).Name + Environment.NewLine; + + return text; + } + + /// async void is VSTHRD100, from the threading analyser. + public static async void Fire() + { + await System.Threading.Tasks.Task.Delay(1).ConfigureAwait(false); + } + + /// An unbound property is Serilog003, from SerilogAnalyzer. + public static void LogIt() => Log.Information("Hello {Name}"); +} +EOF + +cat > "$app/Vendor.cs" <<'EOF' +namespace Nota.Vendor; + +/// Stub, so the first using resolves. +public class Thing +{ +} +EOF + +# Windows-1252, for NOTA0001 - which proves build/Nota.CodeAnalysis.targets was packed and imported. +printf 'namespace App;\n\n/// Saved in the wrong encoding on purpose.\npublic static class Mis\n{\n /// Danish text.\n public const string T = "' > "$app/MisEncoded.cs" +printf '\346\370\345' >> "$app/MisEncoded.cs" +printf '";\n}\n' >> "$app/MisEncoded.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 +# 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" + +missing="" +for rule in $expected; do + if ! printf '%s' "$output" | grep -qE "(warning|error) $rule[:( ]"; then + missing="$missing $rule" + fi +done + +# 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 + missing="$missing CS9057-was-reported(an-analyser-was-skipped)" +fi + +if [ -n "$missing" ]; then + printf '\nThe package does not deliver:%s\n' "$missing" >&2 + printf 'It installed without complaint. That is what this check is for.\n\n' >&2 + printf '%s\n' "$output" >&2 + exit 1 +fi + +printf 'All rules survive packaging.\n' diff --git a/Nota.CodeAnalysis.Verification/verify.sh b/Nota.CodeAnalysis.Verification/verify.sh new file mode 100755 index 0000000..2f7a395 --- /dev/null +++ b/Nota.CodeAnalysis.Verification/verify.sh @@ -0,0 +1,59 @@ +#!/usr/bin/env bash +# +# Asserts that the rules this package ships actually report. +# +# The product of this repository is configuration, and configuration fails silently. CS8019 sat in +# the globalconfig asking for unused usings to be reported, and reported nothing, for as long as +# anyone can tell - because a package that ships settings has nothing that exercises them. This is +# the thing that would have said so on the first build. +# +# Run from anywhere. Exits non-zero with the missing rules named. + +set -euo pipefail + +here="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +props="$here/../Nota.CodeAnalysis/build/Nota.CodeAnalysis.props" + +failures=() + +# 1. The verification project stands in for a consumer, so the properties it sets itself have to be +# the ones the props file gives a consumer. If they drift, this project proves nothing about what +# anyone actually gets. +for property in EnforceCodeStyleInBuild EnableNETAnalyzers GenerateDocumentationFile ImplicitUsings; do + if ! grep -q "<$property>" "$props"; then + failures+=("build/Nota.CodeAnalysis.props no longer sets $property, which this project assumes") + fi +done + +# 2. Broken.cs breaks each of these on purpose. A rule that stops reporting is a rule that has been +# switched off, renamed, or - as with CS8019 - never worked. +# +# IDE0005 unused using directive the one CS8019 was supposed to cover +# IDE0008 var instead of an explicit type +# UA1000 using directives out of order UsingLayoutAnalyser +# SA1208 System usings not placed first +# SA1516 no blank line after the System group +# 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 SA1208 SA1516 NOTA0001) + +# VerifyRules is what pulls Samples/ into the compilation. Without it the project builds empty, which +# is what every other build of this solution wants. +output="$(dotnet build "$here/Nota.CodeAnalysis.Verification.csproj" \ + --no-incremental -v:m -p:VerifyRules=true -p:TreatWarningsAsErrors=false 2>&1 || true)" + +for rule in "${expected[@]}"; do + if ! grep -qE "(warning|error) $rule[:(]" <<<"$output"; then + failures+=("$rule did not report - it is configured but not reaching consumers") + fi +done + +if [ ${#failures[@]} -ne 0 ]; then + printf 'Verification failed:\n' >&2 + printf ' - %s\n' "${failures[@]}" >&2 + printf '\nFull build output:\n%s\n' "$output" >&2 + exit 1 +fi + +printf 'All %d rules reported.\n' "${#expected[@]}" diff --git a/Nota.CodeAnalysis.sln b/Nota.CodeAnalysis.sln index b323dad..10f67ea 100644 --- a/Nota.CodeAnalysis.sln +++ b/Nota.CodeAnalysis.sln @@ -5,16 +5,42 @@ VisualStudioVersion = 17.8.34525.116 MinimumVisualStudioVersion = 10.0.40219.1 Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Nota.CodeAnalysis", "Nota.CodeAnalysis\Nota.CodeAnalysis.csproj", "{C2D84812-9EAA-4DCF-A562-02A26FACE800}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Nota.CodeAnalysis.Verification", "Nota.CodeAnalysis.Verification\Nota.CodeAnalysis.Verification.csproj", "{1053892B-03AC-49E2-8E7F-4E83D05A835D}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU + Debug|x64 = Debug|x64 + Debug|x86 = Debug|x86 Release|Any CPU = Release|Any CPU + Release|x64 = Release|x64 + Release|x86 = Release|x86 EndGlobalSection GlobalSection(ProjectConfigurationPlatforms) = postSolution {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|Any CPU.ActiveCfg = Debug|Any CPU {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|Any CPU.Build.0 = Debug|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|x64.ActiveCfg = Debug|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|x64.Build.0 = Debug|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|x86.ActiveCfg = Debug|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Debug|x86.Build.0 = Debug|Any CPU {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|Any CPU.ActiveCfg = Release|Any CPU {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|Any CPU.Build.0 = Release|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|x64.ActiveCfg = Release|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|x64.Build.0 = Release|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|x86.ActiveCfg = Release|Any CPU + {C2D84812-9EAA-4DCF-A562-02A26FACE800}.Release|x86.Build.0 = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|Any CPU.Build.0 = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|x64.ActiveCfg = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|x64.Build.0 = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|x86.ActiveCfg = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Debug|x86.Build.0 = Debug|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|Any CPU.ActiveCfg = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|Any CPU.Build.0 = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x64.ActiveCfg = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x64.Build.0 = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x86.ActiveCfg = Release|Any CPU + {1053892B-03AC-49E2-8E7F-4E83D05A835D}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE diff --git a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj index 1e2987d..dc2c206 100644 --- a/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj +++ b/Nota.CodeAnalysis/Nota.CodeAnalysis.csproj @@ -4,18 +4,32 @@ true https://github.com/Notalib/Nota.CodeAnalysis true - 2.1.4 + + 2.2.0 content/README.md + - + + + diff --git a/Nota.CodeAnalysis/README.md b/Nota.CodeAnalysis/README.md deleted file mode 100644 index af6fdd9..0000000 --- a/Nota.CodeAnalysis/README.md +++ /dev/null @@ -1 +0,0 @@ -# Opinionated Installable Nuget Package with Nota Codestyle, Stylecop and Code Analysis rules. diff --git a/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets new file mode 100644 index 0000000..6050bfd --- /dev/null +++ b/Nota.CodeAnalysis/build/Nota.CodeAnalysis.targets @@ -0,0 +1,86 @@ + + + + + + + true + + + + + + + + = 2 + && ((bytes[0] == 0xFF && bytes[1] == 0xFE) || (bytes[0] == 0xFE && bytes[1] == 0xFF))) + { + continue; + } + + try + { + new System.Text.UTF8Encoding(false, true).GetString(bytes); + } + catch (System.Text.DecoderFallbackException) + { + Log.LogError( + null, + "NOTA0001", + null, + path, + 1, + 1, + 0, + 0, + "File is not valid UTF-8. It will compile without complaint and reach the assembly as U+FFFD replacement characters. Re-save it as UTF-8, and check the tool or editor that last wrote it."); + } + } + ]]> + + + + + + + + + diff --git a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig index 73656d5..3e82801 100644 --- a/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig +++ b/Nota.CodeAnalysis/content/Nota.CodeAnalysis.globalconfig @@ -1,5 +1,5 @@ is_global = true -charset = utf-8-bom +charset = utf-8 indent_style = space indent_size = 4 tab_width = 4 @@ -8,8 +8,37 @@ end_of_line = crlf # Stylecop settings ## Using Directives dotnet_sort_system_directives_first = true + +# Half of this is enforced and half is not, so do not read it as a promise. SA1516 uses it to require +# the blank line after the System group, and that part works. The per-vendor grouping the name +# suggests - Microsoft together, Serilog together - is checked by IDE0055 only, which is off here, so +# nothing reports it. UsingLayoutAnalyser below is what actually enforces the grouping; this stays +# for the System boundary, and because turning it off would silence that too. dotnet_separate_import_directive_groups = true +## Using layout - UsingLayoutAnalyser (UA1000, UA1001) +# System, then third party, then the consuming solution's own namespaces, as blocks separated by a +# blank line, each sorted alphabetically, and one run per vendor inside a block. Neither StyleCop nor +# the built-in options can express that: SA1208/SA1210 know only "System first, then alphabetical", +# and dotnet_separate_import_directive_groups groups by first-level namespace with no notion of whose +# code it is. +# +# Defaulted to Nota, because that is what nearly every consumer's own code is called, and a setting +# every repository has to remember is a setting most repositories will not have. A consumer whose +# code is called something else overrides it in their own .editorconfig: +# +# [*.cs] +# usinglayout.first_party_prefixes = Contoso, Fabrikam +# +# That does take precedence - verified against the built package, not assumed: an .editorconfig entry +# beats a global analyzer config entry for the same key. The same is true of every severity here, so +# nothing in this file is a decision a consumer is stuck with. +# +# A repository that gets this wrong still gets a sensible layout - its own namespaces are simply +# treated as one more vendor - which is why it is worth defaulting rather than demanding. +usinglayout.first_party_prefixes = Nota +usinglayout.separate_roots = true + ## naming rules stylecop.naming.tupleElementNameCasing = camelCase @@ -497,7 +526,12 @@ dotnet_diagnostic.CA5405.severity = warning dotnet_diagnostic.CS0649.severity = warning dotnet_diagnostic.CS1574.severity = warning dotnet_diagnostic.CS1591.severity = none +# Inert, and kept only so nobody sets it again expecting an effect. CS8019 is emitted hidden by the +# compiler and a severity here does not raise it: with this line and nothing else, a file with an +# unused using builds clean. Measured, not assumed. IDE0005 below is the rule that reports, and it +# needs GenerateDocumentationFile, which build/Nota.CodeAnalysis.props already sets. dotnet_diagnostic.CS8019.severity = warning +dotnet_diagnostic.IDE0005.severity = warning dotnet_diagnostic.IDE0008.severity = error dotnet_diagnostic.IDE0016.severity = none dotnet_diagnostic.IDE0016WithoutSuggestion.severity = none @@ -520,12 +554,25 @@ dotnet_diagnostic.SA1127.severity = none dotnet_diagnostic.SA1201.severity = none dotnet_diagnostic.SA1202.severity = none dotnet_diagnostic.SA1204.severity = warning -dotnet_diagnostic.SA1210.severity = warning +# Off, and it cannot be anything else while UA1000 owns the ordering. SA1210 sorts the whole list +# alphabetically end to end, so it wants a solution's own root above a vendor whenever it sorts +# earlier - Contoso above Microsoft - which is exactly what the layout moves to the bottom. Every +# laid-out file would be an SA1210 warning. SA1208, SA1209, SA1211, SA1216 and SA1217 do not conflict +# and stay on. +dotnet_diagnostic.SA1210.severity = none dotnet_diagnostic.SA1309.severity = none dotnet_diagnostic.SA1310.severity = warning dotnet_diagnostic.SA1401.severity = warning dotnet_diagnostic.SA1402.severity = suggestion -dotnet_diagnostic.SA1412.severity = warning +# Off: source is UTF-8 without a BOM. SA1412 is the only thing that was requiring one, and it does +# not read charset at all - it demands a BOM whether charset says utf-8 or utf-8-bom, which is why +# turning it off is the change and editing charset is not. +# +# Nothing enforces the other direction. No analyser reports a BOM that should not be there, and +# charset below reaches no editor: this file arrives through MSBuild as a global analyzer config, +# and editors read .editorconfig from disk. A repository that wants the absence of a BOM enforced +# needs its own .editorconfig, or a check outside the compiler. +dotnet_diagnostic.SA1412.severity = none dotnet_diagnostic.SA1413.severity = warning dotnet_diagnostic.SA1600.severity = none dotnet_diagnostic.SA1601.severity = none diff --git a/README.md b/README.md index 8c48bae..93571e4 100644 --- a/README.md +++ b/README.md @@ -1 +1,115 @@ -# Installable Nuget Package with Nota Codestyle, Stylecop and Code Analysis rules. +# Nota.CodeAnalysis + +Nota's code style, StyleCop and code analysis rules, as one package. Install it and a project gets +the same rules as every other project here, enforced at build time rather than agreed in a wiki. + +## Installing + +The package lives on Nota's GitHub Packages feed, so a `NuGet.config` needs the source: + +```xml + +``` + +Then reference it once per project, or once in a `Directory.Build.props` for a whole solution: + +```xml + + all + runtime; build; native; contentfiles; analyzers; buildtransitive + +``` + +`PrivateAssets` matters: without it the rules flow to anything that references your library, and +whether *your* code uses `var` is nobody else's build error. + +## What arrives with it + +Four properties are switched on, in `build/Nota.CodeAnalysis.props`: + +| Property | Why | +|----------|-----| +| `EnforceCodeStyleInBuild` | without it the IDE#### rules never run, whatever their severity says | +| `EnableNETAnalyzers` | the CA#### rules | +| `GenerateDocumentationFile` | load-bearing, and not obviously so: severity is a filter, and this is the switch that produces the diagnostics being filtered. Remove it and `IDE0005` silently stops reporting | +| `ImplicitUsings` | disabled - usings are stated, not inherited | + +Four analyser packages come as dependencies: StyleCop.Analyzers, Microsoft.VisualStudio.Threading.Analyzers, +SerilogAnalyzer, and UsingLayoutAnalyser. Around 400 rule severities are set in +`content/Nota.CodeAnalysis.globalconfig`. + +## Configuring it + +Nothing is required. The one setting most likely to need changing is which namespaces count as +*yours*, for the using layout - it defaults to `Nota`, which is right for almost everything here. + +If your code is called something else, say so in your own `.editorconfig`: + +```ini +[*.cs] +usinglayout.first_party_prefixes = Contoso, Fabrikam +``` + +Comma-separated for several roots. Getting it wrong is not fatal - your namespaces are simply sorted +as one more vendor rather than last. + +## Rules worth knowing before your first build + +Most of this is unsurprising. These are the ones that catch people out: + +- **`IDE0008` is an error.** `var` is banned outright; write the type. +- **`VSTHRD100` is an error.** No `async void`. +- **`NOTA0001`** fails the build on a source file that is not valid UTF-8. It is an MSBuild task + rather than an analyser because it has to see the bytes: a file saved as Windows-1252 compiles with + 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. +- **`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`. + +## Turning things off + +Any rule can be overridden in the consuming repository's own `.editorconfig`. An `.editorconfig` +entry beats a global analyzer config entry for the same key, so nothing this package sets is a +decision you are stuck with: + +```ini +dotnet_diagnostic.IDE0008.severity = suggestion +``` + +The encoding check is a build task rather than a diagnostic, so it has its own switch: + +```xml +false +``` + +## Working on this repository + +The product here is configuration, and configuration fails silently: a rule that cannot report looks +exactly like a rule being obeyed, because the build is quiet either way. `dotnet_diagnostic.CS8019` +asked for unused usings to be reported and reported nothing for years, with forty-five of them +collected behind it in one consuming solution. + +`Nota.CodeAnalysis.Verification` exists to make that noisy. It is a consumer built against files that +break the rules on purpose, and a script that fails if any of them stayed quiet. Read its README +before changing rules. + +```sh +./Nota.CodeAnalysis.Verification/verify.sh # the rules report +./Nota.CodeAnalysis.Verification/verify-encoding.sh # source is valid UTF-8 +``` + +Both run on pull requests as well as on `main`. + +Releases are cut by tagging: + +```sh +git tag -a v2.3.0 -m "Nota.CodeAnalysis 2.3.0" && git push origin v2.3.0 +``` + +The tag is the version, so nothing in the repository can disagree with what shipped. `` in +the csproj is only a local default, for anyone packing by hand. + +Merging to `main` builds and verifies but does not publish, and neither do pull requests - releasing +is a separate act from merging.