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

Fix DataGrid native aot crash when sorting. #17248

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

dameng324
Copy link
Contributor

@dameng324 dameng324 commented Oct 11, 2024

What does the pull request do?

When sorting datagrid with a non string type field, the application will crash as no metadata of Comparer<int>. This PR fix it.

Fixed: #14059

What is the current behavior?

  • When user does not provide the ViewModel meta data (by [DynamicDependcy] or Rd.xml etc), the application does not crash when sorting int field. but the sort function is broken.

  • When user provide the ViewModel meta data (by [DynamicDependcy] or Rd.xml etc), the application will crash when sorting int field.

What is the updated/expected behavior with this PR?

  • When user does not provide the ViewModel meta data (by [DynamicDependcy] or Rd.xml etc), the application will behave same as before when sorting.

  • When user provide the ViewModel meta data (by [DynamicDependcy] or Rd.xml etc), the application will work normal when sorting int/int?/Enum/Enum? and other type which implements IComparable interface when using aot. Other type which does not implements IComparable should not crash, only does nothing when sorting.

How was the solution implemented (if it's not obvious)?

Old solution:

return (typeof(Comparer<>).MakeGenericType(type).GetProperty("Default")).GetValue(null, null) as IComparer;

My solution:

#if NET6_0_OR_GREATER
if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) && type.GetGenericArguments()[0].IsAssignableTo(typeof(IComparable)))
    return Comparer<object>.Create((x, y) => 
    {
        if (x == null)
            return y == null ? 0 : -1;
        else
            return (x as IComparable)!.CompareTo(y);
    });
else if (type.IsAssignableTo(typeof(IComparable)))
    return Comparer<object>.Create((x, y) => (x as IComparable)!.CompareTo(y));
else
    return Comparer<object>.Create((x, y) => 0);
#else
    return (typeof(Comparer<>).MakeGenericType(type).GetProperty("Default")).GetValue(null, null) as IComparer;
#endif

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixed: #14059

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052600-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@cla-avalonia
Copy link
Collaborator

cla-avalonia commented Oct 11, 2024

  • All contributors have signed the CLA.

@dameng324
Copy link
Contributor Author

@cla-avalonia agree

@dameng324 dameng324 marked this pull request as draft October 11, 2024 14:31
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052602-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

else if (type.IsAssignableTo(typeof(IComparable)))
return Comparer<object>.Create((x, y) => (x as IComparable)!.CompareTo(y));
else
return Comparer<object>.Create((x, y) => 0); //avoid using reflection to avoid crash on AOT
Copy link
Member

Choose a reason for hiding this comment

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

On NET6_0_OR_GREATER you can check for this property whether reflection is supported: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.runtimefeature.isdynamiccodesupported?view=net-8.0

On AOT builds IsDynamicCodeSupported returns false, and this fallback would make sense there.

Copy link
Member

@maxkatz6 maxkatz6 Oct 11, 2024

Choose a reason for hiding this comment

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

This way we also won't regress any application that relies on this reflection sorting right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion. Done.

@dameng324
Copy link
Contributor Author

User can not use sort feature when they does not provide metadata of the type when using aot, Can we do better to avoid user make this kind of mistake? such as add some aot compile warning, or add [DynamicallyAccessedMembersAttribute] or [DynamicDependency] to somewhere? @maxkatz6 do you have any idea on it?

@dameng324 dameng324 marked this pull request as ready for review October 12, 2024 01:35
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.3.999-cibuild0052607-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

@dameng324 we could generate DynamicDependency based on DataGridColumn.SortMemberPath type. See #14085. As an extra attribute on https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls.DataGrid/DataGridColumn.cs#L1124C23-L1124C37

Ideally, AOT apps would utilize CustomSortComparer instead https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Controls.DataGrid/DataGridColumn.cs#L1142, which would reduce reflection usage quite a bit.

Other than this, any refactorings of DataGrid control are not planned.

@dameng324
Copy link
Contributor Author

@maxkatz6 Thanks for the info.

solving this by xaml compiler will be really nice.

@MrJul MrJul added this pull request to the merge queue Oct 14, 2024
Merged via the queue into AvaloniaUI:master with commit 4e19ed8 Oct 14, 2024
10 checks passed
@maxkatz6 maxkatz6 added the backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch label Oct 27, 2024
@dameng324
Copy link
Contributor Author

@maxkatz6 I wonder when will it be published? I didn't see this in the latest 11.2 rc version.

maxkatz6 pushed a commit that referenced this pull request Nov 14, 2024
* Fix DataGrid native aot crash when sorting.

* Update DataGridSortDescription.cs

* add customType test

* does not crash when property type is not sortable.

* check `RuntimeFeature.IsDynamicCodeSupported` to fallback to reflection implemention
@maxkatz6 maxkatz6 added backported-11.2.x and removed backport-candidate-11.2.x Consider this PR for backporting to 11.2 branch labels Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Click [decimal/int32/enum] DataGrid Header crash when publish aot.
5 participants