Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x:Bind does not work with C# record #5315

Open
Marv51 opened this issue Jun 29, 2021 · 17 comments
Open

x:Bind does not work with C# record #5315

Marv51 opened this issue Jun 29, 2021 · 17 comments
Labels
area-Binding area-XamlCompiler bug Something isn't working product-winui3 WinUI 3 issues team-Markup Issue for the Markup team wct

Comments

@Marv51
Copy link
Contributor

Marv51 commented Jun 29, 2021

Describe the bug
I am try to x:bind a c# 9 record to the ItemsSource of an ItemsRepeater, with a corresponding DataTemplate. However, compilation fails with

XamlTypeInfo.g.cs(6465,13,6465,23): error CS8852: Init-only property or indexer 'BlogPost.Title' can only be assigned in an object initializer, or on 'this' or 'base' in an instance constructor or an 'init' accessor.

Steps to reproduce the bug

<ItemsRepeater ItemsSource="{x:Bind BlogPosts}">
    <ItemsRepeater.ItemTemplate>
        <DataTemplate x:DataType="local:BlogPost">
                <TextBlock>{x:Bind Title}</TextBlock>
        </DataTemplate>
    </ItemsRepeater.ItemTemplate>
</ItemsRepeater>
public record BlogPost(string Title, string Teaser, Uri? Url, DateTime Published);

public partial class MyPage: Page, INotifyPropertyChanged
{
    public ObservableCollection<BlogPost> BlogPosts { get; } = new()
    {
        new BlogPost("Test1", "Testing", null, DateTime.Now),
        new BlogPost("Test2", "Testing2", null, DateTime.Now),
        new BlogPost("Test2", "Testing2", null, DateTime.Now),
    };
// shortened
}

Version Info

NuGet package version:
WinUI 3 - Windows App SDK 0.8: 0.8.0

Windows app type:

UWP Win32
Yes
Windows 10 version Saw the problem?
Insider Build (xxxxx)
21H1 (19043) Yes
October 2020 Update (19042)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional context
The error message is referring to this generated method in XamlTypeInfo.g.cs which tries to set a property on a record:

private void set_295_BlogPost_Title(object instance, object Value)
{
    var that = (global::BrandMatrix.BlogPost)instance;
    that.Title = (global::System.String)Value;
}

Init Only Setters also seem to be broken in the same way:

 public class BlogPost {
        public string Title { get; init; }
        //...
    }

Of course private set; can be used as an alternative for now, but support for these features is needed to keep up with .net development. (Or at least a way better error message is needed)

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jun 29, 2021
@JohnnyWestlake
Copy link

I'm confused about why it's trying to call the setter in the first place? Have you set the defaultbindmode to TwoWay?

@Marv51
Copy link
Contributor Author

Marv51 commented Jun 29, 2021

@JohnnyWestlake I have not not set DefaultBindMode. The error occurs when compiling, the setter appears to be generated even though it is not used.

@StephenLPeters StephenLPeters added area-Binding team-Markup Issue for the Markup team labels Jun 29, 2021
@StephenLPeters
Copy link
Contributor

@RealTommyKlein FYI

@Marv51
Copy link
Contributor Author

Marv51 commented Jan 10, 2022

In WinAppSDK 1.0 with dotnet 6.0.1 the situation is still the same.

@hawkerm
Copy link

hawkerm commented Feb 14, 2022

I've created a public readonly record struct in UWP and used the IsExternalInit polyfill method, and it works fine when binding. It seems odd that the generated code is trying to set the title for any reason.

@Marv51 Does the same thing occur if you setup the binding as <TextBlock Text="{x:Bind Title}"/> or make the struct readonly? (FYI you should always bind to the Text property directly to get proper graphical optimizations.)

@Marv51
Copy link
Contributor Author

Marv51 commented Feb 14, 2022

@hawkerm I tested this again just now. <TextBlock Text="{x:Bind Title}"/> results in the exact same error message. Even removing the TextBlock and just keeping the x:DataType results in the wrong code getting generated:

XamlTypeInfo.g.cs:

private void set_9_BlogPost_Title(object instance, object Value)
{
    var that = (global::App1.BlogPost)instance;
    that.Title = (global::System.String)Value;
}

@RealTommyKlein
Copy link
Contributor

Unfortunately this isn't an easy fix on the WinUI side - the metadata loading/reflection the Xaml compiler is pretty dated and can't access custom modifiers for types, and the System.Runtime.CompilerServices.IsExternalInit custom modifier is used to specify init properties and records. Because of this limitation, from the Xaml compiler's POV, the type has a valid public setter so it automatically generates the setter code, even when the setter is actually invalid due to being on a record or init property . We may be able to add better logic for detecting if a property is actually used in markup to mitigate this, but the real fix would be switching the Xaml compiler over to a more modern way of loading metadata, which wouldn't happen for Windows App SDK 1.1.

@Marv51
Copy link
Contributor Author

Marv51 commented Feb 14, 2022

@RealTommyKlein Thank you very much for explaining what is going on here and what the problems are.

@michael-hawker
Copy link
Collaborator

@StephenLPeters @RealTommyKlein I can confirm this works fine in UWP, but with the same setup the XamlTypeInfo generator for WinUI 3 exhibits this issue w/ 1.0.3.

I just hit this building our new sample app in the Toolkit which we're cross-building for UWP and WinUI 3. The UWP head runs fine, but then the WinUI 3 compiler spits out the CS8852 error. I guess I don't understand why the XamlTypeInfo is even bothering to generate setters here at all if the property is only ever being read from in XAML (and not TwoWay bound)? Wouldn't that be the only case where the setter is required anyway?

michael-hawker added a commit to CommunityToolkit/Labs-Windows that referenced this issue Apr 29, 2022
michael-hawker added a commit to CommunityToolkit/Labs-Windows that referenced this issue Apr 29, 2022
michael-hawker added a commit to CommunityToolkit/Labs-Windows that referenced this issue May 4, 2022
michael-hawker added a commit to CommunityToolkit/Labs-Windows that referenced this issue May 4, 2022
@wokhan
Copy link

wokhan commented Jun 16, 2022

Hi there, any update on this? I can of course stop using records and switch back to readonly structs, but that's a bit sad. Thanks!

@michaelmelancon
Copy link

Hi there, any update on this? I can of course stop using records and switch back to readonly structs, but that's a bit sad. Thanks!

Agreed.

@lukasf
Copy link

lukasf commented Nov 19, 2022

Still not working. I'd really like to use record classes. But I can't when running in WindowsAppSdk...

@bpulliam bpulliam removed the needs-triage Issue needs to be triaged by the area owners label Dec 6, 2022
@evelynwu-msft evelynwu-msft added the bug Something isn't working label Jul 11, 2023
@Marv51
Copy link
Contributor Author

Marv51 commented Jul 16, 2023

I think this is still very important (re: #8638)

@minesworld
Copy link

Here too. VERY important as providing a setter would make the messages which MUST be immutable mutable... . To have something "somehow" working I will write setters which will throw an Exception if called twice. But MS should take this issue more serious...

Martin1994 pushed a commit to Martin1994/Labs-Windows that referenced this issue Sep 2, 2023
@dotMorten
Copy link
Contributor

I hit the property initializer issue today as well. That's quite an oversight I'd also like to see improved.

@Youssef1313
Copy link
Contributor

Taking a quick look, I think the fix is to be made here:

protected virtual bool LookupIsWritePublic()
{
MethodInfo setter = Setter;
if (setter != null && !setter.IsPublic)
{
return false;
}
return !IsReadOnly;
}

See dotnet/runtime#43088 (comment) for how to detect init-only using reflection.

This feels like an important scenario.

@adstep
Copy link

adstep commented Aug 30, 2024

Also hit this issue today. Was looking to keep the data structure immutable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Binding area-XamlCompiler bug Something isn't working product-winui3 WinUI 3 issues team-Markup Issue for the Markup team wct
Projects
None yet
Development

No branches or pull requests