[CsWinRT 3] ICustomPropertyProvider is not added to all CCWs

[ahmed605]

CsWinRT 3 seems to not add ICustomPropertyProvider to all CCWs under the assumption that XAML will fallback to using Application's IXamlMetadataProvider implementation for binding properties,

But XAML won't actually use any type from the metadata provider as a binding source unless its corresponding IXamlType implementation returns true for IXamlType.IsBindable which is only the case if the type is annotated with [Bindable] attribute.

So there is still a need to have ICustomPropertyProvider automatically added to all CCWs.

This change would require changing [GeneratedCustomPropertyProvider] to generate an implementation for IBindableCustomPropertyImplementation rather than for ICustomPropertyProvider, so basically CsWinRT 2' behavior

the default implementation for ICustomPropertyProvider can use the same behavior of ManagedCustomPropertyProviderVftbl from CsWinRT 2, but an additional IXamlMetadataProvider-based fallback might be needed for non-JIT/NAOT scenarios, example implementation of such fallback (might need some tweaks for CsWinRT 3):

// inside ICustomPropertyProvider.GetCustomProperty impl

var provider = (IXamlMetadataProvider)Application.Current; // probably needs to be invoked through UnsafeAccessor to support both WUX and MUX
if (provider.GetXamlType(target.GetType()) is IXamlType xamlType)
{
    if (xamlType.GetMember(_name) is IXamlMember xamlMember)
    {
        var metadataCustomProperty = new MetadataCustomProperty(xamlMember);
        *result = MarshalInterface<ICustomProperty>.FromManaged(metadataCustomProperty);
        return 0;
    }
}

...

[WinRTExposedType(typeof(CustomPropertyWinRTTypeDetails))]
internal sealed class MetadataCustomProperty : ICustomProperty
{
    private readonly IXamlMember _property;

    public MetadataCustomProperty(IXamlMember propertyMember)
    {
        _property = propertyMember;
    }

    public bool CanRead => true;

    public bool CanWrite => !_property.IsReadOnly;

    public string Name => _property.Name;

    public Type Type => _property.Type.UnderlyingType;

    [DoesNotReturn]
    public object GetIndexedValue(object target, object index)
    {
        throw new NotSupportedException();
    }

    public object GetValue(object target)
    {
        return _property.GetValue(target);
    }

    [DoesNotReturn]
    public void SetIndexedValue(object target, object value, object index)
    {
        throw new NotSupportedException();
    }

    public void SetValue(object target, object value)
    {
        _property.SetValue(target, value);
    }
}

[Sergio0694]

"CsWinRT 3 seems to not add ICustomPropertyProvider to all CCWs under the assumption that XAML will fallback to using Application's IXamlMetadataProvider implementation for binding properties"

Re-reading this I think there's a misunderstanding, that's not why we removed it. The reason why we removed it is because XAML/WinUI has a problematic code path that's triggered when it detects an object implementing ICPP, where it will try to retrieve the tracker host and register a bunch of stuff (there was a long internal discussion about this, it was more complicated than this). The point is, and the conclusion of that conversation was, that we need to stop auto-implementing this on all objects, to avoid triggering those issues in scenarios where objects don't even actually need that. The fact that the auto-implementation was also problematic for trimming is secondary.

So basically we don't plan on auto-implementing ICPP on all objects in 3.0. If ICPP is needed, you should just manually implement it. Or if you want IXMP, then you can manually add [Bindable]. But we won't auto-wire this up (there's also the complexity you mentioned where the code in WinRT.Runtime wouldn't even be able to access e.g. Application.Current, not even with an unsafe accorssor, so working around that would also make things messier).

[dongle-the-gadget]

That would be a massive break for people coming from .NET Native/.NET Framework/CsWinRT 2 + JIT.

[ahmed605]

If ICPP is needed, you should just manually implement it. Or if you want IXMP, then you can manually add [Bindable]

that won't work for platform-agnostic models (models in projects that don't target Windows RID)

The reason why we removed it is because XAML/WinUI has a problematic code path that's triggered when it detects an object implementing ICPP

if the problematic code path is in XAML then I don't think the solution is to not use ICPP but to fix that code path, .NET auto-implemented ICPP on all CCWs since 2011, dropping that now would be a massive breaking change

[Sergio0694]

"That would be a massive break for people coming from .NET Native/.NET Framework/CsWinRT 2 + JIT."

That is by design. That code was never trim-safe in CsWinRT. And CsWinRT 3.0 is AOT-first.

"dropping that now would be a massive breaking change"

Having some targeted breaking changes is part of the point in CsWinRT 3.0. What we plan to do here is to just leave things as they are for now, then port the WinUI 3 Gallery and the Microsoft Store to CsWinRT 3.0, and reassess. My expectation is that in practice this should all be fine, because both of those apps today are already working just fine with AOT in CsWinRT 2.x, meaning that even though every CCW has ICPP, it only works if you manually annotate it. So that would remain the same behavior.

[dongle-the-gadget]

There are many apps that use {Binding} today even in .NET Native, which is AOT.

[Sergio0694]

Yes but my point is that would already not work in CsWinRT 2.x in AOT (without attributes), so 3.0 isn't breaking anything.

[ahmed605]

That code was never trim-safe in CsWinRT

yes but you can make it trim-safe using the method I described in the issue (as in via IXMP)

another thing that I want to mention is that dropping ICPP on all CCWs makes it impossible to bind to external types because you cannot even add attributes to them

[Sergio0694]

I am not following. CsWinRT 2.x doesn't use IXMP as suggested, and all the scenarios mentioned here (including this with external types) are already not working in CsWinRT 2.x with trimming/AOT. Making scenarios that only work in CsWinRT 2.x with JIT and without trimming in 3.0 is explicitly a non-goal. So in practice there's no breaking change being introduced in 3.0 at all here.

We can still look into this in the future, but I'm saying it'd be a separate, opt-in feature proposal, not a "bug fix" or regression.

[dongle-the-gadget]

Making scenarios that only work in CsWinRT 2.x with JIT and without trimming in 3.0 is explicitly a non-goal

I heavily disagree with this. Most people using WinUI 3 do not use Native AOT, and people are already unhappy about UWP .NET 9 "forcing Native AOT" (okay this is technically not true but misleading documentation did that)

Forcing everyone to suffer from Native AOT limitations (which is radically different from .NET Native's "it just works") will make more people mad.

I understand that JIT will not reach full parity with AOT mode in some aspects (binary size for instance), but this doesn't make it acceptable for CsWinRT to penalize users by breaking their workflow for what most of the time isn't even their fault (incompatible libraries).

[ahmed605]

I am not following. CsWinRT 2.x doesn't use IXMP as suggested, and all the scenarios mentioned here

I know that it doesn't use it, what I meant is that you can make it trim-safe by using IXMP instead of the reflection-based code that CsWinRT 2 currently uses.
(I'm not suggesting adding this trim-safe approach to CsWinRT 2, but to CsWinRT 3)