diff --git a/.llm/references/forbidden-patterns.md b/.llm/references/forbidden-patterns.md index 3833e4c..f7c6ee6 100644 --- a/.llm/references/forbidden-patterns.md +++ b/.llm/references/forbidden-patterns.md @@ -36,3 +36,4 @@ Patterns that must not appear in this codebase, with the compliant alternative. | Static mutable state with no documented reason or teardown reset | document why it is static, weak-key by lifecycle owner, unregister on every removal path, and reset it in the owning window's `Cleanup()` (`AssetGuidTypeIndex.Shared` suspension reset and the theme-selection ownership registry are the precedents) | static editor state outlives windows and strands cross-window state (PR #122 review finding) | | `using` directives above the `namespace` declaration | place them inside the namespace block; file-level usings are sanctioned only for namespaceless assembly-attribute files and `[assembly: ...]` preambles like `InternalsVisibleTo` (`npm run lint:csharp-usings` enforces this, `:fix` moves the simple cases) | one namespace-scoped convention keeps the file header stable and the rule mechanically checkable (#124) | | A guard that reads a machine-readable config resolves only part of a construct, such as the first `{a,b}` alternative, a nested group, or one level of a nested mapping | resolve every alternative and nesting level, or refuse the pattern with the file and line that uses it | a partially-read rule still matches its own representative path, so the guard passes while the real configuration disagrees; refusing beats guessing (PR #131 review finding in `scripts/lint-line-endings.js`) | +| A guard that names the argument forms it rejects and passes everything it does not name | require the exact arguments the policy permits, and report the file with every argument found | a switch the reader does not recognize turns off the policy the guard claims to enforce, so `-nowarn:1701` or `-warnaserror-` in a `csc.rsp` passed the guard that owns warnings-as-errors (`scripts/lint-assembly-warnings.ps1`, #132) | diff --git a/scripts/lint-assembly-warnings.ps1 b/scripts/lint-assembly-warnings.ps1 index 05929c7..3894be3 100644 --- a/scripts/lint-assembly-warnings.ps1 +++ b/scripts/lint-assembly-warnings.ps1 @@ -3,6 +3,11 @@ param([string]$Root) $ErrorActionPreference = 'Stop' +# The one argument a compiler response file may carry: the ruleset already +# promotes every warning, so a response file that also switched +# warnings-as-errors on or off would give one policy two sources of truth. +$maximumWarningsArgumentPattern = '^(?:-|/)(?:warn|w):5$' + function Format-DisplayPath { param([string]$FullPath, [string]$BasePath) @@ -52,29 +57,25 @@ try { ForEach-Object { $_.Trim() } | Where-Object { -not [string]::IsNullOrWhiteSpace($_) } ) - $warningLevelArguments = @( + # The response file carries the warning level and nothing else, so + # every argument other than the maximum warning level fails instead + # of a name being checked for: a switch this guard does not name, + # such as -nowarn: or -warnaserror-, turns off the policy silently. + # Comment lines, several arguments on one line, and a repeated + # warning level fail too, because the guard does not model them. + $unexpectedArguments = @( $responseArguments | - Where-Object { $_ -match '^(?:-|/)(?:warn|w)(?::|$)' } + Where-Object { $_ -notmatch $maximumWarningsArgumentPattern } ) - if ( - $warningLevelArguments.Count -ne 1 -or - $warningLevelArguments[0] -notmatch '^(?:-|/)(?:warn|w):5$' - ) { + if ($responseArguments.Count -ne 1 -or 0 -lt $unexpectedArguments.Count) { + $foundArguments = if (0 -eq $responseArguments.Count) { + 'no arguments' + } else { + $responseArguments -join ' ' + } Write-Host ( "[assembly-warnings] ERROR: $responseFilePath must contain exactly one " + - 'maximum warning-level argument (-warn:5)' - ) - $errors++ - } - - $analyzerArguments = @( - $responseArguments | - Where-Object { $_ -match '^(?:-|/)analyzer:' } - ) - if (0 -lt $analyzerArguments.Count) { - Write-Host ( - "[assembly-warnings] ERROR: $responseFilePath must not load analyzers from " + - 'this package repository; configure development analyzers in the host Unity project' + "argument, the maximum warning level (-warn:5); found: $foundArguments" ) $errors++ } diff --git a/scripts/tests/test-assembly-warnings.ps1 b/scripts/tests/test-assembly-warnings.ps1 index 523d05b..717f520 100644 --- a/scripts/tests/test-assembly-warnings.ps1 +++ b/scripts/tests/test-assembly-warnings.ps1 @@ -17,12 +17,13 @@ function Write-AssemblyFixture { param( [string]$Root, [string]$Directory, - [string]$Ruleset = $warningsAsErrorsRuleset + [string]$Ruleset = $warningsAsErrorsRuleset, + [string]$ResponseFile = $maximumWarningsResponseFile ) Write-FixtureFile -Root $Root -RelativePath "$Directory/Fixture.asmdef" -Content $assemblyDefinition Write-FixtureFile -Root $Root -RelativePath "$Directory/WarningsAsErrors.ruleset" -Content $Ruleset - Write-FixtureFile -Root $Root -RelativePath "$Directory/csc.rsp" -Content $maximumWarningsResponseFile + Write-FixtureFile -Root $Root -RelativePath "$Directory/csc.rsp" -Content $ResponseFile } Write-Host '== assembly warnings policy self-tests ==' @@ -91,31 +92,143 @@ Invoke-TestCase 'Fails_WhenAssemblyHasAmbiguousCompilerResponseFiles' { } } -Invoke-TestCase 'Fails_WhenCompilerWarningLevelIsLowerThanMaximum' { - $root = New-TempRoot - try { - Write-AssemblyFixture -Root $root -Directory 'Runtime' - Write-FixtureFile -Root $root -RelativePath 'Runtime/csc.rsp' -Content '-warn:3' - - $output = & $lintScript -Root $root *>&1 | Out-String - Assert-ExitCode 1 'a lower compiler warning level should fail' - Assert-True ($output -match 'maximum warning-level argument') "output should require maximum warnings, got: $output" - } finally { - Remove-TempRoot $root +# Each row is one response file and the outcome the guard owes it. The +# suppression rows are the shapes a prefix list accepts by not naming them: a +# switch it never reads turns off the policy it claims to enforce. The last rows +# are constructs the guard does not model at all, so it refuses them instead of +# guessing what they do. +$responseFileCases = @( + [pscustomobject]@{ + Name = 'Passes_WhenTheResponseFileCarriesOnlyTheMaximumWarningLevel' + Content = '-warn:5' + ExpectPass = $true + Expect = '' } -} - -Invoke-TestCase 'Fails_WhenCompilerWarningLevelIsMalformed' { - $root = New-TempRoot - try { - Write-AssemblyFixture -Root $root -Directory 'Runtime' - Write-FixtureFile -Root $root -RelativePath 'Runtime/csc.rsp' -Content '-warn:all' - - $output = & $lintScript -Root $root *>&1 | Out-String - Assert-ExitCode 1 'a malformed compiler warning level should fail' - Assert-True ($output -match 'maximum warning-level argument') "output should require -warn:5, got: $output" - } finally { - Remove-TempRoot $root + [pscustomobject]@{ + Name = 'Passes_WhenTheResponseFileUsesTheShortWarningLevelForm' + Content = '-w:5' + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + Name = 'Passes_WhenTheResponseFileUsesTheSlashSpelling' + Content = '/warn:5' + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + # Blank lines are not arguments, and most committed files end with one. + Name = 'Passes_WhenTheResponseFileEndsWithBlankLines' + Content = "-warn:5`n`n" + ExpectPass = $true + Expect = '' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheWarningLevelIsLowerThanMaximum' + Content = '-warn:3' + ExpectPass = $false + Expect = 'found: -warn:3' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheWarningLevelIsMalformed' + Content = '-warn:all' + ExpectPass = $false + Expect = 'found: -warn:all' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileSuppressesNamedWarnings' + Content = "-warn:5`n-nowarn:1701,CS0162`n" + ExpectPass = $false + Expect = 'found: -warn:5 -nowarn:1701,CS0162' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileSuppressesEveryWarning' + Content = "-warn:5`n-nowarn`n" + ExpectPass = $false + Expect = 'found: -warn:5 -nowarn' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileUsesTheSlashSuppressionForm' + Content = "-warn:5`n/nowarn`n" + ExpectPass = $false + Expect = 'found: -warn:5 /nowarn' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileDisablesWarningsAsErrors' + Content = "-warn:5`n-warnaserror-`n" + ExpectPass = $false + Expect = 'found: -warn:5 -warnaserror-' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileDisablesWarningsAsErrorsWithTheSlashForm' + Content = "/warn:5`n/warnaserror-`n" + ExpectPass = $false + Expect = 'found: /warn:5 /warnaserror-' + } + [pscustomobject]@{ + # The ruleset already promotes every warning, so a second switch either + # way gives the one policy a second source of truth. + Name = 'Fails_WhenTheResponseFileEnablesWarningsAsErrorsSeparately' + Content = "-warn:5`n-warnaserror+`n" + ExpectPass = $false + Expect = 'found: -warn:5 -warnaserror\+' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileLoadsAPackageAnalyzer' + Content = "-warn:5`n-analyzer:`"Runtime/Analyzer.dll`"`n" + ExpectPass = $false + Expect = 'found: -warn:5 -analyzer:"Runtime/Analyzer\.dll"' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileHidesASuppressionBehindAComment' + Content = "-warn:5`n# -nowarn:1701`n" + ExpectPass = $false + Expect = 'found: -warn:5 # -nowarn:1701' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFilePacksTwoArgumentsOnOneLine' + Content = '-warn:5 -nowarn:1701' + ExpectPass = $false + Expect = 'must contain exactly one argument' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileRepeatsTheWarningLevel' + Content = "-warn:5`n-warn:5`n" + ExpectPass = $false + Expect = 'found: -warn:5 -warn:5' + } + [pscustomobject]@{ + Name = 'Fails_WhenTheResponseFileIsEmpty' + Content = '' + ExpectPass = $false + Expect = 'found: no arguments' + } +) + +foreach ($script:responseFileCase in $responseFileCases) { + Invoke-TestCase $script:responseFileCase.Name { + $root = New-TempRoot -Prefix 'assembly-warnings-' + try { + Write-AssemblyFixture ` + -Root $root ` + -Directory 'Runtime' ` + -ResponseFile $script:responseFileCase.Content + + $output = & $lintScript -Root $root *>&1 | Out-String + if ($script:responseFileCase.ExpectPass) { + Assert-ExitCode 0 "$($script:responseFileCase.Name) should pass" + } else { + Assert-ExitCode 1 "$($script:responseFileCase.Name) should fail" + Assert-True ( + $output -match 'Runtime/csc\.rsp' + ) "the response file should be named, got: $output" + Assert-True ( + $output -match $script:responseFileCase.Expect + ) "expected '$($script:responseFileCase.Expect)' in: $output" + } + } finally { + Remove-TempRoot $root + } } } @@ -164,23 +277,6 @@ Invoke-TestCase 'Fails_WhenIncludeAllDoesNotPromoteWarnings' { } } -Invoke-TestCase 'Fails_WhenCompilerResponseFileLoadsPackageAnalyzer' { - $root = New-TempRoot - try { - Write-AssemblyFixture -Root $root -Directory 'Runtime' - Write-FixtureFile ` - -Root $root ` - -RelativePath 'Runtime/csc.rsp' ` - -Content "-warn:5`n-analyzer:`"Runtime/Analyzer.dll`"" - - $output = & $lintScript -Root $root *>&1 | Out-String - Assert-ExitCode 1 'a package-local analyzer argument should fail' - Assert-True ($output -match 'must not load analyzers') "output should reject package-local analyzers, got: $output" - } finally { - Remove-TempRoot $root - } -} - Invoke-TestCase 'Fails_WhenRepositoryContainsLabeledAnalyzer' { $root = New-TempRoot try {