Skip to content

Commit f80f036

Browse files
Smaug123claude
andcommitted
Use exactly the scanner's equality in schema validation
ToUpperInvariant keying is strictly coarser than OrdinalIgnoreCase (the equality the scanner matches keys with): "s" and "ſ" uppercase to the same string but are distinct keys, so the checked constructor falsely rejected schemas the scanner routes unambiguously. Group claims with StringComparer.OrdinalIgnoreCase instead. Also reject forms no token can ever address: an empty form (its token is the positional separator) and forms containing '=' (a --key=value token splits at its first '='), either of which could otherwise leave a required argument, or a whole union case, permanently unsatisfiable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent c40eb30 commit f80f036

2 files changed

Lines changed: 128 additions & 7 deletions

File tree

WoofWare.Myriad.Plugins.Test/TestArgParser/TestArgParserRuntime.fs

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1325,3 +1325,57 @@ module TestArgParserRuntime =
13251325

13261326
let config = Config.QuickThrowOnFailure.WithMaxTest 500
13271327
Check.One (config, Prop.forAll (Arb.fromGen cases) property)
1328+
1329+
[<Test>]
1330+
let ``Collision detection uses exactly the scanner's equality`` () =
1331+
// "s" and "ſ" (long s) uppercase to the same string, but OrdinalIgnoreCase — the equality
1332+
// the scanner matches keys with — considers them distinct. A coarser normalisation (e.g.
1333+
// ToUpperInvariant keying) would falsely reject this schema even though the scanner
1334+
// routes its tokens unambiguously.
1335+
let schema =
1336+
productSchema
1337+
[
1338+
leaf 0 "s" ErasedArity.One ErasedRequirement.Required
1339+
leaf 1 "ſ" ErasedArity.One ErasedRequirement.Required
1340+
]
1341+
None
1342+
1343+
// The scanner really does distinguish them.
1344+
scan schema [ "--s=1" ] |> shouldEqual [ occ 0 (Some "1") false "--s=1" ]
1345+
scan schema [ "--ſ=1" ] |> shouldEqual [ occ 1 (Some "1") false "--ſ=1" ]
1346+
1347+
WellFormedSchema.errors schema |> shouldEqual []
1348+
1349+
[<Test>]
1350+
let ``An empty form is rejected: its token would be the separator`` () =
1351+
let bad =
1352+
{ leaf 0 "a" ErasedArity.One ErasedRequirement.Required with
1353+
Forms = [ "" ]
1354+
}
1355+
1356+
WellFormedSchema.errors (productSchema [ bad ] None)
1357+
|> shouldEqual [ SchemaError.EmptyForm "argument id 0" ]
1358+
1359+
[<Test>]
1360+
let ``A form containing an equals sign is rejected: the scanner splits at the first equals`` () =
1361+
// A required argument under such a form would make its schema (or its union case)
1362+
// permanently unsatisfiable, while still passing every name-collision check.
1363+
let bad =
1364+
{ leaf 0 "a" ErasedArity.One ErasedRequirement.Required with
1365+
Forms = [ "foo=bar" ]
1366+
}
1367+
1368+
WellFormedSchema.errors (productSchema [ bad ] None)
1369+
|> shouldEqual [ SchemaError.FormContainsEquals ("argument id 0", "foo=bar") ]
1370+
1371+
let badSink =
1372+
{
1373+
Id = 99
1374+
Forms = [ "rest=stuff" ]
1375+
FlagLike = ErasedFlagLikeBehaviour.Reject
1376+
TypeDescription = "string"
1377+
Help = None
1378+
}
1379+
1380+
WellFormedSchema.errors (productSchema [ leaf 0 "a" ErasedArity.One ErasedRequirement.Required ] (Some badSink))
1381+
|> shouldEqual [ SchemaError.FormContainsEquals ("the positional-args sink", "rest=stuff") ]

WoofWare.Myriad.Plugins/ArgParserRuntime.fs

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -608,6 +608,13 @@ module internal ArgParserRuntime =
608608
| NoForms of leafId : int
609609
/// A leaf accepts `--no-` negation but is not boolean-like.
610610
| NegationOnNonBool of leafId : int
611+
/// A form is the empty string: its token would be `--`, which the scanner always
612+
/// treats as the positional separator, so the argument could never be addressed.
613+
| EmptyForm of claimant : string
614+
/// A form contains an equals sign: the scanner splits a `--key=value` token at its
615+
/// *first* `=`, so such a form can never match. An argument required under such a form
616+
/// makes the schema (or a union case of it) permanently unsatisfiable.
617+
| FormContainsEquals of claimant : string * form : string
611618
/// Two distinct claimants respond to the same `--token` under the scanner's
612619
/// case-insensitive matching (so e.g. forms `foo` and `FOO` collide, as do a literal
613620
/// form `no-foo` and the negated variant of a negatable `foo`). The token is reported
@@ -631,6 +638,13 @@ module internal ArgParserRuntime =
631638
| SchemaError.NoForms leafId -> sprintf "argument id %i has no names" leafId
632639
| SchemaError.NegationOnNonBool leafId ->
633640
sprintf "argument id %i accepts --no- negation but is not boolean-like" leafId
641+
| SchemaError.EmptyForm claimant ->
642+
sprintf "%s has an empty name: its token would be '--', which is the positional separator" claimant
643+
| SchemaError.FormContainsEquals (claimant, form) ->
644+
sprintf
645+
"%s has the name '%s', which contains '='; a --key=value token splits at its first '=', so this argument could never be addressed"
646+
claimant
647+
form
634648
| SchemaError.TokenCollision (token, claimants) ->
635649
sprintf
636650
"the token '%s' is claimed by: %s (argument names are matched case-insensitively)"
@@ -736,14 +750,66 @@ module internal ArgParserRuntime =
736750
)
737751
@ [ "--help", "the built-in help flag" ]
738752

753+
// Group under the scanner's own equality (OrdinalIgnoreCase), preserving
754+
// declaration order. This is deliberately not ToUpperInvariant keying, which is a
755+
// strictly coarser relation: e.g. "s" and "ſ" (long s) uppercase to the same string,
756+
// but the scanner considers them distinct, so they do not collide.
739757
let collisions =
740-
claims
741-
|> List.groupBy (fun (token, _) -> token.ToUpperInvariant ())
742-
|> List.choose (fun (_, group) ->
743-
match group with
744-
| []
745-
| [ _ ] -> None
746-
| (token, _) :: _ -> Some (SchemaError.TokenCollision (token, group |> List.map snd))
758+
let indexOf =
759+
System.Collections.Generic.Dictionary<string, int> (StringComparer.OrdinalIgnoreCase)
760+
761+
let buckets = ResizeArray<ResizeArray<string * string>> ()
762+
763+
for token, claimant in claims do
764+
match indexOf.TryGetValue token with
765+
| true, index -> buckets.[index].Add ((token, claimant))
766+
| false, _ ->
767+
indexOf.[token] <- buckets.Count
768+
let bucket = ResizeArray ()
769+
bucket.Add ((token, claimant))
770+
buckets.Add bucket
771+
772+
buckets
773+
|> Seq.choose (fun bucket ->
774+
if bucket.Count < 2 then
775+
None
776+
else
777+
let token, _ = bucket.[0]
778+
Some (SchemaError.TokenCollision (token, bucket |> Seq.map snd |> List.ofSeq))
779+
)
780+
|> List.ofSeq
781+
782+
// Forms which no token could ever address, because of how the scanner tokenises.
783+
let unaddressable =
784+
(schema.Leaves
785+
|> List.collect (fun leaf ->
786+
let claimant = sprintf "argument id %i" leaf.Id
787+
788+
leaf.Forms
789+
|> List.collect (fun form ->
790+
if form = "" then
791+
[ SchemaError.EmptyForm claimant ]
792+
elif form.Contains "=" then
793+
[ SchemaError.FormContainsEquals (claimant, form) ]
794+
else
795+
[]
796+
)
797+
))
798+
@ (
799+
match schema.Positional with
800+
| None -> []
801+
| Some positional ->
802+
let claimant = "the positional-args sink"
803+
804+
positional.Forms
805+
|> List.collect (fun form ->
806+
if form = "" then
807+
[ SchemaError.EmptyForm claimant ]
808+
elif form.Contains "=" then
809+
[ SchemaError.FormContainsEquals (claimant, form) ]
810+
else
811+
[]
812+
)
747813
)
748814

749815
duplicateLeafIds
@@ -753,6 +819,7 @@ module internal ArgParserRuntime =
753819
@ duplicateSumIds
754820
@ noForms
755821
@ negationOnNonBool
822+
@ unaddressable
756823
@ collisions
757824

758825
/// Check the invariants which the scanning and selection semantics rely on.

0 commit comments

Comments
 (0)