Using Compiled Bindings to Properties of a Record Type Doesn't Fail Until Runtime

[colejohnson66]

Describe the bug If I use a compiled binding to a property of a record type, the compilation succeeds, only to get an InvalidProgramException at runtime when Avalonia attempts to set the property value.

To Reproduce

public record TestRecord(string Prop);

public class TestViewModel : ViewModelBase
{
    private TestRecord _testObj = new("");
    public TestRecord TestObj
    {
        get => _testObj;
        set = this.RaiseAndSetIfChanged(ref _testObj, value, nameof(TestObj));
    }
}

...
    <TextBox Text="{CompiledBinding TestObj.Prop}" />
...

Expected behavior This would refuse to compile.

Actual behavior It compiles, and Avalonia generates this IL at CompiledAvaloniaXaml.XamlIlHelpers.{path}.Name!Setter(object, object):

  .method compilercontrolled static void
    '{path}.Name!Setter'(
      [in] object obj0,
      [in] object obj1
    ) cil managed
  {
    .maxstack 2

    IL_0000: ldarg.0      // obj0
    IL_0001: castclass    {object type}
    IL_0006: ldarg        obj1
    IL_000a: castclass    [System.Runtime]System.String
    IL_000f: callvirt     instance void modreq ([System.Runtime]System.Runtime.CompilerServices.IsExternalInit) {path}::set_Name(string)
    IL_0014: pop
    IL_0015: ret

  } // end of method XamlIlHelpers::'{path}.Name!Setter'

After the first two opcodes, there's an object on the stack. The next two push a string onto the stack. Then the callvirt pops those two, leaving the stack empty. Then Avalonia threw in a random pop that would make a stack underflow before returning. Because of that random pop opcode, the runtime refuses to run that function and dotPeek refuses to decompile it.

Screenshots

Desktop

• OS: Windows 11 and Raspbian Buster (not that it matters)
• Version 0.10.15

Additional context Roslyn apparently generates public setters for record types, which Avalonia was able to bind to. Hence, why the compilation succeeded.

[colejohnson66]

Changing the record type to a class type with public getters and setters (obviously) fixes the exception.

[maxkatz6]

It might require changes in XamlX repo, if anybody wants to pick it up https://github.com/kekekeks/XamlX

[colejohnson66]

The emission of the pop opcode is indeed happening inside XamlX:

https://github.com/kekekeks/XamlX/blob/d990d63774a04d2a4b3d52e626a90ee68e19e2b6/src/XamlX/IL/XamlILEmitterExtensions.cs#L23-L24

if (swallowResult && !(method.ReturnType.Namespace == "System" && method.ReturnType.Name == "Void"))
    emitter.Pop();

[MrJul]

The problem appears for init properties (which records use). Minimal repro:

public class TestClass { public string Prop { get; init; } }

<TestClass Prop="foo" />

It currently works with System.Reflection.Emit (loading xaml files at runtime) but not Cecil (build time). Making it work for Cecil seems easy enough: ReturnType.Name is modreq(IsExternalInit) void instead of void in this case, taking the underlying type if there's a modreq/modopt should be enough. I can open a PR for that.

But if this runs, init setters then can be set several times, and they probably shouldn't. IMO, an ideal solution would be to disallow two-way bindings (both compiled and reflection ones) to init properties, and only allow them in XAML setters when an object is being created, to mimic C# behavior. That can be considered a new feature though.

[colejohnson66]

Ah, that would explain why there's a setter. You're right about them being modifiable; a record is supposed to be immutable (barring tricks like reflection). For me, I don't think preventing binding to init only properties would be a feature, but a bugfix. Sure, it's a PEBKAC, but it's still a bug IMO.

[MrJul]

I've opened a PR to at least fix the InvalidProgramException, so compiled bindings don't crash and work the same way as reflection bindings do currently.

Blocking init two-way/one-way-to-source bindings is another task, and if done, should be done both for compiled and reflection bindings. It depends on what the project maintainers think about it.

[colejohnson66]

I see the PR in XamlX has been merged. Should we close this now?

[timunie]

You can double check in latest master and if it's fixed there, it's good to close I guess.

[kekekeks]

Note that merging something to XamlX doesn't automatically include the new version into Avalonia

[colejohnson66]

Hence why I asked. It seems Avalonia's master is on a4e6be2d dated June 30th. I'll leave it open until the submodule commit reference is updated, I guess.

[colejohnson66]

This appears to have been fixed in 9acb2a11 (the 11.0-preview1 tag), so I'm gonna mark this closed now.