ArgParser: reject [<ArgumentFlag>] on record fields - #605
Merged
Conversation
[<ArgumentFlag>] is read only from a union's cases; on a record field nothing read it at all, so it silently did nothing. The attribute's own documentation invited the mistake by describing it as going on "a field". Checked above the leaf/structural dispatch, so it fires for structural, positional and ordinary leaf fields alike, rather than only where the leaf machinery happens to run. Breaking: sources which previously built now fail to generate. Bumps to 11.0; the remaining rejections in this stack land under the same bump. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced 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 1 of the attribute-rejection stack. Each stage is its own PR for review, but the stack lands in
mainas one collection of breaking changes.[<ArgumentFlag>]is read only from a union's cases, where it says which case meanstrueand which meansfalse. Placed on a record field, nothing read it at all — it silently did nothing. The attribute's own doc comment invited exactly that mistake by describing it as going on "a field".The check sits above the leaf/structural dispatch, alongside the existing default-attribute check, so it fires for structural, positional and ordinary leaf fields alike rather than only where the leaf machinery happens to run.
Breaking: sources which previously built now fail to generate. This carries the
11.0bump for the whole stack; the later rejection PRs land under it.Tests written first and observed failing, in
TestArgParserRejection.fs: the attribute on a bool leaf, on a flag-union-typed field (the likeliest mistake), on a nested-record field, on a structural-union field, on a positional field, and under its longArgumentFlagAttributespelling — plus a regression that the correct placement, on the union's cases, is still accepted.🤖 Generated with Claude Code