fix: the eighteen defects of the third review - #74
Merged
Conversation
Assembly.GetTypes() hands such an enum over in its open form, Box`1+Colour,
where Type.IsEnum is true and ContainsGenericParameters is true as well.
FieldInfo.GetValue on any member of it throws ArgumentException out of
Enum.InternalBoxEnum, and that happens in EnumContract's constructor,
before the contract is so much as looked at.
So an enum nobody annotated, nobody registered and nobody wanted took the
whole application down at start-up, with a message naming neither the type
nor this package. Reproduced on a host declaring
public class Box<T> { public enum Colour { Red } } and calling
AddEnumMemberNameBinding() with no options at all.
The scan now passes it by, as it passes by any enum it cannot read. Naming
the closed form stays available and is pinned: it carries no generic
parameter, so AddEnum<Crate<int>.State>() registers it as any other.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NullGuardTests generated a row for AddEnumMemberNameBinding and then returned before building an argument array, so the row asserted nothing and reported green. The reason recorded beside it — that an IMvcBuilder can only come from a configured service collection — was not true of what the row needs, which is a null. Removing the exemption made the row pass as written. The package's one public entry point had therefore gone unguarded by any test for as long as the list existed. CA1062 holds a second line, and the two cover different things: the analyzer refuses a boundary that validates nothing, this row refuses one that validates with the wrong exception. Verified by mutation — replacing the guard with an InvalidOperationException compiles clean and fails here, naming the parameter and the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
FormattingTests.an_alias_is_written_with_the_first_declared_name asserted the literal "first" twice and named a rule EnumContract.Format does not implement. NamesByValue walks Enum.GetNames and keeps the first name it meets there, which sorts by the binary value and, among members sharing one, does not keep the order they were written in. The name survived because its fixture cannot tell the two apart: Aliased is written in ascending order, so declaration order and GetNames order coincide on it and no assertion could separate them. The expectation now comes from JsonSerializer rather than being restated, which is the only reading that cannot go stale, and the test is named for what it holds. FormattingParityTests already covers the shapes where the two orders disagree — three of its seven do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The summary opened "A value with no comma in it." That was true of the caller it was written for, and adfd999 reversed the order without reaching this sentence: TryParse now hands TryParseSingle the whole trimmed value before it tests for a comma, and only splits when the lookup misses. Measured — instrumenting the method shows it receives the literal token "a,b" on the non-[Flags] fixture CommaInsideAName and resolves it to Ab (4), matching System.Text.Json; the split path could only produce 3. The sentence therefore stated the resolution order backwards, against the two paragraphs elsewhere in the file that establish it and the rule it justifies. The summary's second sentence is untouched and stays true: no C# member name can contain a comma, so the exact-spelling branch can only fire for a token that has none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It accepts one. EnumContract.TryParse has no [Flags] test on the parse path at all, so "first,second" on a non-[Flags] contract resolves to 0 | 1, matching System.Text.Json, which splits before it looks at the attribute. Refusing it would make a registered enum stricter than the same enum left alone, which the package promises never to do. What [Flags] decides is whether the values an application will bind are an open set rather than the declared members alone — which is what IsFlagsContract states in its own XML doc one file away, and what the comment on this test now says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three comments said the model binder provider consults the record on every request. IModelBinderProvider.GetBinder is called by ModelBinderFactory only while building a binder for a (parameter, model type), and the factory caches the result. Measured by instrumenting Contains and issuing five requests to one action: called once, not five times. The atomicity argument the surrounding comments make is unaffected — it needs the record to be filled before any binder is built, which is still after AddEnumMemberNameBinding has returned — and the concurrent dictionary is still the right choice for the same reason. Only the frequency was wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The binder's remark listed four behaviours it reproduces from SimpleTypeModelBinder — the repeated key, the blank value, the message a failure earns, and the refusal to bind an undefined value — and closed "ModelBindingBehaviourTests holds all of them". That suite is five tests and holds two of the four. The blank value on a required and a nullable parameter is EmptyValueTests; the undefined-value refusal is ParityWithSystemTextJsonTests, control included. A reader sent to ModelBindingBehaviourTests to check either found a file that does not mention them. The remark now names the three suites and what each holds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment said the pattern sits "inside a JSON snippet in a markdown table, so backslashes are doubled and pipes are escaped for the table". Both pages carry it in a fenced json block, and the pipes of (read|write|delete) are written bare — checked in both languages. So the \| replacement the sentence justified could not fire on any input the regex admits: the capture group stops at the first quote, and no pattern the transformer emits contains \|. Removing it leaves the two tests green. A dead branch behind a false explanation is worse than either alone: the explanation is what stops a reader from noticing the branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The page said "the binder picks the first declaration, and the second member becomes unreachable — silently, and in declaration order". Nothing is picked and nothing is silent: EnumContract's constructor collects Problem.DuplicateName when byContractName.TryAdd fails and then throws EnumContractException for the whole type, so the enum never binds at all. Measured — EnumContract.For on a duplicate-name enum throws. The paragraph now says what happens: the shape is refused, at build time by this rule and at start-up by the exception. The claim about System.Text.Json in the same section is a separate defect, corrected in the commit that follows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It accepts it. Measured on { ["same"] First = 0, ["same"] Second = 1 }
with JsonStringEnumConverter<T>(null, false): the converter builds,
reading "same" gives First, and writing either member gives "same" —
the enum round-trips one value and loses the other with nothing said.
The only enum-identifier throw in the serializer is for a null, empty or
whitespace-padded name, and a comma on a [Flags] enum. Duplicates are not
among them.
So this is one of the few places the package is deliberately stricter
than the serializer, which is a stronger reason for the rule than the one
the page gave — and the page said the opposite of the fact.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two halves, both false, and the second is the one that mattered: the sentence said the undefined-combination 400 is "ASP.NET Core's rather than this package's". It is this package's own. Its binder is registered ahead of the provider ASP.NET Core uses for enums, so EnumTypeModelBinder never sees the value — the check is reproduced here deliberately. The first half called it "the one input the body accepts and no other channel does". There are at least three: this one, the [Flags] shape the limitations page describes, and a name carrying a character a route or a header cannot transport. limitations.en.md was corrected for both a3637ee ago; this page was not, so the two sibling pages have been contradicting each other since. Fixed in both languages. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shipped description read "System.Text.Json refuses a comma inside a declared name on a [Flags] enum, where the whole name is never looked up before the value is split". The subordinate clause is false, and this repository measures the opposite: EnumContract.TryParse tries the whole trimmed value as one name before splitting, with no [Flags] carve-out, and ReadParityTests holds that order against the serializer token by token. I wrote the clause in the commit that scoped this rule to [Flags], and it was wrong there. It is also not the reason. The serializer validates declared names when it builds the converter and throws for a comma on a [Flags] enum — "Flags enums must additionally not contain commas" — so the enum cannot be serialized at all. That is what the rule page and Problem.CommaInFlagsName both say, and now what the description says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both messages said the declared name is matched first, so the value
resolves to the annotated member and the unannotated one is left over.
That is true of one declaration order and false of the other.
Measured against the serializer, both ways round:
{ ["Blue"] Red = 0, Blue = 1 } -> "Blue" reads Red (0)
{ Blue = 0, ["Blue"] Red = 1 } -> "Blue" reads Blue (0)
What decides is Enum.GetNames order: the first member met claims the
spelling, whether its name is the declared one or its own C# name. So
moving the annotation to the second member reverses which member
disappears, and the message named the survivor as the loser.
The messages now say that both answer to the spelling and that GetNames
order decides, without asserting which. A theory holds both orders
against JsonSerializer. The rule pages are untouched: their example
declares the annotated member first, where the old wording was right.
One assertion moves with it — the exception test pinned the word
"casing", which the message no longer uses; it now asks for the word
that carries the meaning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The NoWarn justification for RS2008 reads "ceremony for a five-rule analyzer". SupportedDiagnostics exposes six descriptors, EMN0001 through EMN0006, and the documentation suite holds a page for each. The count is the whole weight of the argument for skipping release tracking, so it should be the real one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestSources() enumerates every *.cs under tests/ recursively, and there are four projects there — Tests, OpenApi.Tests, Analyzers.Tests and Documentation.Tests — all four carrying an NFluent reference. The ADR the remark cites repeated the count in both languages. Named for the directory rather than for a number, since that is what the code does and what stays true the next time a project is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment justified a word split with "none of which can contain a
space, since the repository has no such file and git would quote it if it
did". Both halves are wrong, and they fail in opposite directions.
Measured on a repository holding 'My Enum.cs', 'Café.cs' and 'Plain.cs':
git diff --cached --name-only -> My Enum.cs
"Caf\303\251.cs"
old logic -> 4 paths for 3 files: ["Caf\303\251.cs"] NO, [My] NO,
[Enum.cs] NO, [Plain.cs] yes
new logic -> 3 paths, all three existing
git prints a spaced path unquoted, so the split tore it in two, and it
prints a non-ASCII one quoted and escaped, which the split kept whole and
which names nothing on disk. Either way the checker was handed something
that is not the file.
core.quotePath=false stops the escaping and reading a line at a time into
"$@" stops the tearing, which is also what removes the SC2086 suppression
rather than re-justifying it. A path containing a newline is still beyond
this loop, and the comment says so instead of claiming otherwise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tests/PackageSmokeTest/Directory.Build.props is empty and load-bearing: without it the fixtures inherit the repository's own defaults and stop resembling the thing they exist to imitate. Its comment closed "whatever these fixtures need, they declare themselves", and that was not true. MSBuild walks up for Directory.Build.props and again, independently, for Directory.Build.targets. Only the props file was shadowed, so the second walk reached the repository's own targets and added Microsoft.CodeAnalysis.PublicApiAnalyzers to every fixture under it. Measured: RS0016 appears ten times in InvalidContract's build log. An empty Directory.Build.targets beside the props closes the second walk. After it, RS0016 is gone from that log and the smoke test still reports the published package behaves as documented. The fix is the file rather than the sentence: a fixture imitating somebody else's project consuming us from nuget.org should not be carrying our analyzers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
run.sh, Consumer/Program.cs, both smoke-test READMEs and both changelogs all said AddEnumMemberNameBinding() with no options is a path no test in this repository can reach, because Assembly.GetEntryAssembly() under a test host is testhost.dll. xUnit v3 generates the entry point into the test assembly itself, so GetEntryAssembly() there is the test assembly. EntryAssemblyScanTests .configuring_nothing_scans_the_entry_assembly calls the zero-option form and asserts that Delivery, an enum declared in that assembly and reached by no endpoint, was scanned. It passes. That mattered beyond the sentence: it was the stated reason the fixtures are applications rather than tests, so the whole section rested on it. The reason that holds is the package — a PackageReference to the packed .nupkg, the analyzers inside it, the MSBuild assets and a real Kestrel, none of which a ProjectReference from a test project touches. Every one of the six now says that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The other fourteen are internal — comments, test names, a shell hook, an MSBuild file — and the changelog documents the packages. These four a consumer meets: two diagnostic messages, and two pages they read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The third complete review confirmed 18 distinct defects — 37 candidates found, 16 refuted, every survivor reproduced by execution. All 18 are fixed here, one commit each.
One is a runtime bug that stops an application booting. Two are tests that report green while holding nothing. Fifteen are statements in the repository that are false, and four of those were left behind by earlier fixes in this same repository.
The runtime one
An enum nested in a generic type stopped the application booting.
GetTypes()returns the open formBox\1+Colour, whereIsEnumis true andContainsGenericParametersis true as well, andFieldInfo.GetValue` refuses it. That happens before the contract is looked at, so an enum nobody annotated, nobody registered and nobody wanted took the whole application down. The scan now passes it by; naming the closed form still registers it, which is pinned.The two tests
NullGuardTestsskipped the package's one public entry point. The row forAddEnumMemberNameBindingreturned before building an argument array — green, asserting nothing. The recorded reason was false: removing the exemption made it pass as written. Verified by mutation (anInvalidOperationExceptionin place of the guard compiles clean and now fails).FormattingTestsnamed a rule the code does not implement, on the one fixture where the two orders coincide. The expectation now comes fromJsonSerializer.Four texts that earlier fixes left behind
Worth separating out, because the pattern is mine:
contract-rulesstill carried word for word the sentence corrected inlimitations— the two sibling pages had been contradicting each other since.EMN0004's shipped description, written in the commit that scoped the rule to[Flags], gives a mechanism this repository's own parity tests disprove.EMN0005's message, corrected once already, was still wrong in another dimension: measured both ways round,Enum.GetNamesorder decides which member disappears, so moving the annotation to the second member reverses it and the message named the survivor as the loser.TryParseSingle's summary says it receives "a value with no comma in it"; the reorder that made a comma legal off[Flags]hands it commas.The rest
Two more with teeth: the pre-commit hook word-split its paths on a justification false in both directions — measured, it produced 4 paths for 3 files, 3 of them naming nothing on disk. And the smoke-test fixtures inherited this repository's own analyzers, because shadowing
Directory.Build.propsdoes not shadow the separate walk forDirectory.Build.targets— RS0016 appeared ten times in a fixture meant to be somebody else's project; now zero.Then: "read on every request" (measured: once per action, not five times for five requests); "
ModelBindingBehaviourTestsholds all of them" (it holds two of four); a dead\|unescape behind a comment about a markdown table neither page has ever been;EMN0001wrong about both halves of its own rationale (System.Text.Jsonaccepts a duplicate name silently, and nothing here picks a first declaration); "a contract without[Flags]accepts no combination" (it accepts one); a five-rule analyzer that ships six; three test projects that are four; and "no test can reachAddEnumMemberNameBinding()" — repeated in six places, whileEntryAssemblyScanTestsdoes exactly that and passes.Type of change
Testing
dotnet build -c Release— clean, warnings are errors heredotnet test -c Release— 963 passed, 0 failedtests/PackageSmokeTest/run.sh— run because the analyzers and the fixtures' MSBuild changed; "The published package behaves as documented."Also run:
tools/style/lint-layout.sh(nothing to report), its own test, andtools/commit-lint/lint-commit-message.shover all 19 commit messages. The hook fix and the generic-enum fix were each verified against a reproduction before and after.Public API
Two diagnostic message strings change; no symbol does.
Documentation
docs/updatedCHANGELOG.mdanddocs/for-users/CHANGELOG.fr.mdboth updatedThe changelog records the four a consumer meets — two diagnostic messages and two pages. The other fourteen are internal.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M5U2BZXpHQr7YcfNHVx9dA
Generated by Claude Code