Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Microsoft.Mcp.slnx
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<Folder Name="/core/Microsoft.Mcp.Core/tests/">
<Project Path="core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Core.Tests/Microsoft.Mcp.Core.Tests.csproj" />
<Project Path="core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Tests/Microsoft.Mcp.Tests.csproj" />
<Project Path="core/Microsoft.Mcp.Core/tests/TrimmedOptionContainerApp/TrimmedOptionContainerApp.csproj" />
</Folder>
<Folder Name="/core/Microsoft.ModelContextProtocol.HttpServer.Distributed/" />
<Folder Name="/core/Microsoft.ModelContextProtocol.HttpServer.Distributed/src/">
Expand Down
11 changes: 5 additions & 6 deletions core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ public static class OptionBinder
parentInstances ??= [];
if (!parentInstances.TryGetValue(handler.Descriptor.ParentProperty, out var parent))
{
parent = CreateInstance(handler.Descriptor.ParentProperty.PropertyType);
var parentType = handler.Descriptor.ParentType
?? throw new InvalidOperationException("Nested option descriptor is missing its parent type.");
parent = CreateInstance(parentType);
parentInstances[handler.Descriptor.ParentProperty] = parent;
}
handler.Descriptor.TargetProperty.SetValue(parent, value);
Expand Down Expand Up @@ -124,11 +126,8 @@ public static class OptionBinder
return instance;
}

[UnconditionalSuppressMessage("Trimming", "IL2067:UnrecognizedReflectionPattern",
Justification = "Nested option types are rooted by the application via property references.")]
[UnconditionalSuppressMessage("AOT", "IL3050:RequiresDynamicCode",
Justification = "Nested option types use parameterless constructors rooted by the application.")]
private static object CreateInstance(Type type)
private static object CreateInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type type)
{
return Activator.CreateInstance(type)
?? throw new InvalidOperationException($"Failed to create instance of nested options type '{type.Name}'. Ensure it has a public parameterless constructor.");
Expand Down
24 changes: 0 additions & 24 deletions core/Microsoft.Mcp.Core/src/Options/OptionContainerAttribute.cs

This file was deleted.

Comment thread
LarryOsterman marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Diagnostics.CodeAnalysis;

namespace Microsoft.Mcp.Core.Options;

/// <summary>
/// Identifies a complex option container and preserves the members required to discover and bind its options.
/// </summary>
/// <typeparam name="TContainer">The type of the option container.</typeparam>
[AttributeUsage(AttributeTargets.Property, Inherited = true, AllowMultiple = false)]
public sealed class OptionContainerAttribute<
[DynamicallyAccessedMembers(ContainerMembers)] TContainer> : Attribute
{
private const DynamicallyAccessedMemberTypes ContainerMembers =
DynamicallyAccessedMemberTypes.PublicProperties |
DynamicallyAccessedMemberTypes.PublicParameterlessConstructor;

/// <summary>
/// The prefix to use for the options in the container.
/// If null, the property name (in kebab-case) is used as the prefix.
/// </summary>
public string? Prefix { get; init; }

[DynamicallyAccessedMembers(ContainerMembers)]
public Type ContainerType => typeof(TContainer);
}
70 changes: 55 additions & 15 deletions core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,24 @@ public class OptionDescriptor
public required PropertyInfo TargetProperty { get; init; }
public required Type Type { get; init; }
public PropertyInfo? ParentProperty { get; init; }
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
public Type? ParentType { get; init; }

public static OptionDescriptor[] FromType<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>() where T : class
{
List<OptionDescriptor> optionDescriptors = [];
CollectDescriptors(typeof(T), null, optionDescriptors, new(), true, null);
CollectDescriptors(typeof(T), null, optionDescriptors, new(), true, null, null);
return [.. optionDescriptors];
}

[UnconditionalSuppressMessage("Trimming", "IL2070:UnrecognizedReflectionPattern",
Justification = "Nested option types are rooted by the application.")]
private static void CollectDescriptors(
Type type,
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] Type type,
string? prefix,
List<OptionDescriptor> descriptors,
NullabilityInfoContext nullabilityContext,
bool parentRequired,
PropertyInfo? parentProperty)
PropertyInfo? parentProperty,
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type? parentType)
{
PropertyInfo[] allProperties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);

Expand All @@ -60,16 +61,16 @@ private static void CollectDescriptors(
}

var optionAttribute = property.GetCustomAttribute<OptionAttribute>();
var optionContainerAttribute = property.GetCustomAttribute<OptionContainerAttribute>();
// Only include properties with [Option] or [OptionContainer]
if (optionAttribute == null && optionContainerAttribute == null)
var optionContainerAttribute = GetOptionContainerAttribute(property);
// Only include properties with [Option] or [OptionContainer<TContainer>]
if (optionAttribute == null && optionContainerAttribute is null)
{
continue;
}

if (optionAttribute != null && optionContainerAttribute != null)
if (optionAttribute != null && optionContainerAttribute is not null)
{
throw new InvalidOperationException("Properties can only be attributed with [Option] or [OptionContainer], not both.");
throw new InvalidOperationException("Properties can only be attributed with [Option] or [OptionContainer<TContainer>], not both.");
}

var required = Attribute.IsDefined(property, typeof(RequiredMemberAttribute));
Expand All @@ -78,7 +79,7 @@ private static void CollectDescriptors(
{
if (complex)
{
throw new InvalidOperationException("Complex properties cannot use [Option] attribute. Use [OptionContainer] instead.");
throw new InvalidOperationException("Complex properties cannot use [Option] attribute. Use [OptionContainer<TContainer>] instead.");
}
if (!parentRequired && required)
{
Expand All @@ -100,18 +101,35 @@ private static void CollectDescriptors(
DefaultValue = optionAttribute.DefaultValue,
AllowEmptyOrWhiteSpaceString = optionAttribute.AllowEmptyOrWhiteSpaceString,
TargetProperty = property,
ParentProperty = parentProperty
ParentProperty = parentProperty,
ParentType = parentType
});
}

if (optionContainerAttribute != null)
if (optionContainerAttribute is not null)
{
if (!complex)
{
throw new InvalidOperationException("Non-complex properties cannot use [OptionContainer] attribute. Use [Option] instead.");
throw new InvalidOperationException("Non-complex properties cannot use [OptionContainer<TContainer>] attribute. Use [Option] instead.");
}

var containerType = GetOptionContainerType(optionContainerAttribute);
var declaredContainerType = Nullable.GetUnderlyingType(property.PropertyType) ?? property.PropertyType;
if (containerType != declaredContainerType)
{
throw new InvalidOperationException(
$"Option container type '{containerType.Name}' does not match property type '{declaredContainerType.Name}'.");
}

// Flatten nested complex types with a prefix.
CollectDescriptors(property.PropertyType, GetNameOrPrefix(optionContainerAttribute.Prefix, prefix, property.Name), descriptors, nullabilityContext, required, property);
CollectDescriptors(
containerType,
GetNameOrPrefix(GetOptionContainerPrefix(optionContainerAttribute), prefix, property.Name),
descriptors,
nullabilityContext,
required,
property,
containerType);
}
}
}
Expand Down Expand Up @@ -167,6 +185,28 @@ private static bool IsScalarType(Type type)
underlying == typeof(Guid);
}

private static CustomAttributeData? GetOptionContainerAttribute(PropertyInfo property) =>
property.CustomAttributes.SingleOrDefault(attribute =>
attribute.AttributeType.IsGenericType &&
attribute.AttributeType.GetGenericTypeDefinition() == typeof(OptionContainerAttribute<>));

private static string? GetOptionContainerPrefix(CustomAttributeData attribute) =>
attribute.NamedArguments
.SingleOrDefault(argument => argument.MemberName == nameof(OptionContainerAttribute<object>.Prefix))
.TypedValue.Value as string;

// The container type is recovered from the attribute's type argument, so the trimmer cannot see the
// annotation flow here. Preservation is guaranteed instead by the [DynamicallyAccessedMembers] annotation
// on OptionContainerAttribute<TContainer>, which the trimmer applies at every [OptionContainer<T>] usage.
[UnconditionalSuppressMessage("Trimming", "IL2063:UnrecognizedReflectionPattern",
Justification = "OptionContainerAttribute<TContainer> annotates TContainer with PublicProperties and " +
"PublicParameterlessConstructor, so those members are preserved for every option container.")]
[return: DynamicallyAccessedMembers(
DynamicallyAccessedMemberTypes.PublicProperties |
DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
private static Type GetOptionContainerType(CustomAttributeData attribute) =>
attribute.AttributeType.GetGenericArguments()[0];

[UnconditionalSuppressMessage("Trimming", "IL2070:UnrecognizedReflectionPattern",
Justification = "Collection types used in option properties are rooted by the application.")]
private static Type? GetCollectionElementType(Type type)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,17 +123,17 @@ private sealed class NestedOptional
{
[Option(Description = "The name of the item.")]
public string? Name { get; set; }
[OptionContainer]
[OptionContainer<NetworkSettings>]
public NetworkSettings? Optional { get; set; }
[OptionContainer]
[OptionContainer<NetworkSettings>]
public required NetworkSettings Required { get; set; }
}

private sealed class NestedRequired
{
[Option(Description = "The name of the item.")]
public string? Name { get; set; }
[OptionContainer]
[OptionContainer<RequiredNetworkSettings>]
public required RequiredNetworkSettings Required { get; set; }
}

Expand Down Expand Up @@ -168,7 +168,7 @@ private sealed class EmptyOrWhiteSpaceOptions
private sealed class InvalidAttributesCombinationOptions
{
[Option(Description = "Invalid attribute combination.")]
[OptionContainer]
[OptionContainer<NetworkSettings>]
public NetworkSettings? Invalid { get; set; }
}

Expand All @@ -180,10 +180,16 @@ private sealed class InvalidOptionAttributeOptions

private sealed class InvalidOptionContainerAttributeOptions
{
[OptionContainer]
[OptionContainer<string>]
public string? Invalid { get; set; }
}

private sealed class MismatchedOptionContainerOptions
{
[OptionContainer<NetworkSettings>]
public RequiredNetworkSettings? Invalid { get; set; }
}

#endregion

#region RegisterOptions Tests
Expand Down Expand Up @@ -275,7 +281,7 @@ public void RegisterOptions_InvalidAttributes_Throws()
var ex = Assert.Throws<InvalidOperationException>(
() => OptionBinder.RegisterOptions<InvalidAttributesCombinationOptions>(command));

Assert.Contains("Properties can only be attributed with [Option] or [OptionContainer], not both.", ex.Message);
Assert.Contains("Properties can only be attributed with [Option] or [OptionContainer<TContainer>], not both.", ex.Message);
}

[Fact]
Expand All @@ -286,7 +292,7 @@ public void RegisterOptions_InvalidOptionAttribute_Throws()
var ex = Assert.Throws<InvalidOperationException>(
() => OptionBinder.RegisterOptions<InvalidOptionAttributeOptions>(command));

Assert.Contains("Complex properties cannot use [Option] attribute. Use [OptionContainer] instead.", ex.Message);
Assert.Contains("Complex properties cannot use [Option] attribute. Use [OptionContainer<TContainer>] instead.", ex.Message);
}

[Fact]
Expand All @@ -297,7 +303,18 @@ public void RegisterOptions_InvalidOptionContainerAttribute_Throws()
var ex = Assert.Throws<InvalidOperationException>(
() => OptionBinder.RegisterOptions<InvalidOptionContainerAttributeOptions>(command));

Assert.Contains("Non-complex properties cannot use [OptionContainer] attribute. Use [Option] instead.", ex.Message);
Assert.Contains("Non-complex properties cannot use [OptionContainer<TContainer>] attribute. Use [Option] instead.", ex.Message);
}

[Fact]
public void RegisterOptions_MismatchedOptionContainerType_Throws()
{
var command = new Command("test");

var ex = Assert.Throws<InvalidOperationException>(
() => OptionBinder.RegisterOptions<MismatchedOptionContainerOptions>(command));

Assert.Contains("does not match property type", ex.Message);
}

#endregion
Expand Down Expand Up @@ -739,6 +756,14 @@ public void TrimAnnotations_CommandAnnotations_IncludePublicPropertiesAndParamet
Assert.True(annotations.HasFlag(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor));
}

[Fact]
public void OptionContainerAttribute_GenericType_PreservesBindingMembers()
{
var containerType = typeof(OptionContainerAttribute<>).GetGenericArguments()[0];

AssertHasRequiredMemberAnnotations(containerType);
}

private static void AssertHasRequiredMemberAnnotations(MemberInfo member)
{
var attribute = member.GetCustomAttribute<DynamicallyAccessedMembersAttribute>();
Expand Down
Loading
Loading