Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Mono.Android] fix trimming warnings, part 2 #8758
Changes from 1 commit
2b6a0bf
991674c
6fab46c
4234ebb
2f89efa
c61f027
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.
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
name
would need to be attributed as well, but I'm not sure if that's enough.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.
No, I don't think the trimmer preserves anything here -- this is just ignoring the warning.
This code is looking for a
Resource.designer.cs
file that is generated in every app, which is the root assembly. There will likely be some problem here for NativeAOT one day.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.
I did preserve the attribute that is kind of the "entry point":
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.
Could we add a
Type
property to the attribute?That would be perfectly trimmable and AOT friendly.
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.
We can add a new API in .NET 9, but currently Xamarin.Android class libraries built against
Mono.Android.dll
from ~2012 still work. This is different from iOS, as they have breaking changes.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 comment
The 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
Type
property, and also have appropriate fallback code for whenType
isnull
. (This in turn means that the new property would need to allownull
, e.g.Type? Type {get;set;}
.) We'd also want a "semantically meaningful" name; what isResourceDesignerAttribute.Type
supposed to mean? A better name is warranted.ResourceDesignerType
, perhaps?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
[DynamicallyAccessedMembers-(DynamicallyAccessedMemberTypes.PublicMethods)]
feels contrary to that intention? Why would all methods on theResource
type need to be preserved?Rando aside: why'd we use a string to name the
Resource
type instead oftypeof()
in the first place? A constructor overload feels reasonable here!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.
We used System.Reflection to find an
UpdateIdValues()
method and call it, so I think the current trimmer design is to addDynamicallyAccessedMemberTypes.PublicMethods
?For this part, let me see what it would take to:
Resource.designer.cs
at build timeThere 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.
@jonathanpeppers wrote:
Yes, we used to. Commit dc3ccf2 should have removed that call for most (all?) cases. Yes,
UpdateIdValues()
still exists, but "nothing" should call it (unless some customer decided to manually call it For Reasons™?). We certainly shouldn't call it, and we should be actively removing it, e.g. inClearDesignerClass()
:https://github.com/xamarin/xamarin-android/blob/e873731229274e5382af7672fa4a419f1f1d9bb1/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/LinkDesignerBase.cs#L137
@dellis1972: do we even need
[assembly: ResourceDesigner(…)]
anymore?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.
@dellis1972 replies on Discord:
I overlooked/forgot that the new stuff can be disabled. It's controlled via
$(AndroidUseDesignerAssembly)
.When can we drop support for this? It was added in .NET 8; removing support for
$(AndroidUseDesignerAssembly)
=false from .NET 9 feels too early, maybe? Maybe .NET 10?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.
I'm on with .NET 10 , people should have found all the bugs in the new stuff by then :P