Merge pull request #24 from yllibed/dev/cdb/issue-23-global-options-di

carldebilly · web-flow · commit 2cca95c11409 · 2026-05-01T22:22:02.000-04:00
Fix typed global options DI binding
diff --git a/src/Repl.Core/CoreReplApp.Execution.cs b/src/Repl.Core/CoreReplApp.Execution.cs
@@ -844,6 +844,7 @@ private InvocationBindingContext CreateInvocationBindingContext(
 			_options.Parsing.NumericFormatProvider,
 			serviceProvider,
 			_options.Interaction,
+			_implicitServiceParameters,
 			cancellationToken);
 	}
 
diff --git a/src/Repl.Core/CoreReplApp.cs b/src/Repl.Core/CoreReplApp.cs
@@ -19,6 +19,7 @@ public sealed partial class CoreReplApp : ICoreReplApp
 	private readonly List<RouteDefinition> _routes = [];
 	private readonly List<Func<ReplExecutionContext, ReplNext, ValueTask>> _middleware = [];
 	private readonly ReplOptions _options = new();
+	private readonly ImplicitServiceParameterRegistry _implicitServiceParameters = new();
 	private readonly List<ModuleRegistration> _moduleRegistrations = [];
 	private readonly Stack<int> _moduleMappingScope = new();
 	private int _nextModuleId = 1;
@@ -37,6 +38,7 @@ public sealed partial class CoreReplApp : ICoreReplApp
 	internal string? Description => _description;
 	internal IGlobalOptionsAccessor GlobalOptionsAccessor => _globalOptionsSnapshot;
 	internal GlobalOptionsSnapshot GlobalOptionsSnapshotInstance => _globalOptionsSnapshot;
+	internal ImplicitServiceParameterRegistry ImplicitServiceParameters => _implicitServiceParameters;
 	internal ShellCompletionRuntime ShellCompletionRuntimeInstance => _shellCompletionRuntime;
 	internal IReplExecutionObserver? ExecutionObserver { get; set; }
 	internal List<ContextDefinition> Contexts => _contexts;
@@ -61,6 +63,12 @@ private CoreReplApp()
 			static context => context.Channel is ReplRuntimeChannel.Cli or ReplRuntimeChannel.Interactive);
 	}
 
+	internal void RegisterGlobalOptionsType(Type optionsType)
+	{
+		ArgumentNullException.ThrowIfNull(optionsType);
+		_implicitServiceParameters.AddGlobalOptionsType(optionsType);
+	}
+
 	/// <summary>
 	/// Creates a dependency-free REPL application instance.
 	/// </summary>
@@ -172,7 +180,7 @@ public CommandBuilder Map(string route, Delegate handler)
 				.Select(existingRoute => existingRoute.Template));
 
 		_commands.Add(command);
-		var optionSchema = OptionSchemaBuilder.Build(template, command, _options.Parsing);
+		var optionSchema = OptionSchemaBuilder.Build(template, command, _options.Parsing, _implicitServiceParameters);
 		var routeDefinition = new RouteDefinition(template, command, moduleId, optionSchema);
 		_routes.Add(routeDefinition);
 		InvalidateRouting();
diff --git a/src/Repl.Core/Documentation/DocumentationEngine.cs b/src/Repl.Core/Documentation/DocumentationEngine.cs
@@ -238,7 +238,7 @@ private ReplDocCommand BuildDocumentationCommand(RouteDefinition route)
 				!string.IsNullOrWhiteSpace(parameter.Name)
 				&& parameter.ParameterType != typeof(CancellationToken)
 				&& !routeParameterNames.Contains(parameter.Name!)
-				&& !IsFrameworkInjectedParameter(parameter.ParameterType)
+				&& !app.ImplicitServiceParameters.IsImplicitServiceParameter(parameter.ParameterType)
 				&& parameter.GetCustomAttribute<FromServicesAttribute>() is null
 				&& parameter.GetCustomAttribute<FromContextAttribute>() is null
 				&& !Attribute.IsDefined(parameter.ParameterType, typeof(ReplOptionsGroupAttribute), inherit: true))
@@ -298,19 +298,6 @@ internal ReplDocApp BuildDocumentationApp()
 		return new ReplDocApp(name, version, description);
 	}
 
-	private static bool IsFrameworkInjectedParameter(Type parameterType) =>
-		parameterType == typeof(IServiceProvider)
-		|| parameterType == typeof(ICoreReplApp)
-		|| parameterType == typeof(CoreReplApp)
-		|| parameterType == typeof(IReplSessionState)
-		|| parameterType == typeof(IReplInteractionChannel)
-		|| parameterType == typeof(IReplIoContext)
-		|| parameterType == typeof(IReplKeyReader)
-		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpClientRoots", StringComparison.Ordinal)
-		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpSampling", StringComparison.Ordinal)
-		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpElicitation", StringComparison.Ordinal)
-		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpFeedback", StringComparison.Ordinal);
-
 	private static bool IsRequiredParameter(ParameterInfo parameter)
 	{
 		if (parameter.HasDefaultValue)
diff --git a/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs b/src/Repl.Core/Internal/Options/OptionSchemaBuilder.cs
@@ -9,11 +9,13 @@ internal static class OptionSchemaBuilder
 	public static OptionSchema Build(
 		RouteTemplate template,
 		CommandBuilder command,
-		ParsingOptions parsingOptions)
+		ParsingOptions parsingOptions,
+		ImplicitServiceParameterRegistry implicitServiceParameters)
 	{
 		ArgumentNullException.ThrowIfNull(template);
 		ArgumentNullException.ThrowIfNull(command);
 		ArgumentNullException.ThrowIfNull(parsingOptions);
+		ArgumentNullException.ThrowIfNull(implicitServiceParameters);
 
 		var routeParameterNames = template.Segments
 			.OfType<DynamicRouteSegment>()
@@ -25,7 +27,7 @@ public static OptionSchema Build(
 		var groupPositionalPropertyNames = new List<string>();
 		foreach (var parameter in command.Handler.Method.GetParameters())
 		{
-			if (ShouldSkipSchemaParameter(parameter, routeParameterNames))
+			if (ShouldSkipSchemaParameter(parameter, routeParameterNames, implicitServiceParameters))
 			{
 				continue;
 			}
@@ -57,11 +59,29 @@ public static OptionSchema Build(
 
 	private static bool ShouldSkipSchemaParameter(
 		ParameterInfo parameter,
-		HashSet<string> routeParameterNames)
+		HashSet<string> routeParameterNames,
+		ImplicitServiceParameterRegistry implicitServiceParameters)
 	{
+		var optionAttribute = parameter.GetCustomAttribute<ReplOptionAttribute>(inherit: true);
+		var argumentAttribute = parameter.GetCustomAttribute<ReplArgumentAttribute>(inherit: true);
+		if (implicitServiceParameters.TryGetGlobalOptionsServiceType(parameter.ParameterType, out var globalOptionsType))
+		{
+			if (optionAttribute is not null || argumentAttribute is not null)
+			{
+				var attributeName = optionAttribute is not null
+					? nameof(ReplOptionAttribute).Replace("Attribute", string.Empty, StringComparison.Ordinal)
+					: nameof(ReplArgumentAttribute).Replace("Attribute", string.Empty, StringComparison.Ordinal);
+				throw new InvalidOperationException(
+					$"Parameter '{parameter.Name}' uses typed global options '{globalOptionsType.Name}' registered through "
+					+ $"UseGlobalOptions<T>() and cannot declare [{attributeName}]. Remove the attribute or use a separate command options type.");
+			}
+
+			return true;
+		}
+
 		if (string.IsNullOrWhiteSpace(parameter.Name)
 			|| parameter.ParameterType == typeof(CancellationToken)
-			|| IsFrameworkInjectedParameter(parameter)
+			|| implicitServiceParameters.IsImplicitServiceParameter(parameter.ParameterType)
 			|| parameter.GetCustomAttribute<FromContextAttribute>() is not null
 			|| parameter.GetCustomAttribute<FromServicesAttribute>() is not null)
 		{
@@ -73,8 +93,6 @@ private static bool ShouldSkipSchemaParameter(
 			return false;
 		}
 
-		var optionAttribute = parameter.GetCustomAttribute<ReplOptionAttribute>(inherit: true);
-		var argumentAttribute = parameter.GetCustomAttribute<ReplArgumentAttribute>(inherit: true);
 		if (optionAttribute is null && argumentAttribute is null)
 		{
 			return true;
@@ -190,19 +208,6 @@ private static void AppendValueAliases(
 		}
 	}
 
-	private static bool IsFrameworkInjectedParameter(ParameterInfo parameter) =>
-		parameter.ParameterType == typeof(IServiceProvider)
-		|| parameter.ParameterType == typeof(ICoreReplApp)
-		|| parameter.ParameterType == typeof(CoreReplApp)
-		|| parameter.ParameterType == typeof(IReplSessionState)
-		|| parameter.ParameterType == typeof(IReplInteractionChannel)
-		|| parameter.ParameterType == typeof(IReplIoContext)
-		|| parameter.ParameterType == typeof(IReplKeyReader)
-		|| string.Equals(parameter.ParameterType.FullName, "Repl.Mcp.IMcpClientRoots", StringComparison.Ordinal)
-		|| string.Equals(parameter.ParameterType.FullName, "Repl.Mcp.IMcpSampling", StringComparison.Ordinal)
-		|| string.Equals(parameter.ParameterType.FullName, "Repl.Mcp.IMcpElicitation", StringComparison.Ordinal)
-		|| string.Equals(parameter.ParameterType.FullName, "Repl.Mcp.IMcpFeedback", StringComparison.Ordinal);
-
 	private static ReplArity ResolveArity(ParameterInfo parameter, ReplOptionAttribute? optionAttribute)
 	{
 		if (optionAttribute?.Arity is { } explicitArity)
diff --git a/src/Repl.Core/Parsing/GlobalOptionDefinition.cs b/src/Repl.Core/Parsing/GlobalOptionDefinition.cs
@@ -5,4 +5,5 @@ internal sealed record GlobalOptionDefinition(
 	string CanonicalToken,
 	IReadOnlyList<string> Aliases,
 	string? DefaultValue,
-	Type ValueType);
+	Type ValueType,
+	Type? OwnerType);
diff --git a/src/Repl.Core/Parsing/HandlerArgumentBinder.cs b/src/Repl.Core/Parsing/HandlerArgumentBinder.cs
@@ -53,6 +53,19 @@ internal static class HandlerArgumentBinder
 				$"Unable to bind parameter '{parameter.Name}' ({parameter.ParameterType.Name}).");
 		}
 
+		if (context.ImplicitServiceParameters.TryGetGlobalOptionsServiceType(parameter.ParameterType, out var globalOptionsServiceType))
+		{
+			var globalOptions = context.ServiceProvider.GetService(globalOptionsServiceType);
+			if (globalOptions is not null)
+			{
+				return globalOptions;
+			}
+
+			throw new InvalidOperationException(
+				$"Unable to resolve typed global options parameter '{parameter.Name}' ({globalOptionsServiceType.Name}) from services. "
+				+ $"Ensure the type is registered through UseGlobalOptions<{globalOptionsServiceType.Name}>() before mapping commands.");
+		}
+
 #pragma warning disable IL2072
 		if (IsOptionsGroupParameter(parameter))
 		{
diff --git a/src/Repl.Core/Parsing/ImplicitServiceParameterRegistry.cs b/src/Repl.Core/Parsing/ImplicitServiceParameterRegistry.cs
@@ -0,0 +1,67 @@
+namespace Repl;
+
+internal sealed class ImplicitServiceParameterRegistry
+{
+	private readonly HashSet<Type> _globalOptionsTypes = [];
+
+	public void AddGlobalOptionsType(Type optionsType)
+	{
+		ArgumentNullException.ThrowIfNull(optionsType);
+		_globalOptionsTypes.Add(optionsType);
+	}
+
+	public bool IsImplicitServiceParameter(Type parameterType) =>
+		IsFrameworkInjectedParameter(parameterType)
+		|| TryGetGlobalOptionsServiceType(parameterType, out _);
+
+	public bool TryGetGlobalOptionsServiceType(Type parameterType, out Type serviceType)
+	{
+		ArgumentNullException.ThrowIfNull(parameterType);
+
+		serviceType = typeof(void);
+		if (_globalOptionsTypes.Contains(parameterType))
+		{
+			serviceType = parameterType;
+			return true;
+		}
+
+		if (parameterType == typeof(object))
+		{
+			return false;
+		}
+
+		var matches = _globalOptionsTypes
+			.Where(parameterType.IsAssignableFrom)
+			.OrderBy(static type => type.FullName, StringComparer.Ordinal)
+			.ToArray();
+		if (matches.Length == 0)
+		{
+			return false;
+		}
+
+		if (matches.Length > 1)
+		{
+			throw new InvalidOperationException(
+				$"Ambiguous typed global options binding for parameter type '{parameterType.Name}'. "
+				+ $"Registered matching types: {string.Join(", ", matches.Select(static type => type.Name))}. "
+				+ "Use the concrete registered options type or an explicit [FromServices] parameter.");
+		}
+
+		serviceType = matches[0];
+		return true;
+	}
+
+	private static bool IsFrameworkInjectedParameter(Type parameterType) =>
+		parameterType == typeof(IServiceProvider)
+		|| parameterType == typeof(ICoreReplApp)
+		|| parameterType == typeof(CoreReplApp)
+		|| parameterType == typeof(IGlobalOptionsAccessor)
+		|| parameterType == typeof(IReplSessionState)
+		|| parameterType == typeof(IReplInteractionChannel)
+		|| parameterType == typeof(IReplIoContext)
+		|| parameterType == typeof(IReplKeyReader)
+		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpClientRoots", StringComparison.Ordinal)
+		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpSampling", StringComparison.Ordinal)
+		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpElicitation", StringComparison.Ordinal)
+		|| string.Equals(parameterType.FullName, "Repl.Mcp.IMcpFeedback", StringComparison.Ordinal);
+}
diff --git a/src/Repl.Core/Parsing/InvocationBindingContext.cs b/src/Repl.Core/Parsing/InvocationBindingContext.cs
@@ -12,6 +12,7 @@ internal sealed class InvocationBindingContext(
 	IFormatProvider numericFormatProvider,
 	IServiceProvider serviceProvider,
 	InteractionOptions interactionOptions,
+	ImplicitServiceParameterRegistry implicitServiceParameters,
 	CancellationToken cancellationToken)
 {
 	public IReadOnlyDictionary<string, string> RouteValues { get; } = routeValues;
@@ -33,4 +34,7 @@ internal sealed class InvocationBindingContext(
 	public InteractionOptions InteractionOptions { get; } = interactionOptions;
 
 	public CancellationToken CancellationToken { get; } = cancellationToken;
+
+	public ImplicitServiceParameterRegistry ImplicitServiceParameters { get; } =
+		implicitServiceParameters ?? throw new ArgumentNullException(nameof(implicitServiceParameters));
 }
diff --git a/src/Repl.Core/ParsingOptions.cs b/src/Repl.Core/ParsingOptions.cs
@@ -117,16 +117,16 @@ public void AddGlobalOption<T>(string name, string[]? aliases = null, T? default
 	public void AddGlobalOption(string name, string constraintOrTypeName, string[]? aliases = null, string? defaultValue = null) =>
 		AddGlobalOptionCore(name, ResolveConstraintOrTypeName(constraintOrTypeName, _customRouteConstraints), aliases, defaultValue);
 
-	internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases, string? defaultValue)
+	internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases, string? defaultValue, Type? ownerType = null)
 	{
 		name = string.IsNullOrWhiteSpace(name)
 			? throw new ArgumentException("Global option name cannot be empty.", nameof(name))
 			: name.Trim();
 
 		var normalizedCanonical = NormalizeLongToken(name);
-		if (_globalOptions.ContainsKey(name))
+		if (_globalOptions.TryGetValue(name, out var existing))
 		{
-			throw new InvalidOperationException($"A global option named '{name}' is already registered.");
+			throw new InvalidOperationException(BuildDuplicateGlobalOptionMessage(name, existing.OwnerType, ownerType));
 		}
 
 		var normalizedAliases = (aliases ?? [])
@@ -141,7 +141,24 @@ internal void AddGlobalOptionCore(string name, Type valueType, string[]? aliases
 			CanonicalToken: normalizedCanonical,
 			Aliases: normalizedAliases,
 			DefaultValue: defaultValue,
-			ValueType: valueType);
+			ValueType: valueType,
+			OwnerType: ownerType);
+	}
+
+	private static string BuildDuplicateGlobalOptionMessage(string name, Type? existingOwner, Type? newOwner)
+	{
+		if (existingOwner is null && newOwner is null)
+		{
+			return $"A global option named '{name}' is already registered.";
+		}
+
+		var existingSource = existingOwner is null
+			? "another registration"
+			: $"typed global options '{existingOwner.Name}'";
+		var newSource = newOwner is null
+			? "this registration"
+			: $"typed global options '{newOwner.Name}'";
+		return $"A global option named '{name}' is already registered by {existingSource} and cannot also be registered by {newSource}.";
 	}
 
 	private static Type ResolveConstraintOrTypeName(
diff --git a/src/Repl.Core/Routing/RoutingEngine.cs b/src/Repl.Core/Routing/RoutingEngine.cs
@@ -189,6 +189,7 @@ internal async ValueTask<ContextValidationOutcome> ValidateContextAsync(
 			app.OptionsSnapshot.Parsing.NumericFormatProvider,
 			serviceProvider,
 			app.OptionsSnapshot.Interaction,
+			app.ImplicitServiceParameters,
 			cancellationToken);
 		var arguments = HandlerArgumentBinder.Bind(contextMatch.Context.Validation, bindingContext);
 		var validationResult = await CommandInvoker
@@ -340,6 +341,7 @@ internal async ValueTask InvokeBannerAsync(
 			numericFormatProvider: app.OptionsSnapshot.Parsing.NumericFormatProvider,
 			serviceProvider: serviceProvider,
 			interactionOptions: app.OptionsSnapshot.Interaction,
+			implicitServiceParameters: app.ImplicitServiceParameters,
 			cancellationToken: cancellationToken);
 		var arguments = HandlerArgumentBinder.Bind(banner, bindingContext);
 		var result = await CommandInvoker.InvokeAsync(banner, arguments).ConfigureAwait(false);
diff --git a/src/Repl.Core/Session/InteractiveSession.cs b/src/Repl.Core/Session/InteractiveSession.cs
@@ -545,6 +545,7 @@ private async ValueTask ExecuteCustomAmbientCommandAsync(
 			numericFormatProvider: app.OptionsSnapshot.Parsing.NumericFormatProvider ?? CultureInfo.InvariantCulture,
 			serviceProvider: serviceProvider,
 			interactionOptions: app.OptionsSnapshot.Interaction,
+			implicitServiceParameters: app.ImplicitServiceParameters,
 			cancellationToken: cancellationToken);
 		var arguments = HandlerArgumentBinder.Bind(command.Handler, bindingContext);
 		await CommandInvoker.InvokeAsync(command.Handler, arguments).ConfigureAwait(false);
diff --git a/src/Repl.Defaults/GlobalOptionsExtensions.cs b/src/Repl.Defaults/GlobalOptionsExtensions.cs
@@ -24,26 +24,27 @@ public static class GlobalOptionsExtensions
 	{
 		ArgumentNullException.ThrowIfNull(app);
 
-		ParsingOptions? capturedParsing = null;
+		var parsing = app.Core.OptionsSnapshot.Parsing;
+		var properties = GetOptionProperties<T>();
 		app.Options(options =>
 		{
-			capturedParsing = options.Parsing;
 			var prototype = new T();
-			foreach (var property in GetOptionProperties<T>())
+			foreach (var property in properties)
 			{
 				var optionAttr = property.GetCustomAttribute<ReplOptionAttribute>();
 				var name = optionAttr?.Name ?? ToKebabCase(property.Name);
 				var aliases = optionAttr?.Aliases;
 				var defaultValue = property.GetValue(prototype)?.ToString();
 
-				options.Parsing.AddGlobalOptionCore(name, property.PropertyType, aliases, defaultValue);
+				options.Parsing.AddGlobalOptionCore(name, property.PropertyType, aliases, defaultValue, typeof(T));
 			}
 		});
 
+		app.Core.RegisterGlobalOptionsType(typeof(T));
 		app.ServiceDescriptors.TryAddTransient(sp =>
 		{
 			var accessor = sp.GetRequiredService<IGlobalOptionsAccessor>();
-			return PopulateInstance<T>(accessor, capturedParsing!.NumericFormatProvider);
+			return PopulateInstance<T>(accessor, parsing.NumericFormatProvider);
 		});
 
 		return app;
@@ -74,10 +75,16 @@ public static class GlobalOptionsExtensions
 		return instance;
 	}
 
-	private static PropertyInfo[] GetOptionProperties<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>() =>
-		typeof(T).GetProperties(BindingFlags.Public | BindingFlags.Instance)
-			.Where(p => p.CanWrite)
-			.ToArray();
+	private static IReadOnlyList<PropertyInfo> GetOptionProperties<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>() =>
+		GlobalOptionsMetadata<T>.Properties;
+
+	private static class GlobalOptionsMetadata<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>
+	{
+		internal static readonly IReadOnlyList<PropertyInfo> Properties =
+			typeof(T).GetProperties(BindingFlags.Public | BindingFlags.Instance)
+				.Where(p => p.CanWrite)
+				.ToArray();
+	}
 
 	private static string ToKebabCase(string pascalCase)
 	{
diff --git a/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs b/src/Repl.IntegrationTests/Given_GlobalOptionsAccessor.cs
diff --git a/src/Repl.IntegrationTests/Given_HelpDiscovery.cs b/src/Repl.IntegrationTests/Given_HelpDiscovery.cs
diff --git a/src/Repl.Tests/Given_CommandBuilderEnrichment.cs b/src/Repl.Tests/Given_CommandBuilderEnrichment.cs

Original file line number	Diff line number	Diff line change
`@@ -844,6 +844,7 @@ private InvocationBindingContext CreateInvocationBindingContext(`
`844`	`844`	`_options.Parsing.NumericFormatProvider,`
`845`	`845`	`serviceProvider,`
`846`	`846`	`_options.Interaction,`
	`847`	`+ _implicitServiceParameters,`
`847`	`848`	`cancellationToken);`
`848`	`849`	`}`
`849`	`850`
Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,7 @@ internal sealed class InvocationBindingContext(`
`12`	`12`	`IFormatProvider numericFormatProvider,`
`13`	`13`	`IServiceProvider serviceProvider,`
`14`	`14`	`InteractionOptions interactionOptions,`
	`15`	`+ ImplicitServiceParameterRegistry implicitServiceParameters,`
`15`	`16`	`CancellationToken cancellationToken)`
`16`	`17`	`{`
`17`	`18`	`public IReadOnlyDictionary<string, string> RouteValues { get; } = routeValues;`
`@@ -33,4 +34,7 @@ internal sealed class InvocationBindingContext(`
`33`	`34`	`public InteractionOptions InteractionOptions { get; } = interactionOptions;`
`34`	`35`
`35`	`36`	`public CancellationToken CancellationToken { get; } = cancellationToken;`
	`37`	`+`
	`38`	`+ public ImplicitServiceParameterRegistry ImplicitServiceParameters { get; } =`
	`39`	`+ implicitServiceParameters ?? throw new ArgumentNullException(nameof(implicitServiceParameters));`
`36`	`40`	`}`