Repository navigation
fix(core): refuse an explicit required arity with a default value - #164
Conversation
Follow-ups from the PR #63 review panel, deferred from 0.11: - An explicit Arity of ExactlyOne or OneOrMore requires a value, and the binder enforces it before it would fall back to the parameter's default, so that default could never apply. Map now refuses the combination on a handler parameter, naming the parameter and the optional arity to use instead (ZeroOrOne for ExactlyOne, ZeroOrMore for OneOrMore). This is a breaking change for an app that declares both; docs/parameter-system.md carries a "Changed in 0.12" note. An options-group property's initializer has the same problem but cannot be detected, which the docs and ReplOptionAttribute.Arity now say. - A scalar enum option repeated with the same value under an explicit ZeroOrMore arity passed the enum conflict check, which exempted equal duplicates, and then failed binding with "does not support repeated occurrences". Whether a repeat used the same spelling, the enum flag alias, another casing or the numeric value, the message depended on the spelling rather than the value. The exemption is gone, and every repeat of a scalar enum now reads "accepts a single enum value per invocation". That also removes the case comparison the check needed, and the global case sensitivity it took. - "CaseSensitivity ?? global" was written out at fourteen sites, three of them behind a conditional. It is now EffectiveCaseSensitivity(global) on OptionSchemaEntry and OptionSchemaParameter. - InvocationOptionParser.ResolveParameterArity, a pass-through to OptionSchema.ResolveParameterArity, is inlined at its one caller. - CanConsumePositionalForProperty had two identical branches and is inlined as the comparison it was. The third item of the issue, DI resolution ahead of an explicit lower bound, is now the documented order (docs/commands.md) and is left as is. TDD: the arity test (an option with OneOrMore, one with ExactlyOne, an argument with OneOrMore) and the equal-duplicate enum row were red before the fix: no exception on Map, and "does not support repeated occurrences". They now also pin the parameter name and the suggested arity. A ZeroOrOne option with a default value guards the side the refusal must not reach. The existing ExactlyOne absence test declared "= null" on a nullable parameter; it drops the default and keeps what it guards. The case-parity enum test becomes the casing row of the repeated-enum test. Closes #66
|
@codex review |
|
@autocarl please review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Reviewed exact head 4cde862 and integration with current target d65f6eb. No concrete introduced P0-P2 findings.
Validation:
- Release build of src/Repl.slnx: 0 warnings / 0 errors. Six repository test suites on exact head: 2,605 passed / 3 skipped / 0 failed.
- Five targeted regression rows fail against the previous schema builder/parser with successful compilation: three previously accepted required-arity/default declarations and two enum diagnostic changes. Exact head sources restored and rebuilt afterward; checkout clean. The enum rows already refused execution previously; these validate a diagnostic change, not newly rejected input.
- Additional public-API probes pass for C# and DefaultParameterValue metadata, inferred/defaulted options, explicit ZeroOrOne and ZeroOrMore, DI-before-lower-bound precedence, required absence, group property initializers and scalar enum repeats.
- Prospective merge with target: conflict-free, tree 9c4b8d010bee53fb568e50c702db41aa0634a2e9, identical to GitHub PR merge-ref tree with the expected parents. Release solution build clean; same six suites: 2,617 passed / 3 skipped / 0 failed. Public arity probes and the previous raw MCP confidentiality/concurrent-log probes also pass on this combined tree.
- Applicable CI Linux/Windows/macOS, packaging, protocol Inspector and smoke checks are successful. External review skipped for credits is not counted as independent evidence.
Independent static review found no additional issue in the changed schema/arity/parser surface. Its two other alerts were rejected after attribution checks: Telnet encoding and MCP diagnostic differences came from fixes #162/#163 on newer main, not changes introduced by this PR. Neither file is in the merge-base-to-head patch, and the combined tree preserves both fixes byte-for-byte; their regression tests pass.
The breaking Map-time refusal is deliberate, bounded to explicit required arity plus a handler default, and migration guidance is present. Inferred arity, optional explicit arity and documented DI fallback stay intact.
Approved. No merge or product modifications performed.
Closes #66.
Six low-severity follow-ups from the PR #63 review panel, deferred past 0.11. Before changing anything, I checked each one against the current
main:mainOneOrMore/ExactlyOnewith a C# defaultMaprefuses it (breaking)docs/commands.mdCaseSensitivity ?? globalinlinedEffectiveCaseSensitivity(global)ResolveParameterAritypass-throughCanConsumePositionalidentical branchesBreaking: explicit required arity with a default value
The binder enforces an explicit
ExactlyOneorOneOrMorebefore it would fall back to the parameter's default, so that default could never apply.Mapnow throws, naming the parameter and the optional arity to use instead, for example:docs/parameter-system.mdcarries a "Changed in 0.12" note.Mapcannot detect it becausePropertyInfodoes not expose an initializer. The docs andReplOptionAttribute.Aritysay so.Repeated scalar enum
Under an explicit
ZeroOrMore, the message depended on how the repeat was spelled, not on the value. I measured both versions with--syllable Gafollowed by a second entry:--syllable Ga,--ga(flag alias)does not support repeated occurrencesaccepts a single enum value per invocation--syllable ga,--syllable 1,--syllable Bureceived multiple enum valuesaccepts a single enum value per invocationAll of these already exited with code 2; only the message changes. A repeat that names the same value stays refused, as for every other scalar.
Test plan
When_AnExplicitLowerBoundArityHasADefaultValue_Then_MapFails, and the same-value row of the repeated-enum test.ZeroOrOneoption with a default value guards the side the refusal must not reach.ExactlyOneabsence test drops its= nulland keeps what it guards. The case-parity enum test becomes a row of the repeated-enum test.private, and the two records only gain a method. No friend assembly calls them (Repl.Remote.DefaultsusesHandlerArgumentBinder.TryGetCollectionElementType, unchanged).