ArgParser: reject leaf-only attributes on structural fields (#606)

* 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>

Author: Smaug123

CHANGELOG.md

@@ -1,5 +1,14 @@
 Notable changes are recorded here.
 
+# WoofWare.Myriad.Plugins 11.0.1
+
+Breaking change: `ArgParserGenerator` now rejects, at generation time, several attribute placements which it previously accepted and then silently ignored.
+
+* `[<ArgumentFlag>]` on a record field. It belongs on the two cases of a flag discriminated union, where it says which case means `true` and which means `false`; a field is not where that question can be answered.
+* `[<PositionalArgs>]`, `[<ParseExact>]`, `[<InvariantCulture>]` or `[<ArgumentNegateWithPrefix>]` on a field whose type is another argument record, or a union of alternative argument sets. Such a field contributes a whole set of arguments rather than one, so there is no single argument for these to collect into, spell or negate. `[<ArgumentHelpText>]` is deliberately still accepted there: it heads the group of arguments the field contributes.
+* `[<ArgumentNegateWithPrefix>]` on a `[<PositionalArgs>]` field. A positional field is a sink which accumulates values; its keyed spellings take a value rather than setting anything, so there is no boolean there for a `--no-` form to invert.
+* `[<ParseExact>]` or `[<InvariantCulture>]` on anything but a `System.TimeSpan`. These are read only by the `TimeSpan` parser. They continue to reach through the shapes which wrap a value — `TimeSpan option`, `TimeSpan list`, `Choice<TimeSpan, TimeSpan>`, and a `Map` either of whose halves is a `TimeSpan` — and are rejected only where no `TimeSpan` is reached at all. Future work may expand the range of types these attributes affect.
+
 # WoofWare.Myriad.Plugins 10.7.2
 
 `ArgParserGenerator` now reads `[<ArgumentHelpText>]` from a nested argument record or union of alternative argument sets, and not only from the `[<ArgParser>]`-tagged root, and from a discriminated union's case (and that case's payload record).

README.md

@@ -243,7 +243,8 @@ and you get back respectively these objects:
 }
 ```
 
-You can control `TimeSpan` and friends with the `[<InvariantCulture>]` and `[<ParseExact @"hh\:mm\:ss">]` attributes.
+You can control how a `System.TimeSpan` is read with the `[<InvariantCulture>]` and `[<ParseExact @"hh\:mm\:ss">]` attributes.
+These are honoured on `TimeSpan` only — reaching through the shapes which wrap it, so `TimeSpan option`, `TimeSpan list` and a `Map` with a `TimeSpan` half are all covered — and are rejected on any other type, where nothing would read them.
 
 You can generate extension methods for the type, instead of a module with the type's name, using `[<ArgParser (* isExtensionMethod = *) true>]`.
 
@@ -379,6 +380,18 @@ Discriminated unions compose with each other and with records, hopefully as you
 We will fail at build time to generate a parser if you have several DU cases which contain fields of the same name, though, because in general resolving the parse in that case is NP-hard.
 (An upcoming piece of work will hopefully relax this restriction.)
 
+#### Attribute placement at a structural boundary
+
+A field whose type is another argument record, or a union of alternative argument sets, is *structural*: it contributes that type's whole set of arguments rather than being one argument itself.
+Most attributes describe single arguments, so are rejected at build time when applied to a structural field.
+
+Two attributes do apply to structural fields, because they are specifically about the group:
+
+* `[<ArgumentHelpText>]` describes the whole group of arguments the field contributes, and appears on the header line introducing it.
+* `[<ArgumentPrefix>]` namespaces every argument in the subtree; conversely, it is rejected on a leaf, which has no subtree.
+
+`[<ArgumentFlag>]` is not a field attribute at all: it goes on the two cases of a flag discriminated union, and is rejected on a record field.
+
 ### What's the point?
 
 I got fed up of waiting for us to find time to rewrite the in-house one at work.

WoofWare.Myriad.Plugins.Attributes/ArgParserAttributes.fs

@@ -31,6 +31,18 @@ type ArgParserAttribute (isExtensionMethod : bool) =
 /// tell you whether each arg came before or after a standalone `--` separator.
 /// For example, `MyApp foo bar -- baz` with PositionalArgs of `Choice<string, string>`
 /// would yield `Choice1Of2 foo, Choice1Of2 bar, Choice2Of2 baz`.
+///
+/// This attribute applies to an argument *leaf*. It is rejected on a field whose type is another
+/// [<ArgParser>]-schema record or a discriminated union of alternative argument sets: such a field
+/// contributes a whole set of arguments rather than being a place values can be collected into, so
+/// put the attribute on the leaf field inside that type which should do the collecting.
+///
+/// A positional field is a sink which accumulates values. It is addressable: `--foo value` and
+/// `--foo=value` route to it, as does any explicit [<ArgumentLongForm>]. But those keyed forms
+/// take a value rather than setting anything, so the field is rejected in combination with
+/// [<ArgumentNegateWithPrefix>] (there is no boolean for a `--no-` form to invert) and with
+/// [<ArgumentPrefix>] (there is no subtree to namespace), and it may not carry a default
+/// (positional args are collected, not defaulted).
 type PositionalArgsAttribute (includeFlagLike : bool) =
     inherit Attribute ()
 
@@ -102,25 +114,42 @@ type ArgumentDefaultValueAttribute (defaultValue : obj) =
 type ArgumentHelpTextAttribute (helpText : string) =
     inherit Attribute ()
 
-/// Attribute indicating that this field should be parsed with a ParseExact method on its type.
-/// For example, on a TimeSpan field, with [<ArgumentParseExact @"hh\:mm\:ss">], we will call
-/// `TimeSpan.ParseExact (s, @"hh\:mm\:ss", CultureInfo.CurrentCulture).
+/// Attribute giving the format in which this field's value is written.
+/// For example, on a TimeSpan field, with [<ParseExact @"hh\:mm\:ss">], we will call
+/// `TimeSpan.ParseExact (s, @"hh\:mm\:ss", CultureInfo.CurrentCulture)`.
+///
+/// This attribute is honoured on `System.TimeSpan` only, and is rejected anywhere else: no other
+/// type's parser reads it, so it would otherwise be dropped in silence while the generated help
+/// text went on advertising a format the parser did not apply.
+///
+/// It reaches through the shapes which wrap a value, since those parse their components with it:
+/// `TimeSpan option`, `TimeSpan list` and `Choice<TimeSpan, TimeSpan>` are all honoured, as is a
+/// `Map` either of whose halves is a `TimeSpan` (in which case the format describes each `TimeSpan`
+/// half; a map with no `TimeSpan` at all is rejected).
 type ParseExactAttribute (format : string) =
     inherit Attribute ()
 
 /// Attribute indicating that this field should be parsed in the invariant culture, rather than the
 /// default current culture.
-/// For example, on a TimeSpan field, with [<InvariantCulture>] and [<ArgumentParseExact @"hh\:mm\:ss">], we will call
-/// `TimeSpan.ParseExact (s, @"hh\:mm\:ss", CultureInfo.InvariantCulture).
+/// For example, on a TimeSpan field, with [<InvariantCulture>] and [<ParseExact @"hh\:mm\:ss">], we will call
+/// `TimeSpan.ParseExact (s, @"hh\:mm\:ss", CultureInfo.InvariantCulture)`.
+///
+/// As with [<ParseExact>], this is honoured on `System.TimeSpan` only -- reaching through option,
+/// list, choice and map in the same way -- and is rejected anywhere else, where nothing would read
+/// it.
 type InvariantCultureAttribute () =
     inherit Attribute ()
 
-/// Attribute placed on a field of a two-case no-data discriminated union, indicating that this is "basically a bool".
+/// Attribute placed on each case of a two-case no-data discriminated union, indicating that the
+/// union is "basically a bool".
 /// For example: `type DryRun = | [<ArgumentFlag true>] Dry | [<ArgumentFlag false>] Wet`
 /// A record with `{ DryRun : DryRun }` will then be parsed like `{ DryRun : bool }` (so the user supplies `--dry-run`),
 /// but that you get this strongly-typed value directly in the code (so you `match args.DryRun with | DryRun.Dry ...`).
 ///
 /// You must put this attribute on both cases of the discriminated union, with opposite values in each case.
+///
+/// It belongs on the union's *cases*, and is rejected on a record field: a field is not where the
+/// question "which case means true?" can be answered, and nothing read the attribute there.
 type ArgumentFlagAttribute (flagValue : bool) =
     inherit Attribute ()
 
@@ -132,6 +161,12 @@ type ArgumentFlagAttribute (flagValue : bool) =
 /// You can place this argument multiple times.
 ///
 /// Omit the initial `--` that you expect the user to type.
+///
+/// This attribute applies to an argument *leaf*. It is rejected on a field whose type is another
+/// [<ArgParser>]-schema record or a discriminated union of alternative argument sets: such a field
+/// contributes a whole set of arguments, each named by its own field, so there is no single
+/// argument here to rename. Put the attribute on the field you mean to rename, or use
+/// [<ArgumentPrefix>] to rename the whole subtree at once.
 [<AttributeUsage(AttributeTargets.Field, AllowMultiple = true)>]
 type ArgumentLongForm (s : string) =
     inherit Attribute ()
@@ -215,6 +250,12 @@ type ArgumentMapEntrySeparatorAttribute (separator : char) =
 ///   --no-field-name=false sets to the [<ArgumentFlag true>] case
 ///
 /// This attribute can only be applied to bool fields or flag DU fields (two-case DUs with [<ArgumentFlag>]).
+///
+/// It applies to an argument *leaf*, and to a leaf which has a name. So it is also rejected on a
+/// field whose type is another [<ArgParser>]-schema record or a discriminated union of alternative
+/// argument sets (which contributes a whole set of arguments, none of them singled out for
+/// negation), and on a [<PositionalArgs>] field (which accumulates values, so although it is
+/// addressable, there is no boolean there for a `--no-` form to invert).
 [<AttributeUsage(AttributeTargets.Field, AllowMultiple = false)>]
 type ArgumentNegateWithPrefixAttribute () =
     inherit Attribute ()