`GetLabelString` does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties'

[Mr-Technician]

Bug type

Component

Component name

MudFormComponent

What happened?

@mikes-gh Pointed out this trimming error .NET7:

Error   IL2075  'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicProperties' in call to 'System.Type.GetProperty(String)'. The return value of method 'System.Linq.Expressions.Expression.Type.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.

https://github.com/MudBlazor/MudBlazor/blob/dev/src/MudBlazor/Extensions/ExpressionExtensions.cs#L41 It also appears here: https://github.com/MudBlazor/MudBlazor/blob/dev/src/MudBlazor/Extensions/ElementReferenceExtensions.cs#L104

Expected behavior

No error :)

Reproduction link

N/A

Reproduction steps

N/A

Relevant log output

No response

Version (bug)

6.0.0.17

Version (working)

No response

What browsers are you seeing the problem on?

Other

On what operating system are you experiencing the issue?

Windows

Pull Request

• [X] I would like to do a Pull Request

Code of Conduct

• [X] I agree to follow this project's Code of Conduct

[mikes-gh]

@Mr-Technician Thanks for starting this issue sorry I didn't get round to it.

       public static string GetLabelString<T>(this Expression<Func<T>> expression)
        {
            var memberExpression = (MemberExpression)expression.Body;
            var propertyInfo = memberExpression.Expression?.Type.GetProperty(memberExpression.Member.Name);
            return propertyInfo?.GetCustomAttributes(typeof(LabelAttribute), true).Cast<LabelAttribute>().FirstOrDefault()?.Name ?? string.Empty;
        }

The problem here is that the built in extension method Expression?.Type cannot satisfy DynamicallyAccessedMemberTypes.PublicProperties

Some how we need to rewrite this routine in a trimming friendly way.

It would be good if we could work together on this in this issue

[Mr-Technician]

It would be good if we could work together on this in this issue

For sure! I haven't dealt with a situation like this before, however.

The problem here is that the built in extension method Expression?.Type cannot satisfy DynamicallyAccessedMemberTypes.PublicProperties

Can you explain why this is the case?

[mikes-gh]

From what I've read we need to use Type(current code) to get to the Child type from the MemberInfo since it defaults to the DeclaringType. But Type isn't trim annotated by MS. Using Member of the expression returns the (base) DeclaringType.

https://stackoverflow.com/questions/9466582/how-to-get-the-child-declaring-type-from-an-expression

Using Member causes

https://github.com/MudBlazor/MudBlazor/blob/47e9738af32343a89a6611abfdec922b5065d6e4/src/MudBlazor.UnitTests/Components/TextFieldTests.cs#L505

to fail since we get the attribute of the base type returned

[mikes-gh]

@ScarletKuro Whats your thoughts on the Type property of Expression. I can't see another way to get the associated real type of the Label Attribute. Perhaps we can pragma it out?

[mikes-gh]

Question asked on StackOverflow

[ScarletKuro]

I did small tests, and this code is safe the way we use it. The custom attribute doesn't get trimmed unless the property is trimmed. The property won't be trimmed if it's used like we do

For="@(() => model.Name)"

The warring happens because the Expression.Type(https://github.com/dotnet/runtime/blob/215b39abf947da7a40b0cb137eab4bceb24ad3e3/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Expression.cs#L97) missing the annotation. There is nothing we can do. But the underlying type of model will not be trimmed because we are using it. Just ignore by now.

UPD: vitek-karas gave more detailed explanation on why it cannot be annotated for Expression.

[ScarletKuro]

The only way to fix it and not rely on the memberExpression.Expression?.Type would require to change the approach. Something like T="SomeModel" For="..." annotate the T on our own and use typeof(T).GetProperty(...). But meh, I'm not fan of this.

[mikes-gh]

. But the underlying type of model will not be trimmed because we are using it. Just ignore by now.

Please see MS issue why we shouldnt rely on type usage for reflection capabilities.

[ScarletKuro]

Yeah, I read that. Not sure if I'm happy with where they are going with the trimming because you can't make everything fully trim compatible: Expression and ValidationContext is a good example. Earlier you could walk on this edge like "I know it's not safe method, but in our scenario it will work". This gets invalid in future or in AOT, and because of 1-2 not trim compatible methods you might require to completely disable trimming which is sad for WASM because 8mb (in case of MudBlazor) vs few kilobytes is a huge deal. Unless they provide much more mechanisms to deal with all this, and I hope they do.

The question is now: what's the plan? Do we keep it as it is for now? Do we care about AOT compilation? Do we remove methods/features like this?(I don't think its a huge deal breaker if we remove the ability to put attributes on model and just leave the parameter "Label" in MudBaseInput) or we try to rewrite to not rely on unsafe methods.

But the biggest concern still stays the validation and the datagrid.

[mikes-gh]

Yes sorry to be so pedantic about it but I really wanted to push the point that just using a type in user code should not be relied upon. Its good to talk to people who have knowledge of this. Theres not a lot of information around. I thought the issue with MS was very informative. I would just like to try to do the "right" solution now even if it works as is now. We have breathing space for this anyway since the type of change they are suggesting would probably be aligned with a new major version. Even then I will presume the new level of trimming will be configurable so it may just be in net8 we set the trimming mode to what net7 was.

I can certainly understand how it would not be easy to annotate Member.Type because it would depend on what you were using Type for. I am not even sure the linker understands how to descend inside the expression anyway.

I think AOT for the whole UI framework is overkill particularly as you then pay a significant cost on size and probably we don't need the extra speed. However who knows what the future of Blazor will be. Maybe it will all be compiled to native wasm at some stage.

I don't think we are far off in the library (DataGrid aside). There are however some suppressions that assume instantiating means types and properties can be reflected upon that may need to be revisited.

You will notice that all the PRs I did were all annotating DAM which sometimes is a bit more thinking but gives the best solution now and in the future.

I also noted that the analyser did not pick up https://github.com/MudBlazor/MudBlazor/pull/5711 Thank you for trimming the Docs as that picked it up fairly quickly.

DataGrid is a problem however. It is very loose with its configuration often using strings and reflection where I am sure there are better type safe solutions.

You could convert DataGrid to a non trimmable extension maintained by the author since any changes that make it more trim friendly are likely to be breaking.

That would mean writing a new grid though which is too much work for most people. Or we could partially redesign the column bindings so they were trim friendly. I particularly like QuickGrid by Steve Sanderson as a starting point.

I have still not given up on the Validation and Label problem We still haven't explored a code generator solution.

Another idea I am exploring

Expression<Func<TModel [DymanicallyAccesedMembers(DynamicallyAccessedMemberTypes.PublicProperties) ,T>> expression

With the configuration

For= "model => model.SomeProp"

and doing typeof(TModel) or something probably its a dead end I havent really started yet

Thanks for listening :-)

[ScarletKuro]

@mikes-gh i wonder, how important is this feature in general, and how important is the fact that it works with inherited model? Can we use an alternative with the DisplayAttribute and the new source-generator to get the value, tho it will not work with derived class.

[Mr-Technician]

@ScarletKuro IMO it's important that it works with inherited models. I have several model classes that share a common base class and it's useful to be able to pull a label from any property.

The LabelAttribute was created as an alternative to DisplayAttribute, I believe because some users were using DisplayAttribute already in a way that caused conflict. I'd need to find the specific issue.

Using a source generator would completely defeat the purpose of fetching the Label automatically, would it not?

[ScarletKuro]

Using a source generator would completely defeat the purpose of fetching the Label automatically, would it not?

It would eliminate the need for GetLabelString (it's impossible to make this method trimmable / linker friendly). MS Linker Team recommended to use SourceGenerator, in fact this is all 'static' information. But we would need to make a new source generator compared to this one #6306 since this only works only with enums. In general, we would need 3 things:

1. It must work with classes.
2. It must take into account inherited models.
3. We need to release it as separate nuget package, because it's the end user who annotates their models with the LabelAttribute

[mikes-gh]

Id be interested to see the source generator solution. As you say the GetLabelString is final trimming incompatible method.

[Mr-Technician]

Ok, point 3 clarifies it for me. Is there any chance we could add an attribute that users could decorate their classes with without adding a separate nuget?

[ScarletKuro]

Ok, point 3 clarifies it for me. Is there any chance we could add an attribute that users could decorate their classes with without adding a separate nuget?

You can have the attribute in the main library, but the code won't be generated unless you explicitly add XYZ.SourceGenerator to your project. Well, I know that ComputeSharp and CommunityToolkit.Mvvm (same author) bundles somehow the SourceGenerator in the main library, but I haven't found much information about it on the internet. I tried to look at the structure of these projects, but it looks complicated. The only insights I found is this comment:

[ScarletKuro]

@mikes-gh after almost a year I think we should just remove this part of feature where it support the virtual -> override with different labels on the same property. For validation etc, you use your DTO, a clear class without logic, methods etc, maximum attributes, you just don't use virtual and override there in general, and even if you have inheritance why would you want to have different labels? I find it very useless feature that makes life harder.