ArgParser: reject leaf-only attributes on structural fields - #606
Merged
Smaug123 merged 2 commits intoJul 30, 2026
Merged
Conversation
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>
…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>
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 2 of the attribute-rejection stack; stacked on #605.
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 there 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, which is what the group-header work made it mean. There's an explicit regression test for that.Follows
rejectLongFormAttributeexactly, which is the same argument for the same reason; called from both structural branches alongside it.Tests written first and observed failing: each of the four attributes on a nested-record field and on a structural-union field, each under its long
...Attributespelling,[<PositionalArgs true>]with its explicit constructor argument, and regressions that all four remain fine on the leaf fields the messages point at.Breaking; lands under the
11.0bump carried by #605.🤖 Generated with Claude Code