[Trimming] Use resource designer Type in ResourceIdManager#11439
[Trimming] Use resource designer Type in ResourceIdManager#11439simonrozsival wants to merge 23 commits into
Conversation
Emit ResourceDesignerAttribute with typeof(Resource) for app resource designers so ResourceIdManager can use the attribute-provided Type instead of Assembly.GetType(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates how the runtime locates the application Resource designer type by introducing a Type-based ResourceDesignerAttribute API and emitting typeof(Resource) in generated designer code, allowing ResourceIdManager to avoid string-based reflection.
Changes:
- Added
ResourceDesignerAttribute(Type resourceType)plusResourceTypeproperty, and updated PublicAPI baselines. - Updated resource designer generation to emit
typeof(global::...Resource)for application designers (while keeping string form for libraries). - Updated
ResourceIdManagerto use the newResourceTypeproperty.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Xamarin.Android.Build.Tasks/Utilities/ResourceDesignerImportGenerator.cs | Updates comments/documentation for how library designer attributes are discovered. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Expected/GenerateDesignerFileWithLibraryReferenceExpected.cs | Updates expected generated designer output to use typeof(Resource) for application designers. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Expected/GenerateDesignerFileWithElevenStyleableAttributesExpected.cs | Updates expected generated designer output to use typeof(Resource) for application designers. |
| src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Expected/GenerateDesignerFileExpected.cs | Updates expected generated designer output to use typeof(Resource) for application designers. |
| src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs | Emits typeof(Resource) for application assemblies and a string type name for library assemblies. |
| src/Mono.Android/PublicAPI/API-37/PublicAPI.Unshipped.txt | Records new ResourceDesignerAttribute(Type) and ResourceType API as unshipped. |
| src/Mono.Android/PublicAPI/API-36/PublicAPI.Unshipped.txt | Records new ResourceDesignerAttribute(Type) and ResourceType API as unshipped. |
| src/Mono.Android/PublicAPI/API-36.1/PublicAPI.Unshipped.txt | Records new ResourceDesignerAttribute(Type) and ResourceType API as unshipped. |
| src/Mono.Android/PublicAPI/API-35/PublicAPI.Unshipped.txt | Records new ResourceDesignerAttribute(Type) and ResourceType API as unshipped. |
| src/Mono.Android/Android.Runtime/ResourceIdManager.cs | Switches application resource designer lookup from Assembly.GetType(FullName) to ResourceDesignerAttribute.ResourceType. |
| src/Mono.Android/Android.Runtime/ResourceDesignerAttribute.cs | Adds a Type-based constructor and ResourceType property while retaining the string-based constructor. |
simonrozsival
changed the title
Mark the legacy string-based ResourceDesignerAttribute constructor as requiring dynamic code, and emit the Type-based constructor from generated resource designers so internal code avoids the dynamic-code path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep the legacy string constructor behavior in ResourceDesignerAttribute, but route it through a dynamic-code-marked constructor and a localized Assembly.GetType fallback. The Type constructor remains the safe path used by generated resource designers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When ResourceDesignerAttribute is constructed with a Type, ignore assemblies other than the resource type's declaring assembly instead of asserting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The legacy string-based ResourceDesignerAttribute path reaches Assembly.GetType(string), which is annotated RequiresUnreferencedCode. Mark the compatibility constructor accordingly while keeping the Type constructor as the safe generated path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Only return from ResourceIdManager when ResourceDesignerAttribute resolves a non-null resource type, allowing mismatched Type-based attributes to be skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ResourceDesignerAttribute may now be generated with typeof(Resource). When importing resource designer attributes from metadata, use Type.FullName for Type fixed arguments while preserving legacy string arguments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Decode ResourceDesignerAttribute type arguments with a local provider so DummyCustomAttributeProvider keeps its existing behavior while resource designer imports still handle typeof(Resource). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the local ResourceDesignerAttributeTypeProvider and read the serialized fixed attribute argument directly so both string and typeof(Resource) constructors are handled without changing the shared dummy provider. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document why reading the first fixed argument as a serialized string works for both string and System.Type ResourceDesignerAttribute constructors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a build-task test proving ResourceDesignerImportGenerator imports library resources when ResourceDesignerAttribute is emitted with typeof(Resource), and remove the experimental provider strategy from ResourceDesignerAttribute. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
simonrozsival
added
the
copilot
|
/review |
|
✅ Android PR Reviewer completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ LGTM — Clean trimming improvement
Summary: This PR adds a Type-based constructor to ResourceDesignerAttribute so the resource designer can pass typeof(Resource) instead of a string name, enabling the trimmer to statically track the type reference and eliminating Assembly.GetType() at runtime. The old string-based constructor is preserved with [RequiresUnreferencedCode] for backward compatibility.
What I verified:
- ✅ The strategy pattern (
IResourceTypeProvider) cleanly separates the two constructor paths without code duplication - ✅
DynamicallyAccessedMembersannotations are correctly propagated through the type hierarchy - ✅ The raw blob parsing in
ResourceDesignerImportGeneratorcorrectly handles both old (string) and new (typeof) custom attribute encodings, since both are SerString-encoded in ECMA-335 - ✅
LinkDesignerBase.FindResourceDesigner(Cecil-based, not modified) continues to work because Cecil'sTypeReference.ToString()returns the namespace-qualified name, matching the old string format - ✅
TypeResourceTypeProvider.GetResourceTypeFromAssemblyhas a sensible assembly equality check preventing cross-assembly type mismatches - ✅ New test covers the import generator decoding path for the typeof-based attribute
- ✅ PublicAPI files updated for all API levels
- ✅ CI is green
Issue counts: 0 ❌ errors · 1
Generated by Android PR Reviewer for issue #11439 · ● 3.2M
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Decode System.Type custom attribute arguments through DummyCustomAttributeProvider so ResourceDesignerImportGenerator can use the shared GetCustomAttributeArguments() helper for both typeof(Resource) and string ResourceDesignerAttribute constructors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename the private metadata decoder marker to systemTypeSentinel so it is clear the value is only an identity token for DecodeValue() type checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This reverts commit c42f89d.
This reverts commit 99877b0.
Document that legacy ResourceDesignerAttribute string values may be namespace-qualified and suppress the expected IL2026 in the AndroidX NativeAOT code-behind test until transitive binding packages rebuild with the Type constructor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Android PR Reviewer completed successfully! |
There was a problem hiding this comment.
✅ LGTM — Clean, well-structured trimming improvement
Summary: This PR makes ResourceDesignerAttribute trim-safe by generating assembly-qualified names ("Foo.Resource, Foo") instead of bare type names, enabling Type.GetType() to resolve types without trim-unsafe Assembly.GetType() calls. The fallback path for legacy non-AQN bindings is properly preserved with appropriate trim warning suppressions.
Strengths:
- Clean separation: type resolution logic moved into the attribute itself, simplifying
ResourceIdManager - Backward-compatible: old non-AQN format still works via the fallback path
GetTypeFullNameFromAssemblyQualifiedNameis simple, correct, and reused in bothResourceDesignerImportGeneratorandLinkDesignerBase- Good test coverage: expected files updated, new test verifies AQN round-trip through the import generator
- Well-documented temporary
IL2122suppression with clear plan for removal
Minor suggestions only (3 💡): inconsistent brace style, cache field effectiveness note, and edge-case test idea. No blocking issues found.
| Severity | Count |
|---|---|
| ❌ Error | 0 |
| 0 | |
| 💡 Suggestion | 3 |
Generated by Android PR Reviewer for issue #11439 · ● 11.7M
| [return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] | ||
| internal Type? GetResourceTypeFromAssembly (Assembly assembly) | ||
| { | ||
| resourceType ??= Type.GetType (FullName, throwOnError: false); |
There was a problem hiding this comment.
🤖 💡 Performance — The resourceType field is used as a cache via ??=, but Assembly.GetCustomAttributes() creates fresh attribute instances on each call, so this cache is never hit more than once per instance. It's harmless but also doesn't save anything in the current call pattern (GetResourceTypeFromAssembly is called once per attribute instance in ResourceIdManager). If caching was intentional for a future multi-call scenario, a comment would help clarify the intent.
Rule: Remove stale comments / document intent (Postmortem #59)
| if (resourceType is not null) | ||
| { | ||
| if (resourceType.Assembly == assembly) { | ||
| return resourceType; | ||
| } else { | ||
| return null; // no need to fallback to the assembly lookup if the type is found but in a different assembly | ||
| } | ||
| } |
There was a problem hiding this comment.
🤖 💡 Formatting — Inconsistent brace style: the outer if uses Allman style (brace on next line) while the inner if/else uses K&R style (brace on same line). The rest of the file and repo generally uses K&R / Mono style. Consider making this consistent:
| if (resourceType is not null) | |
| { | |
| if (resourceType.Assembly == assembly) { | |
| return resourceType; | |
| } else { | |
| return null; // no need to fallback to the assembly lookup if the type is found but in a different assembly | |
| } | |
| } | |
| if (resourceType is not null) { | |
| if (resourceType.Assembly == assembly) { | |
| return resourceType; | |
| } else { | |
| return null; // no need to fallback to the assembly lookup if the type is found but in a different assembly | |
| } | |
| } |
Rule: Consistent indentation per file (Postmortem #35)
| StringAssert.Contains ("global::Library.Resource.Animator.slide_in_bottom = global::Foo.Foo.Resource.Animator.slide_in_bottom;", designer); | ||
| Directory.Delete (Path.Combine (Root, path), recursive: true); | ||
| } | ||
|
|
There was a problem hiding this comment.
🤖 💡 Testing — Good targeted test for the AQN round-trip! Consider also adding a negative/edge-case assertion that a library with the old non-AQN format (e.g., "Library.Resource" without assembly name) still works, to confirm backward compatibility in the same test or a sibling test. The existing GenerateDesignerFileFromRtxt test may already cover this, but an explicit assertion would make the compat guarantee visible.
Rule: Test edge cases
Summary
ResourceDesignerAttributeconstructor and use it for newly generated resource designer attributesXamarin.Kotlin.Resource, not assembly-qualified names that can satisfy DAM preservationResourceIdManagerto resolve resource designer types through the attribute provider instead of directly callingAssembly.GetType()typeof(Resource)or stringResourceDesignerAttributemetadata continue to import IDsIL2026in the AndroidX code-behind test until transitive binding packages rebuild with the Type constructorPart of #10794.
Binding-library follow-up
Xamarin.AndroidX.AppCompat 1.6.1.6pullsXamarin.Kotlin.StdLib 1.9.21.1.net6.0-android31.0andnet7.0-android33.0Kotlin assemblies contain[assembly: ResourceDesigner("Xamarin.Kotlin.Resource", IsApplication = false)].[DynamicallyAccessedMembers]on the string parameter cannot make the legacy constructor trim-safe.ResourceDesignerAttribute(typeof(...)).IL2026suppression fromCodeBehindTests.Validation
RUNNINGONCI=true ./dotnet-local.sh test bin/TestDebug/net10.0/Xamarin.Android.Build.Tests.dll --filter Name~SuccessfulAndroidXAppXamarin.Kotlin.StdLib 1.9.21.1with ILSpy to confirm the legacy string value isXamarin.Kotlin.Resource