From ef88bb4e23a356b7c46f905e948dfed2e0f75dff Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 14:22:31 -0400 Subject: [PATCH 1/7] fix(parameters): make nullable-enum attribute overrides settable (CS0655 dead code) Nullable enums are not legal attribute named arguments, so four override properties were impossible to set from user code and always resolved to null: - ReplOptionAttribute.CaseSensitivity - ReplOptionAttribute.Arity - ReplEnumFlagAttribute.CaseSensitivity - ReplValueAliasAttribute.CaseSensitivity Each now exposes a non-nullable public property backed by a nullable field, with the unset state surfaced through internal *Override properties consumed by OptionSchemaBuilder. Behavior tests exercise each override end-to-end (RED observed as CS0655 before the fix). Fixes #57 --- .../Internal/Options/OptionSchemaBuilder.cs | 28 +++---- .../Attributes/ReplEnumFlagAttribute.cs | 17 +++- .../Attributes/ReplOptionAttribute.cs | 30 ++++++- .../Attributes/ReplValueAliasAttribute.cs | 17 +++- .../Given_OptionAttributeOverrides.cs | 82 +++++++++++++++++++ 5 files changed, 156 insertions(+), 18 deletions(-) create mode 100644 src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs diff --git a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs index bea2252b..83be3d24 100644 --- a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs +++ b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs @@ -119,7 +119,7 @@ private static void AppendParameterSchemaEntries( parameter.Name!, parameter.ParameterType, mode, - CaseSensitivity: optionAttribute?.CaseSensitivity); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride); if (mode == ReplParameterMode.ArgumentOnly) { return; @@ -133,7 +133,7 @@ private static void AppendParameterSchemaEntries( parameter.Name!, tokenKind, arity, - CaseSensitivity: optionAttribute?.CaseSensitivity)); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride)); AppendOptionAliases(parameter, tokenKind, arity, optionAttribute, entries); AppendReverseAliases(parameter, optionAttribute, entries); AppendValueAliases(parameter, optionAttribute, entries); @@ -168,7 +168,7 @@ private static void AppendOptionAliases( parameter.Name!, tokenKind, arity, - CaseSensitivity: optionAttribute?.CaseSensitivity)); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride)); } } @@ -185,7 +185,7 @@ private static void AppendReverseAliases( parameter.Name!, OptionSchemaTokenKind.ReverseFlag, ReplArity.ZeroOrOne, - CaseSensitivity: optionAttribute?.CaseSensitivity, + CaseSensitivity: optionAttribute?.CaseSensitivityOverride, InjectedValue: "false")); } } @@ -203,14 +203,14 @@ private static void AppendValueAliases( parameter.Name!, OptionSchemaTokenKind.ValueAlias, ReplArity.ZeroOrOne, - CaseSensitivity: valueAlias.CaseSensitivity ?? optionAttribute?.CaseSensitivity, + CaseSensitivity: valueAlias.CaseSensitivityOverride ?? optionAttribute?.CaseSensitivityOverride, InjectedValue: valueAlias.Value)); } } private static ReplArity ResolveArity(ParameterInfo parameter, ReplOptionAttribute? optionAttribute) { - if (optionAttribute?.Arity is { } explicitArity) + if (optionAttribute?.ArityOverride is { } explicitArity) { return explicitArity; } @@ -293,7 +293,7 @@ private static void AppendEnumAliases( parameter.Name!, OptionSchemaTokenKind.EnumAlias, ReplArity.ZeroOrOne, - CaseSensitivity: enumFlag.CaseSensitivity ?? optionAttribute?.CaseSensitivity, + CaseSensitivity: enumFlag.CaseSensitivityOverride ?? optionAttribute?.CaseSensitivityOverride, InjectedValue: field.Name)); } } @@ -412,7 +412,7 @@ private static void AppendPropertySchemaEntries( property.Name, property.PropertyType, mode, - CaseSensitivity: optionAttribute?.CaseSensitivity); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride); if (mode != ReplParameterMode.OptionOnly) { positionalPropertyNames.Add(property.Name); @@ -430,7 +430,7 @@ private static void AppendPropertySchemaEntries( property.Name, tokenKind, arity, - CaseSensitivity: optionAttribute?.CaseSensitivity)); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride)); AppendPropertyOptionAliases(property.Name, tokenKind, arity, optionAttribute, entries); AppendPropertyReverseAliases(property.Name, optionAttribute, entries); AppendPropertyValueAliases(property, optionAttribute, entries); @@ -439,7 +439,7 @@ private static void AppendPropertySchemaEntries( private static ReplArity ResolvePropertyArity(Type propertyType, ReplOptionAttribute? optionAttribute) { - if (optionAttribute?.Arity is { } explicitArity) + if (optionAttribute?.ArityOverride is { } explicitArity) { return explicitArity; } @@ -472,7 +472,7 @@ private static void AppendPropertyOptionAliases( propertyName, tokenKind, arity, - CaseSensitivity: optionAttribute?.CaseSensitivity)); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride)); } } @@ -489,7 +489,7 @@ private static void AppendPropertyReverseAliases( propertyName, OptionSchemaTokenKind.ReverseFlag, ReplArity.ZeroOrOne, - CaseSensitivity: optionAttribute?.CaseSensitivity, + CaseSensitivity: optionAttribute?.CaseSensitivityOverride, InjectedValue: "false")); } } @@ -507,7 +507,7 @@ private static void AppendPropertyValueAliases( property.Name, OptionSchemaTokenKind.ValueAlias, ReplArity.ZeroOrOne, - CaseSensitivity: valueAlias.CaseSensitivity ?? optionAttribute?.CaseSensitivity, + CaseSensitivity: valueAlias.CaseSensitivityOverride ?? optionAttribute?.CaseSensitivityOverride, InjectedValue: valueAlias.Value)); } } @@ -541,7 +541,7 @@ private static void AppendPropertyEnumAliases( property.Name, OptionSchemaTokenKind.EnumAlias, ReplArity.ZeroOrOne, - CaseSensitivity: enumFlag.CaseSensitivity ?? optionAttribute?.CaseSensitivity, + CaseSensitivity: enumFlag.CaseSensitivityOverride ?? optionAttribute?.CaseSensitivityOverride, InjectedValue: field.Name)); } } diff --git a/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs index 5c5f7165..ecad1815 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs @@ -20,8 +20,23 @@ public ReplEnumFlagAttribute(params string[] aliases) /// public string[] Aliases { get; } + // Nullable enums are not legal attribute named arguments (CS0655), so the optional + // override exposes a non-nullable property and tracks the unset state in a nullable + // backing field surfaced through the internal CaseSensitivityOverride property. + private ReplCaseSensitivity? _caseSensitivity; + /// /// Optional case-sensitivity override for these aliases. + /// Only an explicit assignment overrides the global parsing default. + /// + public ReplCaseSensitivity CaseSensitivity + { + get => _caseSensitivity ?? default; + set => _caseSensitivity = value; + } + + /// + /// Explicit case-sensitivity override, or null to inherit the global default. /// - public ReplCaseSensitivity? CaseSensitivity { get; set; } + internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; } diff --git a/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs index 3480496c..fc352470 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs @@ -26,13 +26,39 @@ public sealed class ReplOptionAttribute : Attribute /// public ReplParameterMode Mode { get; set; } = ReplParameterMode.OptionAndPositional; + // Nullable enums are not legal attribute named arguments (CS0655), so the optional + // overrides expose a non-nullable property and track the unset state in a nullable + // backing field surfaced through the internal *Override properties. + private ReplCaseSensitivity? _caseSensitivity; + private ReplArity? _arity; + /// /// Optional case-sensitivity override for this option. + /// Only an explicit assignment overrides the global parsing default. + /// + public ReplCaseSensitivity CaseSensitivity + { + get => _caseSensitivity ?? default; + set => _caseSensitivity = value; + } + + /// + /// Explicit case-sensitivity override, or null to inherit the global default. /// - public ReplCaseSensitivity? CaseSensitivity { get; set; } + internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; /// /// Optional arity override. + /// Only an explicit assignment overrides the arity inferred from the parameter shape. + /// + public ReplArity Arity + { + get => _arity ?? default; + set => _arity = value; + } + + /// + /// Explicit arity override, or null to use the arity inferred from the parameter shape. /// - public ReplArity? Arity { get; set; } + internal ReplArity? ArityOverride => _arity; } diff --git a/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs index 0737bb72..63ddb3c9 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs @@ -29,8 +29,23 @@ public ReplValueAliasAttribute(string token, string value) /// public string Value { get; } + // Nullable enums are not legal attribute named arguments (CS0655), so the optional + // override exposes a non-nullable property and tracks the unset state in a nullable + // backing field surfaced through the internal CaseSensitivityOverride property. + private ReplCaseSensitivity? _caseSensitivity; + /// /// Optional case-sensitivity override for this alias. + /// Only an explicit assignment overrides the global parsing default. + /// + public ReplCaseSensitivity CaseSensitivity + { + get => _caseSensitivity ?? default; + set => _caseSensitivity = value; + } + + /// + /// Explicit case-sensitivity override, or null to inherit the global default. /// - public ReplCaseSensitivity? CaseSensitivity { get; set; } + internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; } diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs new file mode 100644 index 00000000..d335275c --- /dev/null +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -0,0 +1,82 @@ +namespace Repl.IntegrationTests; + +[TestClass] +[DoNotParallelize] +public sealed class Given_OptionAttributeOverrides +{ + private enum ShadockSyllable + { + None = 0, + + [ReplEnumFlag("--ga", CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] + Ga = 1, + + [ReplEnumFlag("--bu")] + Bu = 2, + } + + [TestMethod] + [Description("Regression guard for issue #57: [ReplOption(CaseSensitivity = ...)] must be a legal attribute argument (a nullable enum triggers CS0655, making the override dead code) and the per-option override must accept casing variants while the global default stays case-sensitive.")] + public void When_OptionCaseSensitivityOverriddenViaAttribute_Then_CasingVariantIsAccepted() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] string text) => text); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--Text", "bib overalls", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("bib overalls"); + } + + [TestMethod] + [Description("Regression guard for issue #57 (expanded): [ReplOption(Arity = ...)] must be a legal attribute argument. A collection parameter naturally allows repetition (ZeroOrMore); the explicit ZeroOrOne override must be honored so a repeated option is rejected at parse time.")] + public void When_ArityOverriddenViaAttributeToZeroOrOne_Then_RepeatedOptionIsRejected() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.ZeroOrOne)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--items", "ga", "--items", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("accepts at most one value"); + } + + [TestMethod] + [Description("Regression guard for issue #57 (expanded): [ReplValueAlias(..., CaseSensitivity = ...)] must be a legal attribute argument so an alias token matched ignoring case still injects its configured value.")] + public void When_ValueAliasCaseSensitivityOverriddenViaAttribute_Then_CasingVariantInjectsValue() + { + var sut = ReplApp.Create(); + sut.Map("wear", ([ReplValueAlias("--denim", "bib overalls", CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] string outfit = "none") => outfit); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["wear", "--DENIM", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("bib overalls"); + } + + [TestMethod] + [Description("Regression guard for issue #57 (expanded): [ReplEnumFlag(..., CaseSensitivity = ...)] must be a legal attribute argument so an enum-flag alias matched ignoring case binds the enum member while other members stay case-sensitive.")] + public void When_EnumFlagCaseSensitivityOverriddenViaAttribute_Then_CasingVariantBindsEnumMember() + { + var sut = ReplApp.Create(); + sut.Map("say", (ShadockSyllable syllable = ShadockSyllable.None) => syllable.ToString()); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["say", "--GA", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("Ga"); + } + + [TestMethod] + [Description("Guards the override boundary: a sibling enum-flag alias without a case-sensitivity override must keep the global case-sensitive default, so per-member overrides stay scoped to their own aliases.")] + public void When_EnumFlagWithoutOverrideAndCasingDiffers_Then_TokenIsRejected() + { + var sut = ReplApp.Create(); + sut.Map("say", (ShadockSyllable syllable = ShadockSyllable.None) => syllable.ToString()); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["say", "--BU", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("--BU"); + } +} From 851ddf2194bf21af09b6ee328c94085c5f959e53 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 14:29:25 -0400 Subject: [PATCH 2/7] test(parameters): guard ambiguous-token rejection reachable via case override MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A token matching both a case-insensitive alias and another option's case-sensitive canonical token — reachable now that the per-option case-sensitivity override is settable — is rejected as 'Ambiguous option' at parse time. Documents the behavior flagged during the #55 review as previously unreachable. --- .../Given_OptionAttributeOverrides.cs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index d335275c..6eaf9466 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -67,6 +67,22 @@ public void When_EnumFlagCaseSensitivityOverriddenViaAttribute_Then_CasingVarian output.Text.Should().Contain("Ga"); } + [TestMethod] + [Description("Guards the ambiguity edge unlocked by issue #57: a typed token matching both a case-insensitive alias and another option's case-sensitive canonical token must be rejected as ambiguous at parse time, never silently bound to either parameter.")] + public void When_TokenMatchesCaseInsensitiveAliasAndAnotherOption_Then_AmbiguityIsRejected() + { + var sut = ReplApp.Create(); + sut.Map( + "probe", + ([ReplOption(Aliases = ["--Mode"], CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] string first = "ga", + [ReplOption(Name = "mode")] string second = "bu") => $"first={first};second={second}"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["probe", "--mode", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("Ambiguous option '--mode'"); + } + [TestMethod] [Description("Guards the override boundary: a sibling enum-flag alias without a case-sensitivity override must keep the global case-sensitive default, so per-member overrides stay scoped to their own aliases.")] public void When_EnumFlagWithoutOverrideAndCasingDiffers_Then_TokenIsRejected() From 543b6f2cf925a86d02e88afb3dc85aa747b5d39c Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 14:44:30 -0400 Subject: [PATCH 3/7] =?UTF-8?q?fix(parameters):=20review=20batch=20?= =?UTF-8?q?=E2=80=94=20public=20tri-state,=20enum-conflict=20case=20parity?= =?UTF-8?q?,=20global-options=20fail-fast?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Expose *Override read-only properties publicly so external tooling can distinguish an unset override from an explicit enum-default assignment; XML docs on the non-nullable getters state they do not reflect effective behavior when unset. - ValidateEnumConflicts now falls back to the global case sensitivity for unset per-option overrides (was hardcoded case-insensitive), aligning with every other effective-case resolution site. Reachable via the now-settable ZeroOrMore arity override (RED observed). - UseGlobalOptions fails fast (NotSupportedException) when a property declares a CaseSensitivity/Arity override, instead of silently discarding it (RED observed). - Coverage: unset-override states (collection arity, global-insensitive inheritance), explicit enum-value-zero override, options-group property path (both overrides), tightened unknown-option assertion. --- .../Attributes/ReplEnumFlagAttribute.cs | 8 +- .../Attributes/ReplOptionAttribute.cs | 14 ++- .../Attributes/ReplValueAliasAttribute.cs | 8 +- .../Parsing/InvocationOptionParser.cs | 10 +- src/Repl.Defaults/GlobalOptionsExtensions.cs | 20 ++++ .../Given_OptionAttributeOverrides.cs | 107 +++++++++++++++++- 6 files changed, 152 insertions(+), 15 deletions(-) diff --git a/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs index ecad1815..dfb73bd9 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplEnumFlagAttribute.cs @@ -22,12 +22,14 @@ public ReplEnumFlagAttribute(params string[] aliases) // Nullable enums are not legal attribute named arguments (CS0655), so the optional // override exposes a non-nullable property and tracks the unset state in a nullable - // backing field surfaced through the internal CaseSensitivityOverride property. + // backing field surfaced through the read-only CaseSensitivityOverride property. private ReplCaseSensitivity? _caseSensitivity; /// /// Optional case-sensitivity override for these aliases. - /// Only an explicit assignment overrides the global parsing default. + /// Only an explicit assignment overrides the global parsing default; when unset, the getter + /// returns the enum default and does not reflect the effective behavior — read + /// to distinguish unset from an explicit value. /// public ReplCaseSensitivity CaseSensitivity { @@ -38,5 +40,5 @@ public ReplCaseSensitivity CaseSensitivity /// /// Explicit case-sensitivity override, or null to inherit the global default. /// - internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; + public ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; } diff --git a/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs index fc352470..96858fab 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplOptionAttribute.cs @@ -28,13 +28,15 @@ public sealed class ReplOptionAttribute : Attribute // Nullable enums are not legal attribute named arguments (CS0655), so the optional // overrides expose a non-nullable property and track the unset state in a nullable - // backing field surfaced through the internal *Override properties. + // backing field surfaced through the read-only *Override properties. private ReplCaseSensitivity? _caseSensitivity; private ReplArity? _arity; /// /// Optional case-sensitivity override for this option. - /// Only an explicit assignment overrides the global parsing default. + /// Only an explicit assignment overrides the global parsing default; when unset, the getter + /// returns the enum default and does not reflect the effective behavior — read + /// to distinguish unset from an explicit value. /// public ReplCaseSensitivity CaseSensitivity { @@ -45,11 +47,13 @@ public ReplCaseSensitivity CaseSensitivity /// /// Explicit case-sensitivity override, or null to inherit the global default. /// - internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; + public ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; /// /// Optional arity override. - /// Only an explicit assignment overrides the arity inferred from the parameter shape. + /// Only an explicit assignment overrides the arity inferred from the parameter shape; when + /// unset, the getter returns the enum default and does not reflect the effective arity — read + /// to distinguish unset from an explicit value. /// public ReplArity Arity { @@ -60,5 +64,5 @@ public ReplArity Arity /// /// Explicit arity override, or null to use the arity inferred from the parameter shape. /// - internal ReplArity? ArityOverride => _arity; + public ReplArity? ArityOverride => _arity; } diff --git a/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs index 63ddb3c9..fe41ea02 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplValueAliasAttribute.cs @@ -31,12 +31,14 @@ public ReplValueAliasAttribute(string token, string value) // Nullable enums are not legal attribute named arguments (CS0655), so the optional // override exposes a non-nullable property and tracks the unset state in a nullable - // backing field surfaced through the internal CaseSensitivityOverride property. + // backing field surfaced through the read-only CaseSensitivityOverride property. private ReplCaseSensitivity? _caseSensitivity; /// /// Optional case-sensitivity override for this alias. - /// Only an explicit assignment overrides the global parsing default. + /// Only an explicit assignment overrides the global parsing default; when unset, the getter + /// returns the enum default and does not reflect the effective behavior — read + /// to distinguish unset from an explicit value. /// public ReplCaseSensitivity CaseSensitivity { @@ -47,5 +49,5 @@ public ReplCaseSensitivity CaseSensitivity /// /// Explicit case-sensitivity override, or null to inherit the global default. /// - internal ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; + public ReplCaseSensitivity? CaseSensitivityOverride => _caseSensitivity; } diff --git a/src/Repl.Core/Parsing/InvocationOptionParser.cs b/src/Repl.Core/Parsing/InvocationOptionParser.cs index 151e1590..29239449 100644 --- a/src/Repl.Core/Parsing/InvocationOptionParser.cs +++ b/src/Repl.Core/Parsing/InvocationOptionParser.cs @@ -202,7 +202,7 @@ public static OptionParsingResult Parse( diagnostics); } - ValidateArityAndConflicts(schema, namedOptions, diagnostics); + ValidateArityAndConflicts(schema, namedOptions, options.OptionCaseSensitivity, diagnostics); var readonlyNamedOptions = namedOptions.ToDictionary( pair => pair.Key, pair => (IReadOnlyList)pair.Value, @@ -440,6 +440,7 @@ private static void AddNamedValue( private static void ValidateArityAndConflicts( OptionSchema schema, Dictionary> namedOptions, + ReplCaseSensitivity globalCaseSensitivity, List diagnostics) { foreach (var parameter in schema.Parameters.Values) @@ -451,7 +452,7 @@ private static void ValidateArityAndConflicts( ValidateTooManyValues(schema, parameter, values, diagnostics); ValidateBooleanConflicts(parameter, values, diagnostics); - ValidateEnumConflicts(parameter, values, diagnostics); + ValidateEnumConflicts(parameter, values, globalCaseSensitivity, diagnostics); } } @@ -504,6 +505,7 @@ private static void ValidateBooleanConflicts( private static void ValidateEnumConflicts( OptionSchemaParameter parameter, List values, + ReplCaseSensitivity globalCaseSensitivity, List diagnostics) { var effectiveType = Nullable.GetUnderlyingType(parameter.ParameterType) ?? parameter.ParameterType; @@ -512,7 +514,9 @@ private static void ValidateEnumConflicts( return; } - var comparer = parameter.CaseSensitivity == ReplCaseSensitivity.CaseSensitive + // Unset per-option override falls back to the global default, matching every other + // effective-case resolution site (OptionSchema.ResolveToken, HandlerArgumentBinder, ...). + var comparer = (parameter.CaseSensitivity ?? globalCaseSensitivity) == ReplCaseSensitivity.CaseSensitive ? StringComparer.Ordinal : StringComparer.OrdinalIgnoreCase; if (!values.Distinct(comparer).Skip(1).Any()) diff --git a/src/Repl.Defaults/GlobalOptionsExtensions.cs b/src/Repl.Defaults/GlobalOptionsExtensions.cs index d19d8c6e..9c0a06a1 100644 --- a/src/Repl.Defaults/GlobalOptionsExtensions.cs +++ b/src/Repl.Defaults/GlobalOptionsExtensions.cs @@ -26,6 +26,26 @@ public static class GlobalOptionsExtensions var parsing = app.Core.OptionsSnapshot.Parsing; var properties = GetOptionProperties(); + + // Typed global options do not flow per-option CaseSensitivity/Arity overrides into + // AddGlobalOptionCore (the global-option pipeline has no per-option override concept + // yet). Fail fast instead of silently discarding a now-settable override. + foreach (var property in properties) + { + var optionAttr = property.GetCustomAttribute(); + if (optionAttr?.CaseSensitivityOverride is not null) + { + throw new NotSupportedException( + $"Global option property '{typeof(T).Name}.{property.Name}' declares a CaseSensitivity override, which is not supported for typed global options."); + } + + if (optionAttr?.ArityOverride is not null) + { + throw new NotSupportedException( + $"Global option property '{typeof(T).Name}.{property.Name}' declares an Arity override, which is not supported for typed global options."); + } + } + app.Options(options => { var prototype = new T(); diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index 6eaf9466..3743be94 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -15,6 +15,22 @@ private enum ShadockSyllable Bu = 2, } + [ReplOptionsGroup] + public sealed class DenimOutfitOptions + { + [ReplOption(CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] + public string Fabric { get; set; } = "denim"; + + [ReplOption(Arity = ReplArity.ZeroOrOne)] + public string[] Patches { get; set; } = []; + } + + public sealed class OverrideGlobals + { + [ReplOption(Name = "tenant", CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] + public string Tenant { get; set; } = "ga"; + } + [TestMethod] [Description("Regression guard for issue #57: [ReplOption(CaseSensitivity = ...)] must be a legal attribute argument (a nullable enum triggers CS0655, making the override dead code) and the per-option override must accept casing variants while the global default stays case-sensitive.")] public void When_OptionCaseSensitivityOverriddenViaAttribute_Then_CasingVariantIsAccepted() @@ -93,6 +109,95 @@ public void When_EnumFlagWithoutOverrideAndCasingDiffers_Then_TokenIsRejected() var output = ConsoleCaptureHelper.Capture(() => sut.Run(["say", "--BU", "--no-logo"])); output.ExitCode.Should().Be(1); - output.Text.Should().Contain("--BU"); + output.Text.Should().Contain("Unknown option '--BU'"); + } + + [TestMethod] + [Description("Guards the unset state of the Arity override: an attributed collection parameter without an explicit Arity must keep the inferred ZeroOrMore, so a builder-site drift back to the public (non-nullable) property — which compiles cleanly under ?. — would wrongly force ZeroOrOne and break repetition.")] + public void When_AttributedCollectionParameterWithoutArityOverride_Then_RepetitionIsAccepted() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Name = "item")] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--item", "ga", "--item", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("ga,bu"); + } + + [TestMethod] + [Description("Guards the unset state of the CaseSensitivity override at execution: an attributed option without an explicit override must inherit the global CaseInsensitive default instead of the enum default (CaseSensitive, value 0).")] + public void When_GlobalCaseInsensitiveAndNoOverride_Then_CasingVariantIsAccepted() + { + var sut = ReplApp.Create() + .Options(options => options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive); + sut.Map("echo", ([ReplOption(Name = "channel")] string channel) => channel); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--Channel", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("zo"); + } + + [TestMethod] + [Description("Guards the explicit-assignment-of-enum-value-zero edge: [ReplOption(CaseSensitivity = CaseSensitive)] (enum value 0) under a global CaseInsensitive default must register as a real override and reject casing variants — a sentinel-style refactor treating value 0 as unset would silently pass them.")] + public void When_ExplicitCaseSensitiveOverrideUnderGlobalInsensitive_Then_CasingVariantIsRejected() + { + var sut = ReplApp.Create() + .Options(options => options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive); + sut.Map("echo", ([ReplOption(CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string channel) => channel); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--Channel", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("Unknown option '--Channel'"); + } + + [TestMethod] + [Description("Guards the options-group property path (a parallel branch in OptionSchemaBuilder): a case-sensitivity override on a group property must be honored the same way as on a handler parameter.")] + public void When_GroupPropertyCaseSensitivityOverridden_Then_CasingVariantIsAccepted() + { + var sut = ReplApp.Create(); + sut.Map("wear", (DenimOutfitOptions options) => options.Fabric); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["wear", "--FABRIC", "bib overalls", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("bib overalls"); + } + + [TestMethod] + [Description("Guards the options-group property path for the arity override: a collection group property naturally infers ZeroOrMore; the explicit ZeroOrOne override must be honored so repetition is rejected.")] + public void When_GroupPropertyArityOverriddenToZeroOrOne_Then_RepeatedOptionIsRejected() + { + var sut = ReplApp.Create(); + sut.Map("wear", (DenimOutfitOptions options) => string.Join(',', options.Patches)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["wear", "--patches", "ga", "--patches", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("accepts at most one value"); + } + + [TestMethod] + [Description("Guards execution parity for enum duplicate detection: with an explicit ZeroOrMore arity (reachable only through the now-settable override), repeated enum values differing only by casing are distinct under the global CaseSensitive default and must be reported as conflicting — the validator previously hardcoded case-insensitive comparison when no per-option override was set.")] + public void When_RepeatedEnumValuesDifferByCaseUnderGlobalCaseSensitive_Then_ConflictIsReported() + { + var sut = ReplApp.Create(); + sut.Map("say", ([ReplOption(Arity = ReplArity.ZeroOrMore)] ShadockSyllable syllable = ShadockSyllable.None) => syllable.ToString()); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["say", "--syllable", "Ga", "--syllable", "GA", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("received multiple enum values"); + } + + [TestMethod] + [Description("Guards against a silent no-op newly reachable through issue #57: typed global options (UseGlobalOptions) do not support per-option CaseSensitivity/Arity overrides, so declaring one must fail fast at registration instead of being silently discarded.")] + public void When_GlobalOptionsPropertyDeclaresOverride_Then_RegistrationFailsFast() + { + var act = () => ReplApp.Create().UseGlobalOptions(); + + act.Should().Throw().WithMessage("*CaseSensitivity*"); } } From 07edb22b53f14371558e86a7493ae4be98d9460a Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 14:53:04 -0400 Subject: [PATCH 4/7] fix(parameters): pairwise collision case sensitivity + OneOrMore lower bound (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ValidateTokenCollisions decides collisions per pair under each entry's effective case sensitivity: ordinal-equal tokens always collide; tokens differing only by casing collide only when both entries are effectively case-insensitive. Two explicitly case-sensitive options differing only by case now register under a global case-insensitive default (RED observed). - HandlerArgumentBinder enforces the OneOrMore lower bound after both the named and positional binding attempts come up empty — option parsing alone cannot see positional consumption. Handler is no longer invoked with a missing required-repeatable value (RED observed); positional satisfaction is guarded by a boundary test. - ResolveParameterArity moved onto OptionSchema for reuse by the binder. --- .../Internal/Options/OptionSchema.cs | 12 +++++ .../Internal/Options/OptionSchemaBuilder.cs | 43 +++++++++++----- .../Parsing/HandlerArgumentBinder.cs | 10 ++++ .../Parsing/InvocationOptionParser.cs | 9 +--- .../Given_OptionAttributeOverrides.cs | 49 +++++++++++++++++++ 5 files changed, 104 insertions(+), 19 deletions(-) diff --git a/src/Repl.Core/Internal/Options/OptionSchema.cs b/src/Repl.Core/Internal/Options/OptionSchema.cs index 060410c9..11ccd74b 100644 --- a/src/Repl.Core/Internal/Options/OptionSchema.cs +++ b/src/Repl.Core/Internal/Options/OptionSchema.cs @@ -50,4 +50,16 @@ public IReadOnlyList ResolveToken(string token, ReplCaseSensi public bool TryGetParameter(string parameterName, out OptionSchemaParameter parameter) => Parameters.TryGetValue(parameterName, out parameter!); + + /// + /// Effective arity of a parameter, resolved from its named-option/flag entry; + /// parameters without such an entry default to the permissive . + /// + public ReplArity ResolveParameterArity(string parameterName) + { + var entry = Entries.FirstOrDefault(candidate => + string.Equals(candidate.ParameterName, parameterName, StringComparison.OrdinalIgnoreCase) + && candidate.TokenKind is OptionSchemaTokenKind.NamedOption or OptionSchemaTokenKind.BoolFlag); + return entry?.Arity ?? ReplArity.ZeroOrMore; + } } diff --git a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs index 83be3d24..686f8383 100644 --- a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs +++ b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs @@ -551,27 +551,46 @@ private static void ValidateTokenCollisions( IReadOnlyList entries, ParsingOptions parsingOptions) { - var comparer = parsingOptions.OptionCaseSensitivity == ReplCaseSensitivity.CaseInsensitive - ? StringComparer.OrdinalIgnoreCase - : StringComparer.Ordinal; - var map = new Dictionary(comparer); + // Collisions are decided per PAIR under each entry's effective case sensitivity, not + // under the global comparer alone: ordinal-equal tokens always collide, while tokens + // differing only by casing collide only when BOTH entries are effectively + // case-insensitive (full mutual shadowing — no typed token can distinguish them). + // Two explicitly case-sensitive entries stay distinguishable ordinally even under a + // global case-insensitive default, and a mixed pair keeps per-token runtime + // resolution (the parser's "Ambiguous option" diagnostic) instead of failing + // registration. Buckets are keyed ignoring case to surface every candidate pair. + var map = new Dictionary>(StringComparer.OrdinalIgnoreCase); foreach (var entry in entries) { - if (!map.TryGetValue(entry.Token, out var existing)) + if (!map.TryGetValue(entry.Token, out var bucket)) { - map[entry.Token] = entry; + map[entry.Token] = [entry]; continue; } - if (string.Equals(existing.ParameterName, entry.ParameterName, StringComparison.OrdinalIgnoreCase) - && existing.TokenKind == entry.TokenKind - && string.Equals(existing.InjectedValue, entry.InjectedValue, StringComparison.Ordinal)) + foreach (var existing in bucket) { - continue; + var ordinalEqual = string.Equals(existing.Token, entry.Token, StringComparison.Ordinal); + var bothInsensitive = + (existing.CaseSensitivity ?? parsingOptions.OptionCaseSensitivity) == ReplCaseSensitivity.CaseInsensitive + && (entry.CaseSensitivity ?? parsingOptions.OptionCaseSensitivity) == ReplCaseSensitivity.CaseInsensitive; + if (!ordinalEqual && !bothInsensitive) + { + continue; + } + + if (string.Equals(existing.ParameterName, entry.ParameterName, StringComparison.OrdinalIgnoreCase) + && existing.TokenKind == entry.TokenKind + && string.Equals(existing.InjectedValue, entry.InjectedValue, StringComparison.Ordinal)) + { + continue; + } + + throw new InvalidOperationException( + $"Option token collision detected for '{entry.Token}' between '{existing.ParameterName}' and '{entry.ParameterName}'."); } - throw new InvalidOperationException( - $"Option token collision detected for '{entry.Token}' between '{existing.ParameterName}' and '{entry.ParameterName}'."); + bucket.Add(entry); } } } diff --git a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs index fbff1705..dce5aa41 100644 --- a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs +++ b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs @@ -118,6 +118,16 @@ internal static class HandlerArgumentBinder return positionalValue; } + // The OneOrMore lower bound can only be enforced here, after BOTH the named and the + // positional binding attempts came up empty — option parsing alone cannot see values + // the parameter would have consumed positionally. + if (isUserOption + && context.OptionSchema.ResolveParameterArity(parameterName) == ReplArity.OneOrMore) + { + throw new InvalidOperationException( + $"Option '--{parameterName}' requires at least one value."); + } + if (parameter.HasDefaultValue) { return parameter.DefaultValue; diff --git a/src/Repl.Core/Parsing/InvocationOptionParser.cs b/src/Repl.Core/Parsing/InvocationOptionParser.cs index 29239449..2c94a54e 100644 --- a/src/Repl.Core/Parsing/InvocationOptionParser.cs +++ b/src/Repl.Core/Parsing/InvocationOptionParser.cs @@ -529,13 +529,8 @@ private static void ValidateEnumConflicts( $"Option '--{parameter.Name}' received multiple enum values in a single invocation.")); } - private static ReplArity ResolveParameterArity(OptionSchema schema, string parameterName) - { - var entry = schema.Entries.FirstOrDefault(candidate => - string.Equals(candidate.ParameterName, parameterName, StringComparison.OrdinalIgnoreCase) - && candidate.TokenKind is OptionSchemaTokenKind.NamedOption or OptionSchemaTokenKind.BoolFlag); - return entry?.Arity ?? ReplArity.ZeroOrMore; - } + private static ReplArity ResolveParameterArity(OptionSchema schema, string parameterName) => + schema.ResolveParameterArity(parameterName); private static List ExpandResponseFiles( IReadOnlyList tokens, diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index 3743be94..e8040dca 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -200,4 +200,53 @@ public void When_GlobalOptionsPropertyDeclaresOverride_Then_RegistrationFailsFas act.Should().Throw().WithMessage("*CaseSensitivity*"); } + + [TestMethod] + [Description("Guards collision validation against false positives newly reachable through issue #57: under a global CaseInsensitive default, two options explicitly marked CaseSensitive whose tokens differ only by casing are distinguishable ordinally at resolution time, so registration must accept them and route each token to its own parameter.")] + public void When_TwoExplicitlyCaseSensitiveOptionsDifferOnlyByCase_Then_RegistrationAndResolutionSucceed() + { + var sut = ReplApp.Create() + .Options(options => options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive); + sut.Map( + "cfg", + ([ReplOption(Name = "mode", CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string lower = "ga", + [ReplOption(Name = "MODE", CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string upper = "bu") => $"lower={lower};upper={upper}"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["cfg", "--MODE", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("lower=ga;upper=zo"); + } + + [TestMethod] + [Description("Guards the OneOrMore lower bound newly reachable through issue #57: an option with an explicit OneOrMore arity invoked without any value (named or positional) must fail with an arity diagnostic instead of invoking the handler with a missing value.")] + public void When_OneOrMoreArityOptionIsAbsent_Then_BindingFailsWithoutInvokingHandler() + { + var invoked = false; + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.OneOrMore)] string[] items) => + { + invoked = true; + return string.Join(',', items); + }); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("requires at least one value"); + invoked.Should().BeFalse(); + } + + [TestMethod] + [Description("Guards the boundary of the OneOrMore lower bound: values consumed positionally satisfy the arity, so the absence check must run only after both named and positional binding attempts — not at option-parse time, which cannot see positional consumption.")] + public void When_OneOrMoreArityOptionReceivesPositionalValues_Then_BindingSucceeds() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.OneOrMore)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "ga", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("ga,bu"); + } } From dd198043df42254fe61aced38b75ce4796464574 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 15:20:55 -0400 Subject: [PATCH 5/7] =?UTF-8?q?fix(parameters):=20review=20panel=20batch?= =?UTF-8?q?=20=E2=80=94=20explicit=20lower=20bounds=20everywhere,=20honest?= =?UTF-8?q?=20diagnostics,=20dead=20Position=20removed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - OptionSchemaParameter carries ExplicitArity so explicit lower bounds (OneOrMore, ExactlyOne) reject absence on ALL binding paths: handler parameters, options-group properties, and ArgumentOnly parameters (previously silently dropped — no schema entry). Inferred arities keep their historical behavior. - Lower-bound diagnostics name the canonical option token, falling back to the parameter name for ArgumentOnly; ResolveDisplayToken added beside ResolveParameterArity on OptionSchema (explicit loop, no closure). - Token-collision message names the command being registered; duplicate parameter/property names get a duplicate-name diagnostic instead of the misleading token-collision wording. - KnownTokens dedupes ordinally so case-distinct tokens of case-sensitive twins both survive for 'Did you mean' suggestions. - UseGlobalOptions fail-fast extended to value-alias CaseSensitivity overrides, with remediation hints; enum-flag overrides deliberately excluded (declared on possibly-shared enum types) with rationale. - ReplArgumentAttribute.Position removed: unsettable (CS0655, nullable int) AND never consumed — doubly dead public API; removal sanctioned pre-stable. Follow-up issue tracks real explicit-position support. All behavior changes RED-observed before the fix (8 failing tests). --- docs/parameter-system.md | 1 - .../Internal/Options/OptionSchema.cs | 31 +++- .../Internal/Options/OptionSchemaBuilder.cs | 17 +- .../Internal/Options/OptionSchemaParameter.cs | 3 +- .../Attributes/ReplArgumentAttribute.cs | 5 - .../Parsing/HandlerArgumentBinder.cs | 35 +++- src/Repl.Defaults/GlobalOptionsExtensions.cs | 52 +++--- .../Given_OptionAttributeOverrides.cs | 149 ++++++++++++++++++ .../Given_OptionsGroupBinding.cs | 4 +- 9 files changed, 250 insertions(+), 47 deletions(-) diff --git a/docs/parameter-system.md b/docs/parameter-system.md index 8c2771ea..472d90b5 100644 --- a/docs/parameter-system.md +++ b/docs/parameter-system.md @@ -29,7 +29,6 @@ Application-facing parameter DSL: - optional per-parameter `CaseSensitivity` - optional `Arity` - `ReplArgumentAttribute` - - optional positional `Position` - `Mode` - `ReplValueAliasAttribute` - maps a token to an injected parameter value (for example `--json` -> `output=json`) diff --git a/src/Repl.Core/Internal/Options/OptionSchema.cs b/src/Repl.Core/Internal/Options/OptionSchema.cs index 11ccd74b..5f88fdce 100644 --- a/src/Repl.Core/Internal/Options/OptionSchema.cs +++ b/src/Repl.Core/Internal/Options/OptionSchema.cs @@ -22,8 +22,11 @@ public OptionSchema( // Benign race: concurrent first reads compute the same array. private string[]? _knownTokens; + // Ordinal dedup: case-differing tokens can belong to DIFFERENT case-sensitive options + // (per-entry overrides), so collapsing them ignoring case would drop a real token and + // make "Did you mean" suggest the wrong casing. public IReadOnlyCollection KnownTokens => - _knownTokens ??= [.. Entries.Select(entry => entry.Token).Distinct(StringComparer.OrdinalIgnoreCase)]; + _knownTokens ??= [.. Entries.Select(entry => entry.Token).Distinct(StringComparer.Ordinal)]; public IReadOnlyList ResolveToken(string token, ReplCaseSensitivity globalCaseSensitivity) { @@ -55,11 +58,27 @@ public bool TryGetParameter(string parameterName, out OptionSchemaParameter para /// Effective arity of a parameter, resolved from its named-option/flag entry; /// parameters without such an entry default to the permissive . /// - public ReplArity ResolveParameterArity(string parameterName) + public ReplArity ResolveParameterArity(string parameterName) => + FindNamedEntry(parameterName)?.Arity ?? ReplArity.ZeroOrMore; + + /// + /// Canonical display token of a parameter's named-option/flag entry (with prefix), + /// or null for parameters without one (e.g. ArgumentOnly). + /// + public string? ResolveDisplayToken(string parameterName) => + FindNamedEntry(parameterName)?.Token; + + private OptionSchemaEntry? FindNamedEntry(string parameterName) { - var entry = Entries.FirstOrDefault(candidate => - string.Equals(candidate.ParameterName, parameterName, StringComparison.OrdinalIgnoreCase) - && candidate.TokenKind is OptionSchemaTokenKind.NamedOption or OptionSchemaTokenKind.BoolFlag); - return entry?.Arity ?? ReplArity.ZeroOrMore; + foreach (var entry in Entries) + { + if (string.Equals(entry.ParameterName, parameterName, StringComparison.OrdinalIgnoreCase) + && entry.TokenKind is OptionSchemaTokenKind.NamedOption or OptionSchemaTokenKind.BoolFlag) + { + return entry; + } + } + + return null; } } diff --git a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs index 686f8383..d1e69bde 100644 --- a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs +++ b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs @@ -53,7 +53,7 @@ public static OptionSchema Build( } ValidatePositionalBindingCompatibility(regularPositionalParameterNames, groupPositionalPropertyNames); - ValidateTokenCollisions(entries, parsingOptions); + ValidateTokenCollisions(entries, parsingOptions, template); return new OptionSchema(entries, parameters); } @@ -112,14 +112,15 @@ private static void AppendParameterSchemaEntries( if (parameters.ContainsKey(parameter.Name!)) { throw new InvalidOperationException( - $"Option token collision detected for parameter name '{parameter.Name}'."); + $"Duplicate parameter name '{parameter.Name}' in the command signature."); } parameters[parameter.Name!] = new OptionSchemaParameter( parameter.Name!, parameter.ParameterType, mode, - CaseSensitivity: optionAttribute?.CaseSensitivityOverride); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride, + ExplicitArity: optionAttribute?.ArityOverride); if (mode == ReplParameterMode.ArgumentOnly) { return; @@ -405,14 +406,15 @@ private static void AppendPropertySchemaEntries( if (parameters.ContainsKey(property.Name)) { throw new InvalidOperationException( - $"Option token collision detected for parameter name '{property.Name}'."); + $"Duplicate parameter name '{property.Name}' between options group '{property.DeclaringType?.Name}' and the command signature."); } parameters[property.Name] = new OptionSchemaParameter( property.Name, property.PropertyType, mode, - CaseSensitivity: optionAttribute?.CaseSensitivityOverride); + CaseSensitivity: optionAttribute?.CaseSensitivityOverride, + ExplicitArity: optionAttribute?.ArityOverride); if (mode != ReplParameterMode.OptionOnly) { positionalPropertyNames.Add(property.Name); @@ -549,7 +551,8 @@ private static void AppendPropertyEnumAliases( private static void ValidateTokenCollisions( IReadOnlyList entries, - ParsingOptions parsingOptions) + ParsingOptions parsingOptions, + RouteTemplate template) { // Collisions are decided per PAIR under each entry's effective case sensitivity, not // under the global comparer alone: ordinal-equal tokens always collide, while tokens @@ -587,7 +590,7 @@ private static void ValidateTokenCollisions( } throw new InvalidOperationException( - $"Option token collision detected for '{entry.Token}' between '{existing.ParameterName}' and '{entry.ParameterName}'."); + $"Option token collision detected for '{entry.Token}' between '{existing.ParameterName}' and '{entry.ParameterName}' while registering command '{template.Template}'."); } bucket.Add(entry); diff --git a/src/Repl.Core/Internal/Options/OptionSchemaParameter.cs b/src/Repl.Core/Internal/Options/OptionSchemaParameter.cs index 3bf67141..e506c84d 100644 --- a/src/Repl.Core/Internal/Options/OptionSchemaParameter.cs +++ b/src/Repl.Core/Internal/Options/OptionSchemaParameter.cs @@ -6,4 +6,5 @@ internal sealed record OptionSchemaParameter( string Name, Type ParameterType, ReplParameterMode Mode, - ReplCaseSensitivity? CaseSensitivity = null); + ReplCaseSensitivity? CaseSensitivity = null, + ReplArity? ExplicitArity = null); diff --git a/src/Repl.Core/Parameters/Attributes/ReplArgumentAttribute.cs b/src/Repl.Core/Parameters/Attributes/ReplArgumentAttribute.cs index 78f1eb45..2769e016 100644 --- a/src/Repl.Core/Parameters/Attributes/ReplArgumentAttribute.cs +++ b/src/Repl.Core/Parameters/Attributes/ReplArgumentAttribute.cs @@ -6,11 +6,6 @@ namespace Repl.Parameters; [AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property, AllowMultiple = false, Inherited = true)] public sealed class ReplArgumentAttribute : Attribute { - /// - /// Optional explicit position for positional binding. - /// - public int? Position { get; set; } - /// /// Binding mode for the parameter. /// diff --git a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs index dce5aa41..f3b4f6f4 100644 --- a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs +++ b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs @@ -118,15 +118,10 @@ internal static class HandlerArgumentBinder return positionalValue; } - // The OneOrMore lower bound can only be enforced here, after BOTH the named and the + // Explicit lower bounds can only be enforced here, after BOTH the named and the // positional binding attempts came up empty — option parsing alone cannot see values // the parameter would have consumed positionally. - if (isUserOption - && context.OptionSchema.ResolveParameterArity(parameterName) == ReplArity.OneOrMore) - { - throw new InvalidOperationException( - $"Option '--{parameterName}' requires at least one value."); - } + ThrowIfExplicitLowerBoundUnsatisfied(context.OptionSchema, parameterName); if (parameter.HasDefaultValue) { @@ -143,6 +138,26 @@ internal static class HandlerArgumentBinder $"Unable to bind parameter '{parameter.Name}' ({parameter.ParameterType.Name})."); } + // Only EXPLICIT arity overrides reject absence: inferred ExactlyOne (plain required + // scalars) keeps its historical binding behavior, and inferred arities never produce + // OneOrMore. ArgumentOnly parameters have no named entry, so the message falls back to + // the parameter name instead of a token the parser would not accept. + private static void ThrowIfExplicitLowerBoundUnsatisfied(OptionSchema schema, string parameterName) + { + if (!schema.TryGetParameter(parameterName, out var schemaParameter) + || schemaParameter.ExplicitArity is not (ReplArity.OneOrMore or ReplArity.ExactlyOne)) + { + return; + } + + var token = schema.ResolveDisplayToken(parameterName); + var subject = token is null ? $"Parameter '{parameterName}'" : $"Option '{token}'"; + var requirement = schemaParameter.ExplicitArity == ReplArity.OneOrMore + ? "at least one value" + : "exactly one value"; + throw new InvalidOperationException($"{subject} requires {requirement}."); + } + private static bool HasExplicitBindingDirection(System.Reflection.ParameterInfo parameter) => parameter.GetCustomAttributes(typeof(FromContextAttribute), inherit: true).Length > 0 || parameter.GetCustomAttributes(typeof(FromServicesAttribute), inherit: true).Length > 0; @@ -604,7 +619,13 @@ private static object BindOptionsGroup( out var positionalValue)) { property.SetValue(instance, positionalValue); + continue; } + + // Mirrors the handler-parameter path: an explicit lower bound rejects absence + // after both named and positional attempts, instead of silently keeping the + // property default. + ThrowIfExplicitLowerBoundUnsatisfied(context.OptionSchema, propertyName); } return instance; diff --git a/src/Repl.Defaults/GlobalOptionsExtensions.cs b/src/Repl.Defaults/GlobalOptionsExtensions.cs index 9c0a06a1..34765b7d 100644 --- a/src/Repl.Defaults/GlobalOptionsExtensions.cs +++ b/src/Repl.Defaults/GlobalOptionsExtensions.cs @@ -27,24 +27,7 @@ public static class GlobalOptionsExtensions var parsing = app.Core.OptionsSnapshot.Parsing; var properties = GetOptionProperties(); - // Typed global options do not flow per-option CaseSensitivity/Arity overrides into - // AddGlobalOptionCore (the global-option pipeline has no per-option override concept - // yet). Fail fast instead of silently discarding a now-settable override. - foreach (var property in properties) - { - var optionAttr = property.GetCustomAttribute(); - if (optionAttr?.CaseSensitivityOverride is not null) - { - throw new NotSupportedException( - $"Global option property '{typeof(T).Name}.{property.Name}' declares a CaseSensitivity override, which is not supported for typed global options."); - } - - if (optionAttr?.ArityOverride is not null) - { - throw new NotSupportedException( - $"Global option property '{typeof(T).Name}.{property.Name}' declares an Arity override, which is not supported for typed global options."); - } - } + ThrowIfUnsupportedOverrides(typeof(T), properties); app.Options(options => { @@ -75,6 +58,39 @@ public static class GlobalOptionsExtensions return app; } + // Typed global options do not flow per-option CaseSensitivity/Arity overrides into + // AddGlobalOptionCore (the global-option pipeline has no per-option override concept + // yet). Fail fast instead of silently discarding a now-settable override. Enum-flag + // overrides are declared on the enum TYPE's fields — which may be legitimately shared + // with route commands where they do work — so they are deliberately not rejected here. + private static void ThrowIfUnsupportedOverrides(Type optionsType, IReadOnlyList properties) + { + foreach (var property in properties) + { + var optionAttr = property.GetCustomAttribute(); + if (optionAttr?.CaseSensitivityOverride is not null) + { + throw new NotSupportedException( + $"Global option property '{optionsType.Name}.{property.Name}' declares a CaseSensitivity override, which is not supported for typed global options. Remove the override or expose the option through a per-command options type."); + } + + if (optionAttr?.ArityOverride is not null) + { + throw new NotSupportedException( + $"Global option property '{optionsType.Name}.{property.Name}' declares an Arity override, which is not supported for typed global options. Remove the override or expose the option through a per-command options type."); + } + + foreach (var valueAlias in property.GetCustomAttributes()) + { + if (valueAlias.CaseSensitivityOverride is not null) + { + throw new NotSupportedException( + $"Global option property '{optionsType.Name}.{property.Name}' declares a CaseSensitivity override on value alias '{valueAlias.Token}', which is not supported for typed global options. Remove the override or expose the option through a per-command options type."); + } + } + } + } + internal static T PopulateInstance<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>(IGlobalOptionsAccessor accessor, IFormatProvider numericFormatProvider) where T : class, new() { diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index e8040dca..45bbd07d 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -31,6 +31,25 @@ public sealed class OverrideGlobals public string Tenant { get; set; } = "ga"; } + public sealed class OverrideArityGlobals + { + [ReplOption(Name = "retries", Arity = ReplArity.ExactlyOne)] + public int Retries { get; set; } = 42; + } + + public sealed class ValueAliasOverrideGlobals + { + [ReplValueAlias("--prod", "production", CaseSensitivity = ReplCaseSensitivity.CaseInsensitive)] + public string Environment { get; set; } = "dev"; + } + + [ReplOptionsGroup] + public sealed class RequiredPatchesOptions + { + [ReplOption(Arity = ReplArity.OneOrMore)] + public string[] Patches { get; set; } = []; + } + [TestMethod] [Description("Regression guard for issue #57: [ReplOption(CaseSensitivity = ...)] must be a legal attribute argument (a nullable enum triggers CS0655, making the override dead code) and the per-option override must accept casing variants while the global default stays case-sensitive.")] public void When_OptionCaseSensitivityOverriddenViaAttribute_Then_CasingVariantIsAccepted() @@ -249,4 +268,134 @@ public void When_OneOrMoreArityOptionReceivesPositionalValues_Then_BindingSuccee output.ExitCode.Should().Be(0); output.Text.Should().Contain("ga,bu"); } + + [TestMethod] + [Description("Guards the OneOrMore lower bound on the options-group property path: an absent group property with an explicit OneOrMore arity must fail binding instead of silently keeping its default and invoking the handler.")] + public void When_GroupPropertyOneOrMoreArityIsAbsent_Then_BindingFailsWithoutInvokingHandler() + { + var invoked = false; + var sut = ReplApp.Create(); + sut.Map("wear", (RequiredPatchesOptions options) => + { + invoked = true; + return string.Join(',', options.Patches); + }); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["wear", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("requires at least one value"); + invoked.Should().BeFalse(); + } + + [TestMethod] + [Description("Guards the OneOrMore lower bound for ArgumentOnly parameters: they have no named-option schema entry, so the explicit arity must be carried at the parameter level — otherwise the declared override is silently dropped and the handler receives a missing value.")] + public void When_ArgumentOnlyOneOrMoreArityIsAbsent_Then_BindingFails() + { + var sut = ReplApp.Create(); + sut.Map("copy", ([ReplOption(Mode = ReplParameterMode.ArgumentOnly, Arity = ReplArity.OneOrMore)] string[] files) => string.Join(',', files)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["copy", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("requires at least one value"); + } + + [TestMethod] + [Description("Boundary for the ArgumentOnly OneOrMore bound: positional values satisfy the explicit arity, so the same registration invoked with positionals must bind and execute.")] + public void When_ArgumentOnlyOneOrMoreArityReceivesPositionals_Then_BindingSucceeds() + { + var sut = ReplApp.Create(); + sut.Map("copy", ([ReplOption(Mode = ReplParameterMode.ArgumentOnly, Arity = ReplArity.OneOrMore)] string[] files) => string.Join(',', files)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["copy", "ga", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("ga,bu"); + } + + [TestMethod] + [Description("Guards the explicit ExactlyOne lower bound: ReplArity.ExactlyOne is documented as 'must appear exactly one time', so an EXPLICIT override on an otherwise-optional parameter must reject absence — while inferred ExactlyOne (plain required scalars) keeps its existing binding behavior.")] + public void When_ExplicitExactlyOneArityOptionIsAbsent_Then_BindingFails() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.ExactlyOne)] string? channel = null) => channel ?? "none"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("requires exactly one value"); + } + + [TestMethod] + [Description("Guards diagnostic accuracy: the lower-bound failure message must name the canonical option token ([ReplOption(Name = ...)]), not the CLR parameter name — telling the user to supply a token the parser would reject is actively misleading.")] + public void When_RenamedOneOrMoreOptionIsAbsent_Then_MessageUsesCanonicalToken() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Name = "item", Arity = ReplArity.OneOrMore)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("Option '--item' requires"); + } + + [TestMethod] + [Description("Guards the Arity branch of the typed-global-options fail-fast: reverting only that branch would silently discard the override while the CaseSensitivity twin keeps the suite green.")] + public void When_GlobalOptionsPropertyDeclaresArityOverride_Then_RegistrationFailsFast() + { + var act = () => ReplApp.Create().UseGlobalOptions(); + + act.Should().Throw().WithMessage("*Arity*"); + } + + [TestMethod] + [Description("Guards the value-alias branch of the typed-global-options fail-fast: a [ReplValueAlias(..., CaseSensitivity = ...)] override on a global property is equally newly settable and must be rejected instead of silently discarded.")] + public void When_GlobalOptionsValueAliasDeclaresOverride_Then_RegistrationFailsFast() + { + var act = () => ReplApp.Create().UseGlobalOptions(); + + act.Should().Throw().WithMessage("*CaseSensitivity*"); + } + + [TestMethod] + [Description("Guards suggestion fidelity for case-distinct tokens: KnownTokens must not dedupe tokens ignoring case, otherwise one of two case-sensitive twins vanishes and 'Did you mean' proposes the wrong casing.")] + public void When_CaseSensitiveTwinTokensRegistered_Then_SuggestionPreservesEachCasing() + { + var sut = ReplApp.Create(); + sut.Map( + "cfg", + ([ReplOption(Name = "mode")] string lower = "ga", + [ReplOption(Name = "MODE")] string upper = "bu") => $"lower={lower};upper={upper}"); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["cfg", "--MODEs", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("Did you mean '--MODE'"); + } + + [TestMethod] + [Description("Guards diagnostic context: a token collision thrown at Map() time must name the command being registered, so an app with many registrations does not need a debugger to find the offending one.")] + public void When_TokenCollisionDetected_Then_MessageNamesTheCommand() + { + var sut = ReplApp.Create(); + var act = () => sut.Map( + "cfg", + ([ReplOption(Name = "mode")] string first = "ga", + [ReplOption(Aliases = ["--mode"])] string second = "bu") => $"{first}{second}"); + + act.Should().Throw().WithMessage("*cfg*"); + } + + [TestMethod] + [Description("Guards diagnostic wording: a group property whose name collides with a handler parameter is a duplicate-name error, not a token collision — the old wording sent developers hunting for alias conflicts that do not exist.")] + public void When_GroupPropertyNameCollidesWithParameter_Then_MessageSaysDuplicateName() + { + var sut = ReplApp.Create(); + var act = () => sut.Map( + "wear", + (DenimOutfitOptions options, [ReplOption] string fabric = "denim") => fabric); + + act.Should().Throw().WithMessage("*Duplicate*"); + } } diff --git a/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs b/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs index a564ae47..d0f1ff38 100644 --- a/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs +++ b/src/Repl.IntegrationTests/Given_OptionsGroupBinding.cs @@ -155,7 +155,7 @@ public void When_NullableGroupPropertyInitializedToUnderlyingClrDefault_Then_Hel } [TestMethod] - [Description("Regression guard: verifies token collision between group and regular parameter fails at registration.")] + [Description("Regression guard: verifies a duplicate name between a group property and a regular parameter fails at registration with a duplicate-name diagnostic (not a token-collision one).")] public void When_GroupPropertyCollidesWithParam_Then_MapFails() { var sut = ReplApp.Create(); @@ -163,7 +163,7 @@ public void When_GroupPropertyCollidesWithParam_Then_MapFails() var act = () => sut.Map("list", (TestOutputOptions output, string format) => format); act.Should().Throw() - .WithMessage("*collision*"); + .WithMessage("*Duplicate parameter name*"); } [TestMethod] From c490a47754c3794171867f92912a462cc8154bfb Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 15:41:21 -0400 Subject: [PATCH 6/7] fix(parameters): positional upper-bound parity + permissive-mode case-override bypass (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Explicit ZeroOrOne/ExactlyOne upper bounds now apply to positional consumption on both the handler-parameter and options-group paths: a collection consuming several positionals under an explicit cap fails with the same diagnostics as the repeated named option (RED observed). - The permissive unknown-option fallback (AllowUnknownOptions) no longer rebinds a casing-mismatched token to a parameter whose effective case sensitivity is CaseSensitive — the token stays an inert unknown, while unconstrained parameters keep the documented bind-by-name behavior (RED observed; boundary test guards the preserved behavior). --- .../Parsing/HandlerArgumentBinder.cs | 60 ++++++++++---- .../Parsing/InvocationOptionParser.cs | 12 +++ .../Given_OptionAttributeOverrides.cs | 83 +++++++++++++++++++ 3 files changed, 141 insertions(+), 14 deletions(-) diff --git a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs index f3b4f6f4..91d6e7c5 100644 --- a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs +++ b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs @@ -106,16 +106,23 @@ internal static class HandlerArgumentBinder return resolved; } - if (bindingMode != ReplParameterMode.OptionOnly - && TryConsumePositional( - parameter, - context.PositionalArguments, - context.NumericFormatProvider, - enumIgnoreCase, - ref positionalIndex, - out var positionalValue)) + if (bindingMode != ReplParameterMode.OptionOnly) { - return positionalValue; + var positionalStart = positionalIndex; + if (TryConsumePositional( + parameter, + context.PositionalArguments, + context.NumericFormatProvider, + enumIgnoreCase, + ref positionalIndex, + out var positionalValue)) + { + // Positional parity for explicit upper bounds: a collection consumes every + // remaining positional, so the named-path arity diagnostics must apply here too. + ThrowIfExplicitUpperBoundExceeded( + context.OptionSchema, parameterName, positionalIndex - positionalStart); + return positionalValue; + } } // Explicit lower bounds can only be enforced here, after BOTH the named and the @@ -158,6 +165,25 @@ private static void ThrowIfExplicitLowerBoundUnsatisfied(OptionSchema schema, st throw new InvalidOperationException($"{subject} requires {requirement}."); } + // Positional twin of the parse-time ValidateTooManyValues: only explicit upper bounds are + // enforced (inferred collection arity is ZeroOrMore), and only collections can consume + // more than one positional. + private static void ThrowIfExplicitUpperBoundExceeded(OptionSchema schema, string parameterName, int consumedCount) + { + if (consumedCount <= 1 + || !schema.TryGetParameter(parameterName, out var schemaParameter) + || schemaParameter.ExplicitArity is not (ReplArity.ZeroOrOne or ReplArity.ExactlyOne)) + { + return; + } + + var token = schema.ResolveDisplayToken(parameterName); + var subject = token is null ? $"Parameter '{parameterName}'" : $"Option '{token}'"; + throw new InvalidOperationException(schemaParameter.ExplicitArity == ReplArity.ZeroOrOne + ? $"{subject} accepts at most one value." + : $"{subject} requires exactly one value."); + } + private static bool HasExplicitBindingDirection(System.Reflection.ParameterInfo parameter) => parameter.GetCustomAttributes(typeof(FromContextAttribute), inherit: true).Length > 0 || parameter.GetCustomAttributes(typeof(FromServicesAttribute), inherit: true).Length > 0; @@ -609,17 +635,23 @@ private static object BindOptionsGroup( continue; } - if (bindingMode != ReplParameterMode.OptionOnly - && TryConsumePositionalForProperty( + if (bindingMode != ReplParameterMode.OptionOnly) + { + var positionalStart = positionalIndex; + if (TryConsumePositionalForProperty( property, context.PositionalArguments, context.NumericFormatProvider, enumIgnoreCase, ref positionalIndex, out var positionalValue)) - { - property.SetValue(instance, positionalValue); - continue; + { + // Same positional upper-bound parity as the handler-parameter path. + ThrowIfExplicitUpperBoundExceeded( + context.OptionSchema, propertyName, positionalIndex - positionalStart); + property.SetValue(instance, positionalValue); + continue; + } } // Mirrors the handler-parameter path: an explicit lower bound rejects absence diff --git a/src/Repl.Core/Parsing/InvocationOptionParser.cs b/src/Repl.Core/Parsing/InvocationOptionParser.cs index 2c94a54e..0f7646e1 100644 --- a/src/Repl.Core/Parsing/InvocationOptionParser.cs +++ b/src/Repl.Core/Parsing/InvocationOptionParser.cs @@ -300,6 +300,18 @@ private static void HandleUnknownOption( value = effectiveTokens[index]; } + // A casing-mismatched token naming a KNOWN parameter whose effective sensitivity is + // CaseSensitive must stay an inert unknown: storing it would let the namedOptions + // comparer (global) rebind it to a parameter whose schema entries explicitly rejected + // this casing. Unconstrained parameters keep the documented permissive + // bind-by-name behavior. The value token stays consumed either way. + if (schema.TryGetParameter(optionName, out var knownParameter) + && !string.Equals(knownParameter.Name, optionName, StringComparison.Ordinal) + && (knownParameter.CaseSensitivity ?? options.OptionCaseSensitivity) == ReplCaseSensitivity.CaseSensitive) + { + return; + } + AddNamedValue(namedOptions, optionName, value ?? "true"); } diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index 45bbd07d..30b4a10e 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -50,6 +50,13 @@ public sealed class RequiredPatchesOptions public string[] Patches { get; set; } = []; } + [ReplOptionsGroup] + public sealed class SinglePatchOptions + { + [ReplOption(Mode = ReplParameterMode.OptionAndPositional, Arity = ReplArity.ZeroOrOne)] + public string[] Patches { get; set; } = []; + } + [TestMethod] [Description("Regression guard for issue #57: [ReplOption(CaseSensitivity = ...)] must be a legal attribute argument (a nullable enum triggers CS0655, making the override dead code) and the per-option override must accept casing variants while the global default stays case-sensitive.")] public void When_OptionCaseSensitivityOverriddenViaAttribute_Then_CasingVariantIsAccepted() @@ -398,4 +405,80 @@ public void When_GroupPropertyNameCollidesWithParameter_Then_MessageSaysDuplicat act.Should().Throw().WithMessage("*Duplicate*"); } + + [TestMethod] + [Description("Guards named/positional parity for explicit upper bounds: a ZeroOrOne collection rejects a repeated named option, so two positional values feeding the same collection must be rejected too — otherwise the override is enforceable on one input path and bypassable on the other.")] + public void When_ZeroOrOneCollectionReceivesTwoPositionals_Then_BindingFails() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.ZeroOrOne)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "ga", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("accepts at most one value"); + } + + [TestMethod] + [Description("Guards named/positional parity for the ExactlyOne upper bound on the positional path: two positional values must be rejected with the exactly-one diagnostic, mirroring the named-option validator.")] + public void When_ExactlyOneCollectionReceivesTwoPositionals_Then_BindingFails() + { + var sut = ReplApp.Create(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.ExactlyOne)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["echo", "ga", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("requires exactly one value"); + } + + [TestMethod] + [Description("Guards named/positional parity for explicit upper bounds on the options-group property path: a positional ZeroOrOne collection property must reject two positional values the same way repeated named values are rejected.")] + public void When_GroupZeroOrOneCollectionReceivesTwoPositionals_Then_BindingFails() + { + var sut = ReplApp.Create(); + sut.Map("wear", (SinglePatchOptions options) => string.Join(',', options.Patches)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["wear", "ga", "bu", "--no-logo"])); + + output.ExitCode.Should().Be(1); + output.Text.Should().Contain("accepts at most one value"); + } + + [TestMethod] + [Description("Guards the CaseSensitive override against the permissive unknown-option fallback: with AllowUnknownOptions and a global CaseInsensitive default, a casing-mismatched token must stay an inert unknown instead of rebinding by parameter name to an option that explicitly rejected that casing.")] + public void When_PermissiveUnknownTokenCaseMismatchesCaseSensitiveOption_Then_ValueIsNotBound() + { + var sut = ReplApp.Create() + .Options(options => + { + options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive; + options.Parsing.AllowUnknownOptions = true; + }); + sut.Map("run", ([ReplOption(CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string mode = "ga") => mode); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["run", "--Mode", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("ga"); + output.Text.Should().NotContain("zo"); + } + + [TestMethod] + [Description("Boundary for the permissive-fallback guard: without a per-option override, permissive mode keeps binding unknown casing variants by parameter name under the global CaseInsensitive comparer — the documented migration behavior must survive the fix.")] + public void When_PermissiveUnknownTokenCaseMismatchesUnconstrainedOption_Then_ValueStillBinds() + { + var sut = ReplApp.Create() + .Options(options => + { + options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive; + options.Parsing.AllowUnknownOptions = true; + }); + sut.Map("run", ([ReplOption] string mode = "ga") => mode); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["run", "--MoDe", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("zo"); + } } From adeee5d592bb463f5228afbf34ebf62f63e55330 Mon Sep 17 00:00:00 2001 From: Carl de Billy Date: Wed, 15 Jul 2026 16:23:15 -0400 Subject: [PATCH 7/7] fix(parameters): renamed-token permissive guard + explicit-arity doc requiredness (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The permissive-fallback guard now also drops tokens rejected by an EXPLICITLY CaseSensitive option even when the rejected token ordinally equals the CLR parameter name (renamed-token edge: Name differing from the parameter name only by casing). Unconstrained parameters keep the documented bind-by-name behavior (RED observed). - Documentation export reports options with an explicit OneOrMore/ExactlyOne arity as required — they now fail binding when omitted — on both the handler-parameter and options-group paths (group options were hard-coded as not required) (RED observed x2). --- .../Documentation/DocumentationEngine.cs | 11 ++++- .../Parsing/InvocationOptionParser.cs | 26 ++++++---- .../Given_OptionAttributeOverrides.cs | 47 +++++++++++++++++++ 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/Repl.Core/Documentation/DocumentationEngine.cs b/src/Repl.Core/Documentation/DocumentationEngine.cs index 324b7956..8c77d2e2 100644 --- a/src/Repl.Core/Documentation/DocumentationEngine.cs +++ b/src/Repl.Core/Documentation/DocumentationEngine.cs @@ -294,6 +294,13 @@ internal ReplDocApp BuildDocumentationApp() return new ReplDocApp(name, version, description); } + // An explicit OneOrMore/ExactlyOne arity now fails binding when the option is absent + // (HandlerArgumentBinder), so exported docs must report it as required regardless of the + // CLR parameter shape. + private static bool HasExplicitRequiredArity(OptionSchema schema, string parameterName) => + schema.TryGetParameter(parameterName, out var schemaParameter) + && schemaParameter.ExplicitArity is ReplArity.OneOrMore or ReplArity.ExactlyOne; + private static bool IsRequiredParameter(ParameterInfo parameter) { if (parameter.HasDefaultValue) @@ -406,7 +413,7 @@ private static ReplDocOption BuildDocumentationOptionFromProperty( return new ReplDocOption( Name: aliases.Length > 0 ? aliases[0].TrimStart('-') : property.Name, Type: GetFriendlyTypeName(property.PropertyType), - Required: false, + Required: HasExplicitRequiredArity(schema, property.Name), Description: property.GetCustomAttribute()?.Description, Aliases: aliases, ReverseAliases: reverseAliases, @@ -446,7 +453,7 @@ private static ReplDocOption BuildDocumentationOption(OptionSchema schema, Param return new ReplDocOption( Name: aliases.Length > 0 ? aliases[0].TrimStart('-') : parameter.Name!, Type: GetFriendlyTypeName(parameter.ParameterType), - Required: IsRequiredParameter(parameter), + Required: IsRequiredParameter(parameter) || HasExplicitRequiredArity(schema, parameter.Name!), Description: parameter.GetCustomAttribute()?.Description, Aliases: aliases, ReverseAliases: reverseAliases, diff --git a/src/Repl.Core/Parsing/InvocationOptionParser.cs b/src/Repl.Core/Parsing/InvocationOptionParser.cs index 0f7646e1..e8eed28a 100644 --- a/src/Repl.Core/Parsing/InvocationOptionParser.cs +++ b/src/Repl.Core/Parsing/InvocationOptionParser.cs @@ -300,16 +300,24 @@ private static void HandleUnknownOption( value = effectiveTokens[index]; } - // A casing-mismatched token naming a KNOWN parameter whose effective sensitivity is - // CaseSensitive must stay an inert unknown: storing it would let the namedOptions - // comparer (global) rebind it to a parameter whose schema entries explicitly rejected - // this casing. Unconstrained parameters keep the documented permissive - // bind-by-name behavior. The value token stays consumed either way. - if (schema.TryGetParameter(optionName, out var knownParameter) - && !string.Equals(knownParameter.Name, optionName, StringComparison.Ordinal) - && (knownParameter.CaseSensitivity ?? options.OptionCaseSensitivity) == ReplCaseSensitivity.CaseSensitive) + // A token naming a KNOWN parameter must stay an inert unknown when its schema entries + // rejected it for case-sensitivity reasons: storing it would let the namedOptions + // comparer (global) rebind it to that parameter, bypassing the declared casing. + // Two shapes reach here: an EXPLICIT CaseSensitive override always wins (even when the + // rejected token ordinally equals the CLR parameter name — the renamed-token edge), + // and an effectively sensitive parameter drops casing VARIANTS (they were inert under + // a global-case-sensitive dictionary anyway). Unconstrained parameters keep the + // documented permissive bind-by-name behavior. The value token stays consumed. + if (schema.TryGetParameter(optionName, out var knownParameter)) { - return; + var explicitlySensitive = knownParameter.CaseSensitivity == ReplCaseSensitivity.CaseSensitive; + var effectivelySensitive = + (knownParameter.CaseSensitivity ?? options.OptionCaseSensitivity) == ReplCaseSensitivity.CaseSensitive; + var matchesClrName = string.Equals(knownParameter.Name, optionName, StringComparison.Ordinal); + if (explicitlySensitive || (effectivelySensitive && !matchesClrName)) + { + return; + } } AddNamedValue(namedOptions, optionName, value ?? "true"); diff --git a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs index 30b4a10e..24187f78 100644 --- a/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs +++ b/src/Repl.IntegrationTests/Given_OptionAttributeOverrides.cs @@ -481,4 +481,51 @@ public void When_PermissiveUnknownTokenCaseMismatchesUnconstrainedOption_Then_Va output.ExitCode.Should().Be(0); output.Text.Should().Contain("zo"); } + + [TestMethod] + [Description("Guards the permissive-fallback guard against the renamed-token edge: when [ReplOption(Name = ...)] differs from the CLR parameter name only by casing, a token rejected by the case-sensitive schema entry must not slip through the name-equality path of the fallback and bind anyway.")] + public void When_PermissiveTokenMatchesRenamedCaseSensitiveOption_Then_ValueIsNotBound() + { + var sut = ReplApp.Create() + .Options(options => + { + options.Parsing.OptionCaseSensitivity = ReplCaseSensitivity.CaseInsensitive; + options.Parsing.AllowUnknownOptions = true; + }); + sut.Map("run", ([ReplOption(Name = "Mode", CaseSensitivity = ReplCaseSensitivity.CaseSensitive)] string mode = "ga") => mode); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["run", "--mode", "zo", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("ga"); + output.Text.Should().NotContain("zo"); + } + + [TestMethod] + [Description("Guards documentation fidelity for explicit lower bounds: an option that now fails when omitted (explicit OneOrMore) must be exported as required — otherwise generated docs tell users a runtime-required option is optional.")] + public void When_ExportingDocForExplicitOneOrMoreOption_Then_OptionIsRequired() + { + var sut = ReplApp.Create() + .UseDocumentationExport(); + sut.Map("echo", ([ReplOption(Arity = ReplArity.OneOrMore)] string[] items) => string.Join(',', items)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["doc", "export", "--json", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("\"required\": true"); + } + + [TestMethod] + [Description("Guards documentation fidelity on the options-group path: group options were hard-coded as not required, so an explicit OneOrMore group property — which now fails binding when omitted — must be exported as required.")] + public void When_ExportingDocForExplicitOneOrMoreGroupProperty_Then_OptionIsRequired() + { + var sut = ReplApp.Create() + .UseDocumentationExport(); + sut.Map("wear", (RequiredPatchesOptions options) => string.Join(',', options.Patches)); + + var output = ConsoleCaptureHelper.Capture(() => sut.Run(["doc", "export", "--json", "--no-logo"])); + + output.ExitCode.Should().Be(0); + output.Text.Should().Contain("\"required\": true"); + } }