ArgParser: reject [<ArgumentNegateWithPrefix>] on positional fields - #607
Merged
Smaug123 merged 3 commits intoJul 30, 2026
Conversation
`hasNegateAttr` was computed inside the non-positional leaf branch only. The positional branch never looked at it and hardcoded `AcceptsNegation = false`, so the attribute was silently ignored. A positional field is not spelled at all -- it collects whatever carries no name -- so there is no name from which a --no- variant could be formed, whatever the field's type. That is a different complaint from the existing boolean-shape check, and gets its own message. The extraction moves above the positional split so both sides read one definition. Breaking; lands under the 11.0 bump carried earlier in this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A [<PositionalArgs>] field is addressable -- `--rest value` and `--rest=value` route to the sink, and an explicit [<ArgumentLongForm>] can add further spellings -- so claiming it "has no spelling to negate" contradicted the parser's own behaviour. The rejection stands; the reason is that those keyed forms are value-taking routing keys, so a sink which accumulates values has no boolean for a `--no-` form to invert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…are not read (#608) * ArgParser: reject [<ParseExact>] and [<InvariantCulture>] where unread Both are read in exactly one place: the TimeSpan arm of createParseFunction. On any other type they were dropped -- and for [<ParseExact>] that is worse than silence, because `helpText` advertises the format unconditionally, so the generated --help promised a format the generated parser did not honour. Checked against the type actually handed to the parser rather than the declared field type, exactly as checkSeparatorAttributesPlacement is, so `TimeSpan option` and `TimeSpan list` keep working. Departure from the planned scope: the plan called for rejecting maps outright, on the belief that only the value type sees the attribute. It does not -- the map branch hands the field's attributes to its key parser as well -- so `Map<string, TimeSpan>` and `Map<TimeSpan, string>` both genuinely honour the attribute today, and rejecting them would have removed working behaviour. A map is therefore rejected only when neither component is a TimeSpan. Also noted while testing: System.DateTime, the type an author would most likely want a parse format for, is not supported by the generator at all and fails earlier with its own message. There is no lying help to fix there; the lie is on `string`, `int` and friends. Breaking; lands under the 11.0 bump carried earlier in this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Document ArgParser attribute placement, and the 11.0 breaking changes (#609) Attribute doc comments now state where each attribute is read and where it is rejected. Two were actively misleading: - ArgumentFlagAttribute said it went on "a field of a two-case no-data discriminated union". It goes on the union's *cases*; the loose wording invited precisely the mistake now rejected. - ParseExactAttribute and InvariantCultureAttribute both wrote the attribute as [<ArgumentParseExact>], which does not exist, and neither said it is honoured on TimeSpan alone. README gains an "attribute placement at a structural boundary" section, and no longer claims [<ParseExact>] works on "TimeSpan and friends". CHANGELOG records 11.0 as one entry covering the whole rejection stack, plus the backtick fix which landed after 10.7.2 and was never published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Smaug123
added a commit
that referenced
this pull request
Jul 30, 2026
* ArgParser: reject leaf-only attributes on structural fields A field whose type is another argument record, or a union of alternative argument sets, contributes that type's whole set of arguments rather than one. [<PositionalArgs>], [<ParseExact>], [<InvariantCulture>] and [<ArgumentNegateWithPrefix>] each describe how a single argument is collected, spelled or read, so there is nothing here for them to act on -- and the structural branches take over before the leaf machinery which reads them ever runs, so each was computed and then dropped on the floor. [<ArgumentHelpText>] is deliberately not in the list: on a structural field it introduces the group of arguments the field contributes. Breaking; lands under the 11.0 bump carried by the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ArgParser: reject [<ArgumentNegateWithPrefix>] on positional fields (#607) * ArgParser: reject [<ArgumentNegateWithPrefix>] on positional fields `hasNegateAttr` was computed inside the non-positional leaf branch only. The positional branch never looked at it and hardcoded `AcceptsNegation = false`, so the attribute was silently ignored. A positional field is not spelled at all -- it collects whatever carries no name -- so there is no name from which a --no- variant could be formed, whatever the field's type. That is a different complaint from the existing boolean-shape check, and gets its own message. The extraction moves above the positional split so both sides read one definition. Breaking; lands under the 11.0 bump carried earlier in this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct the rejection message: a positional sink does have keyed forms A [<PositionalArgs>] field is addressable -- `--rest value` and `--rest=value` route to the sink, and an explicit [<ArgumentLongForm>] can add further spellings -- so claiming it "has no spelling to negate" contradicted the parser's own behaviour. The rejection stands; the reason is that those keyed forms are value-taking routing keys, so a sink which accumulates values has no boolean for a `--no-` form to invert. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ArgParser: reject [<ParseExact>] and [<InvariantCulture>] where they are not read (#608) * ArgParser: reject [<ParseExact>] and [<InvariantCulture>] where unread Both are read in exactly one place: the TimeSpan arm of createParseFunction. On any other type they were dropped -- and for [<ParseExact>] that is worse than silence, because `helpText` advertises the format unconditionally, so the generated --help promised a format the generated parser did not honour. Checked against the type actually handed to the parser rather than the declared field type, exactly as checkSeparatorAttributesPlacement is, so `TimeSpan option` and `TimeSpan list` keep working. Departure from the planned scope: the plan called for rejecting maps outright, on the belief that only the value type sees the attribute. It does not -- the map branch hands the field's attributes to its key parser as well -- so `Map<string, TimeSpan>` and `Map<TimeSpan, string>` both genuinely honour the attribute today, and rejecting them would have removed working behaviour. A map is therefore rejected only when neither component is a TimeSpan. Also noted while testing: System.DateTime, the type an author would most likely want a parse format for, is not supported by the generator at all and fails earlier with its own message. There is no lying help to fix there; the lie is on `string`, `int` and friends. Breaking; lands under the 11.0 bump carried earlier in this stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Document ArgParser attribute placement, and the 11.0 breaking changes (#609) Attribute doc comments now state where each attribute is read and where it is rejected. Two were actively misleading: - ArgumentFlagAttribute said it went on "a field of a two-case no-data discriminated union". It goes on the union's *cases*; the loose wording invited precisely the mistake now rejected. - ParseExactAttribute and InvariantCultureAttribute both wrote the attribute as [<ArgumentParseExact>], which does not exist, and neither said it is honoured on TimeSpan alone. README gains an "attribute placement at a structural boundary" section, and no longer claims [<ParseExact>] works on "TimeSpan and friends". CHANGELOG records 11.0 as one entry covering the whole rejection stack, plus the backtick fix which landed after 10.7.2 and was never published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Stage 3 of the attribute-rejection stack; stacked on #606.
hasNegateAttrwas computed inside the non-positional leaf branch only. The positional branch never looked at it and hardcodedAcceptsNegation = false, so[<PositionalArgs>] [<ArgumentNegateWithPrefix>] Foo : bool listsilently ignored the negation.A positional field is not spelled at all — it collects whatever carries no name — so there is no name from which a
--no-variant could be formed, whatever the field's type. That is a different complaint from the existing boolean-shape check (Negation is only available on boolean-like fields), which is why it gets its own message rather than being folded into it.The extraction of
hasNegateAttrmoves above the positional split so both sides read one definition.Tests written first and observed failing: negation on a positional
string list, on a positionalbool list(the shape which most invites the mistake, since every other negation check passes it), and under the longArgumentNegateWithPrefixAttributespelling — plus a regression that a positional field and a negated boolean field coexist happily, since neither attribute is at fault alone.Breaking; lands under the
11.0bump carried by #605.🤖 Generated with Claude Code