-
Notifications
You must be signed in to change notification settings - Fork 528
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
[Mono.Android] fix trimming warnings, part 2 #8758
Changes from all commits
2b6a0bf
991674c
6fab46c
4234ebb
2f89efa
c61f027
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,12 +31,19 @@ public static void UpdateIdValues () | |
} | ||
} | ||
|
||
[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = "Types in Resource.designer.cs are preserved, because it is the root assembly passed to the linker.")] | ||
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
static Type? GetResourceTypeFromAssembly (Assembly assembly) | ||
{ | ||
const string rootAssembly = "Resources.UpdateIdValues() methods are trimmed away by the LinkResourceDesigner trimmer step. This codepath is not called unless $(AndroidUseDesignerAssembly) is disabled."; | ||
|
||
[UnconditionalSuppressMessage ("Trimming", "IL2026", Justification = rootAssembly)] | ||
[UnconditionalSuppressMessage ("Trimming", "IL2073", Justification = rootAssembly)] | ||
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
static Type AssemblyGetType (Assembly a, string name) => a.GetType (name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this really work? I can't see how the trimmer would be able to determine what the returned type is and so it won't be able to preserve the public methods on it. I think at the very least the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think the trimmer preserves anything here -- this is just ignoring the warning. This code is looking for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did preserve the attribute that is kind of the "entry point": [AttributeUsage (AttributeTargets.Assembly)]
public class ResourceDesignerAttribute : Attribute
{
public ResourceDesignerAttribute (string fullName)
public ResourceDesignerAttribute (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
string fullName)
{
FullName = fullName;
}
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public string FullName { get; set; } There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add a [DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] public Type Type { get; set; } That would be perfectly trimmable and AOT friendly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can add a new API in .NET 9, but currently Xamarin.Android class libraries built against So there would be some number of libraries out there that won't use the new API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR is for .NET 9,so we can certainly add the suggested [assembly: Android.Runtime.ResourceDesignerAttribute(
"Example.Android.Resource",
ResourceDesignerType=typeof(Example.Android.Resource),
IsApplication=true
)] That said… the "new" Resource design of dc3ccf2 was thought/hoped to (somewhat?) address trimming support; by using properties/methods for everything the trimmer should be able to "see" what methods are used and preserve them, while allowing everything else to be trimmed away. Setting Rando aside: why'd we use a string to name the partial class ResourceDesignerAttribute {
public ResourceDesignerAttribute (Type resourceDesignerType);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We used System.Reflection to find an
For this part, let me see what it would take to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, we used to. Commit dc3ccf2 should have removed that call for most (all?) cases. Yes, @dellis1972: do we even need There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dellis1972 replies on Discord:
I overlooked/forgot that the new stuff can be disabled. It's controlled via When can we drop support for this? It was added in .NET 8; removing support for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm on with .NET 10 , people should have found all the bugs in the new stuff by then :P |
||
|
||
foreach (var customAttribute in assembly.GetCustomAttributes (typeof (ResourceDesignerAttribute), true)) { | ||
if (customAttribute is ResourceDesignerAttribute resourceDesignerAttribute && resourceDesignerAttribute.IsApplication) { | ||
var type = assembly.GetType (resourceDesignerAttribute.FullName); | ||
var type = AssemblyGetType (assembly, resourceDesignerAttribute.FullName); | ||
if (type != null) | ||
return type; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this reason hold for actual usage?
For example - in OTel, we had a similar issue, and the outcome of the discussion was that it's very important to get something for each frame. So there we rely on .ToString() on the
StackFrame
which works even in NativeAOT/trimming (unless one explicitly disable stack frame info in NativeAOT as a size optimization).I'm not saying this is wrong, just asking where is this used, and if we would like to get at least some info all the time in those scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me investigate if we could just use
StackFrame.ToString()
for this. It's possible we would get the same text, and the tests would pass: 1aa0ea7There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested in a console app, and it appears that
StackFrame.GetMethod()
returnsnull
under NativeAOT. But callingStackFrame.ToString()
returns useful information that contains the method name, example:To support that one day, we could parse the string, but I think delaying that work for now is best.
I left a code comment for now, linking to: