Skip to content
Draft
Show file tree
Hide file tree
Changes from 8 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
122 changes: 111 additions & 11 deletions src/Controls/src/BindingSourceGen/BindingCodeWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,13 @@ public void AppendBindingFactoryMethod(BindingInvocationDescription binding, str
AppendLine('{');
Indent();

// For inaccessible types, use object and cast
var sourceTypeForBinding = GetTypeForBinding(binding.SourceType);
var propertyTypeForBinding = GetTypeForBinding(binding.PropertyType);

// Initialize setter
AppendLines($$"""
global::System.Action<{{binding.SourceType}}, {{binding.PropertyType}}>? setter = null;
global::System.Action<{{sourceTypeForBinding}}, {{propertyTypeForBinding}}>? setter = null;
if ({{GetShouldUseSetterCall(binding.MethodType)}})
{
""");
Expand Down Expand Up @@ -167,10 +171,10 @@ public void AppendBindingFactoryMethod(BindingInvocationDescription binding, str
AppendLine('}');
AppendBlankLine();

// Create instance of TypedBinding
// Create instance of TypedBinding - use object types for inaccessible types
AppendLines($$"""
var binding = new global::Microsoft.Maui.Controls.Internals.TypedBinding<{{binding.SourceType}}, {{binding.PropertyType}}>(
getter: source => (getter(source), true),
var binding = new global::Microsoft.Maui.Controls.Internals.TypedBinding<{{sourceTypeForBinding}}, {{propertyTypeForBinding}}>(
getter: source => ({{GenerateGetterInvocation(binding)}}),
setter,
""");
Indent();
Expand Down Expand Up @@ -200,11 +204,25 @@ public void AppendBindingFactoryMethod(BindingInvocationDescription binding, str
}

AppendUnsafeAccessors(binding);
AppendUnsafeAccessorTypes(binding);

Unindent();
AppendLine('}');
}

private static string GetTypeForBinding(TypeDescription type)
{
// Use object for inaccessible types in TypedBinding
return type.IsAccessible ? type.ToString() : "object";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot is the object sometimes supposed to be object?? or is it always non-nullable (the ? might be added later?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6fe246a. GetTypeForSignature now returns object? when the type is both inaccessible and nullable, otherwise object.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot add snapshot bindingsourcegen tests which would show:

  1. TSource being nullable inaccessible type
  2. TProperty being nullable inaccessible type
  3. Some property in the middle of the path (more than 3 steps) which is nullable inaccessible type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 86c933f. The three new snapshot tests demonstrate:

  1. GenerateBindingWithNullablePrivateSourceType - TSource as nullable private type uses Func<object?, string> and TypedBinding<object?, string>
  2. GenerateBindingWithNullablePrivatePropertyType - TProperty as nullable private type uses object? in UnsafeAccessor parameter
  3. GenerateBindingWithNullablePrivateTypeInMiddleOfPath - 4-step path with nullable private Level2Data in middle uses object? for UnsafeAccessor parameters and return types

All tests verify the correct generation of object? for nullable inaccessible types throughout signatures, TypedBinding instantiation, and UnsafeAccessor methods.

}

private string GenerateGetterInvocation(BindingInvocationDescription binding)
{
// When types are inaccessible, the getter parameter is Func<object, object> (or mixed)
// and we just invoke it directly - no casting needed because the parameter types match
return "getter(source), true";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot this method is unnecessary after all the revisions. just inline the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined in 6fe246a. Changed to single-line expression: private string GenerateGetterInvocation(BindingInvocationDescription binding) => "getter(source), true";


private void AppendFunctionArguments(BindingInvocationDescription binding)
{
AppendLine('(');
Expand All @@ -218,8 +236,17 @@ private void AppendFunctionArguments(BindingInvocationDescription binding)
""");
}

// Use UnsafeAccessorType for inaccessible types
var sourceTypeForSignature = GetTypeForSignature(binding.SourceType);
var propertyTypeForSignature = GetTypeForSignature(binding.PropertyType);

if (!binding.SourceType.IsAccessible && binding.SourceType.AssemblyQualifiedName != null)
{
AppendLine($"[global::System.Runtime.CompilerServices.UnsafeAccessorType(\"{binding.SourceType.AssemblyQualifiedName}\")]");
}

AppendLines($$"""
global::System.Func<{{binding.SourceType}}, {{binding.PropertyType}}> getter,
global::System.Func<{{sourceTypeForSignature}}, {{propertyTypeForSignature}}> getter,
global::Microsoft.Maui.Controls.BindingMode mode = global::Microsoft.Maui.Controls.BindingMode.Default,
global::Microsoft.Maui.Controls.IValueConverter? converter = null,
object? converterParameter = null,
Expand All @@ -232,6 +259,12 @@ private void AppendFunctionArguments(BindingInvocationDescription binding)
Unindent();
}

private static string GetTypeForSignature(TypeDescription type)
{
// Use object for inaccessible types in signatures
return type.IsAccessible ? type.ToString() : "object";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot should this be object? when it is a nullable type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, fixed in 6fe246a. Now uses type.IsNullable ? "object?" : "object" for inaccessible types.

}

private static string GetShouldUseSetterCall(InterceptedMethodType interceptedMethodType) =>
interceptedMethodType switch
{
Expand Down Expand Up @@ -265,6 +298,8 @@ private void AppendSetterLambda(BindingInvocationDescription binding, string sou
AppendLine('{');
Indent();

// No casting needed - when types are inaccessible, parameters are already object
// and UnsafeAccessor methods handle the type conversions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot drop this unnecessary comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 6fe246a.

var assignedValueExpression = valueVariableName;

// early return for nullable values if the setter doesn't accept them
Expand Down Expand Up @@ -331,11 +366,14 @@ private void AppendSetterLambda(BindingInvocationDescription binding, string sou

private void AppendHandlersArray(BindingInvocationDescription binding)
{
AppendLine($"new global::System.Tuple<global::System.Func<{binding.SourceType}, object?>, string>[]");
var sourceTypeForHandlers = GetTypeForBinding(binding.SourceType);
AppendLine($"new global::System.Tuple<global::System.Func<{sourceTypeForHandlers}, object?>, string>[]");
AppendLine('{');

Indent();

// No casting needed - when source type is inaccessible, it's already object
// and we use UnsafeAccessor methods for member access
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot drop thsi unnecessary comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 6fe246a.

string nextExpression = "source";
bool forceConditonalAccessToNextPart = false;
foreach (var part in binding.Path)
Expand Down Expand Up @@ -415,32 +453,38 @@ private void AppendBindingPropertySetters(BindingPropertyFlags propertyFlags)
private void AppendUnsafeAccessors(BindingInvocationDescription binding)
{
// Append unsafe accessors as local methods
var unsafeAccessors = binding.Path.OfType<InaccessibleMemberAccess>();
// Need to unwrap ConditionalAccess to find InaccessibleMemberAccess parts
var unsafeAccessors = binding.Path
.Select(part => part is ConditionalAccess ca ? ca.Part : part)
.OfType<InaccessibleMemberAccess>();

foreach (var unsafeAccessor in unsafeAccessors)
{
AppendBlankLine();

if (unsafeAccessor.Kind == AccessorKind.Field)
{
AppendUnsafeFieldAccessor(unsafeAccessor.MemberName, unsafeAccessor.memberType.GlobalName, unsafeAccessor.ContainingType.GlobalName);
AppendUnsafeFieldAccessorWithType(unsafeAccessor.MemberName, unsafeAccessor.memberType, unsafeAccessor.ContainingType);
}
else if (unsafeAccessor.Kind == AccessorKind.Property)
{
bool isLastPart = unsafeAccessor.Equals(binding.Path.Last());
// Check if this is the last part, unwrapping ConditionalAccess if needed
var lastPart = binding.Path.Last();
var unwrappedLast = lastPart is ConditionalAccess ca ? ca.Part : lastPart;
bool isLastPart = unsafeAccessor.Equals(unwrappedLast);
bool needsGetterForLastPart = binding.RequiresAllUnsafeGetters;

if (!isLastPart || needsGetterForLastPart)
{
// we don't need the unsafe getter if the item is the very last part of the path
// because we don't need to access its value while constructing the handlers array
AppendUnsafePropertyGetAccessors(unsafeAccessor.MemberName, unsafeAccessor.memberType.GlobalName, unsafeAccessor.ContainingType.GlobalName);
AppendUnsafePropertyGetAccessorsWithType(unsafeAccessor.MemberName, unsafeAccessor.memberType, unsafeAccessor.ContainingType);
}

if (isLastPart && binding.SetterOptions.IsWritable)
{
// We only need the unsafe setter if the item is the very last part of the path
AppendUnsafePropertySetAccessors(unsafeAccessor.MemberName, unsafeAccessor.memberType.GlobalName, unsafeAccessor.ContainingType.GlobalName);
AppendUnsafePropertySetAccessorsWithType(unsafeAccessor.MemberName, unsafeAccessor.memberType, unsafeAccessor.ContainingType);
}
}
else
Expand Down Expand Up @@ -469,6 +513,62 @@ private void AppendUnsafePropertySetAccessors(string propertyName, string member
static extern void {{CreateUnsafePropertyAccessorSetMethodName(propertyName)}}({{containingType}} source, {{memberType}} value);
""");

private void AppendUnsafeFieldAccessorWithType(string fieldName, TypeDescription memberType, TypeDescription containingType)
{
var memberTypeStr = memberType.IsAccessible ? memberType.ToString() : "object";
var containingTypeStr = containingType.IsAccessible ? containingType.ToString() : "object";

if (!containingType.IsAccessible && containingType.AssemblyQualifiedName != null)
{
AppendLine($"[global::System.Runtime.CompilerServices.UnsafeAccessor(global::System.Runtime.CompilerServices.UnsafeAccessorKind.Field, Name = \"{fieldName}\")]");
AppendLine($"static extern ref {memberTypeStr} {CreateUnsafeFieldAccessorMethodName(fieldName)}([global::System.Runtime.CompilerServices.UnsafeAccessorType(\"{containingType.AssemblyQualifiedName}\")] {containingTypeStr} source);");
}
else
{
AppendUnsafeFieldAccessor(fieldName, memberTypeStr, containingTypeStr);
}
}

private void AppendUnsafePropertyGetAccessorsWithType(string propertyName, TypeDescription memberType, TypeDescription containingType)
{
var memberTypeStr = memberType.IsAccessible ? memberType.ToString() : "object";
var containingTypeStr = containingType.IsAccessible ? containingType.ToString() : "object";

if (!containingType.IsAccessible && containingType.AssemblyQualifiedName != null)
{
AppendLine($"[global::System.Runtime.CompilerServices.UnsafeAccessor(global::System.Runtime.CompilerServices.UnsafeAccessorKind.Method, Name = \"get_{propertyName}\")]");
AppendLine($"static extern {memberTypeStr} {CreateUnsafePropertyAccessorGetMethodName(propertyName)}([global::System.Runtime.CompilerServices.UnsafeAccessorType(\"{containingType.AssemblyQualifiedName}\")] {containingTypeStr} source);");
}
else
{
AppendUnsafePropertyGetAccessors(propertyName, memberTypeStr, containingTypeStr);
}
}

private void AppendUnsafePropertySetAccessorsWithType(string propertyName, TypeDescription memberType, TypeDescription containingType)
{
var memberTypeStr = memberType.IsAccessible ? memberType.ToString() : "object";
var containingTypeStr = containingType.IsAccessible ? containingType.ToString() : "object";
var valueTypeStr = memberType.IsAccessible ? memberType.ToString() : "object";

if (!containingType.IsAccessible && containingType.AssemblyQualifiedName != null)
{
AppendLine($"[global::System.Runtime.CompilerServices.UnsafeAccessor(global::System.Runtime.CompilerServices.UnsafeAccessorKind.Method, Name = \"set_{propertyName}\")]");
AppendLine($"static extern void {CreateUnsafePropertyAccessorSetMethodName(propertyName)}([global::System.Runtime.CompilerServices.UnsafeAccessorType(\"{containingType.AssemblyQualifiedName}\")] {containingTypeStr} source, {valueTypeStr} value);");
}
else
{
AppendUnsafePropertySetAccessors(propertyName, valueTypeStr, containingTypeStr);
}
}

private void AppendUnsafeAccessorTypes(BindingInvocationDescription binding)
{
// Note: UnsafeAccessorType is applied to the getter parameter in AppendFunctionArguments
// We don't need additional declarations here since the attribute on the parameter
// is sufficient to enable casting from/to object in the method body
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot is this method even needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it was not needed. The method was empty and only contained a comment. Removed in commit 9ce5965.

public void Dispose()
{
_indentedTextWriter.Dispose();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ public sealed record TypeDescription(
string GlobalName,
bool IsValueType = false,
bool IsNullable = false,
bool IsGenericParameter = false)
bool IsGenericParameter = false,
bool IsAccessible = true,
string? AssemblyQualifiedName = null)
{
public override string ToString()
=> IsNullable
Expand Down
5 changes: 1 addition & 4 deletions src/Controls/src/BindingSourceGen/BindingSourceGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,10 +230,7 @@ private static Result<ITypeSymbol> GetLambdaParameterType(LambdaExpressionSyntax
}

var lambdaParamType = parameters[0].Type;
if (!lambdaParamType.IsAccessible())
{
return Result<ITypeSymbol>.Failure(DiagnosticsFactory.UnaccessibleTypeUsedAsLambdaParameter(lambda.GetLocation()));
}
// Now we support inaccessible types using UnsafeAccessorType
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot remove this unnecessary comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 6fe246a.


return Result<ITypeSymbol>.Success(lambdaParamType);
}
Expand Down
4 changes: 2 additions & 2 deletions src/Controls/src/BindingSourceGen/DiagnosticsFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ public static DiagnosticInfo UnaccessibleTypeUsedAsLambdaParameter(Location loca
=> new DiagnosticInfo(
new DiagnosticDescriptor(
id: "BSG0007",
title: "Unaccessible type used as lambda parameter",
messageFormat: "The lambda parameter type has to be declared as public, internal or protected internal.",
title: "Inaccessible type used as lambda parameter",
messageFormat: "The lambda parameter type has to be declared as public, internal or protected internal. Private and protected types cannot be used because the generated binding code is in a different namespace. Consider making the type internal instead of private.",
category: "Usage",
defaultSeverity: DiagnosticSeverity.Error,
isEnabledByDefault: true),
Expand Down
19 changes: 19 additions & 0 deletions src/Controls/src/BindingSourceGen/ISymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,25 @@ internal static bool IsAccessible(this ISymbol symbol) =>
|| symbol.DeclaredAccessibility == Accessibility.Internal
|| symbol.DeclaredAccessibility == Accessibility.ProtectedOrInternal;

// For type symbols, check if the type and all its containing types are accessible
internal static bool IsAccessible(this ITypeSymbol typeSymbol)
{
// Check the type itself
if (!((ISymbol)typeSymbol).IsAccessible())
return false;

// Check all containing types
var containingType = typeSymbol.ContainingType;
while (containingType != null)
{
if (!((ISymbol)containingType).IsAccessible())
return false;
containingType = containingType.ContainingType;
}

return true;
}

internal static AccessorKind ToAccessorKind(this ISymbol symbol)
{
return symbol switch
Expand Down
21 changes: 20 additions & 1 deletion src/Controls/src/BindingSourceGen/ITypeSymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ public static bool IsTypeNullable(this ITypeSymbol typeInfo, bool enabledNullabl
public static TypeDescription CreateTypeDescription(this ITypeSymbol typeSymbol, bool enabledNullable)
{
var isNullable = IsTypeNullable(typeSymbol, enabledNullable);
var isAccessible = typeSymbol.IsAccessible();
return new TypeDescription(
GlobalName: GetGlobalName(typeSymbol, isNullable, typeSymbol.IsValueType),
IsNullable: isNullable,
IsGenericParameter: typeSymbol.Kind == SymbolKind.TypeParameter, //TODO: Add support for generic parameters
IsValueType: typeSymbol.IsValueType);
IsValueType: typeSymbol.IsValueType,
IsAccessible: isAccessible,
AssemblyQualifiedName: isAccessible ? null : GetAssemblyQualifiedName(typeSymbol));
}

private static bool IsNullableValueType(this ITypeSymbol typeInfo) =>
Expand All @@ -43,4 +46,20 @@ private static string GetGlobalName(this ITypeSymbol typeSymbol, bool isNullable

return typeSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
}

private static string GetAssemblyQualifiedName(this ITypeSymbol typeSymbol)
{
// For UnsafeAccessorType, we need assembly-qualified name
// Format: "FullTypeName, AssemblyName"
var typeName = typeSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat)
.Replace("global::", ""); // Remove global:: prefix

var containingAssembly = typeSymbol.ContainingAssembly;
if (containingAssembly != null)
{
return $"{typeName}, {containingAssembly.Name}";
}

return typeName;
}
}
32 changes: 27 additions & 5 deletions src/Controls/src/BindingSourceGen/PathParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,13 @@ private Result<List<IPathPart>> HandleMemberAccessExpression(MemberAccessExpress
var memberType = typeInfo.CreateTypeDescription(_enabledNullable);
var containgType = symbol.ContainingType.CreateTypeDescription(_enabledNullable);

IPathPart part = symbol.IsAccessible()
? new MemberAccess(member, !isReferenceType)
: new InaccessibleMemberAccess(containgType, memberType, accessorKind, member, !isReferenceType);
// If either the member is inaccessible OR the containing type is inaccessible,
// we need to use UnsafeAccessor
bool needsUnsafeAccessor = !symbol.IsAccessible() || !containgType.IsAccessible;

IPathPart part = needsUnsafeAccessor
? new InaccessibleMemberAccess(containgType, memberType, accessorKind, member, !isReferenceType)
: new MemberAccess(member, !isReferenceType);

result.Value.Add(part);
return Result<List<IPathPart>>.Success(result.Value);
Expand Down Expand Up @@ -106,8 +110,26 @@ private Result<List<IPathPart>> HandleMemberBindingExpression(MemberBindingExpre
{
var member = memberBinding.Name.Identifier.Text;
var typeInfo = _context.SemanticModel.GetTypeInfo(memberBinding).Type;
var isReferenceType = typeInfo?.IsReferenceType ?? false;
IPathPart part = new MemberAccess(member, !isReferenceType);
var symbol = _context.SemanticModel.GetSymbolInfo(memberBinding).Symbol;

if (symbol == null || typeInfo == null)
{
return Result<List<IPathPart>>.Failure(DiagnosticsFactory.UnableToResolvePath(memberBinding.GetLocation()));
}

var isReferenceType = typeInfo.IsReferenceType;
var accessorKind = symbol.ToAccessorKind();
var memberType = typeInfo.CreateTypeDescription(_enabledNullable);
var containingType = symbol.ContainingType.CreateTypeDescription(_enabledNullable);

// If either the member is inaccessible OR the containing type is inaccessible,
// we need to use UnsafeAccessor (same logic as HandleMemberAccessExpression)
bool needsUnsafeAccessor = !symbol.IsAccessible() || !containingType.IsAccessible;

IPathPart part = needsUnsafeAccessor
? new InaccessibleMemberAccess(containingType, memberType, accessorKind, member, !isReferenceType)
: new MemberAccess(member, !isReferenceType);

part = new ConditionalAccess(part);

return Result<List<IPathPart>>.Success(new List<IPathPart>([part]));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,10 @@ public void Foo()
[InlineData("protected")]
[InlineData("private protected")]
// https://github.com/dotnet/maui/issues/23534
public void ReportsWarningWhenSourceTypeIsUnaccessible(string modifier)
public void SupportsInaccessibleSourceType(string modifier)
{
// Previously this test checked for BSG0007 error
// Now we support private types using UnsafeAccessorType
var source = $$"""
using Microsoft.Maui.Controls;

Expand All @@ -255,9 +257,9 @@ public void Bar()

var result = SourceGenHelpers.Run(source);

var diagnostic = Assert.Single(result.SourceGeneratorDiagnostics);
Assert.Equal("BSG0007", diagnostic.Id);
AssertExtensions.AssertNoDiagnostics(result.GeneratedCodeCompilationDiagnostics, "Generated code compilation");
// Should not have any diagnostics - private types are now supported
AssertExtensions.AssertNoDiagnostics(result);
Assert.NotNull(result.Binding);
}

[Fact]
Expand Down
Loading