Fix type parsing issues in ILLink and ILC #104060
Conversation
dotnet-issue-labeler
bot
added
the
area-Tools-ILLink
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas |
| { | ||
| CustomAttributeArgument[] arguments = ReadCustomAttributeArguments (nav, attributeType); | ||
| CustomAttributeArgument[] arguments = ReadCustomAttributeArguments (nav, provider); |
There was a problem hiding this comment.
I changed the type resolver to return null when trying to resolve a non-assembly-qualified type name and we didn't know the starting assembly. This uncovered a problem where we were passing the wrong origin here - we were passing the attribute type down into ReadCustomAttributeArgument which expected to receive the attributed member (memberWithAttribute before the diff).
I found the issue because our tests construct a RemoveAttributeInstancesAttribute in memory that doesn't have an associated assembly, so we were passing null to the type resolver and failing to resolve some types specified in the XML.
There was a problem hiding this comment.
Turns out this broke the "library mode" trimming, which was relying on being able to resolve System.String from the same assembly as the attribute type (which we look for in any input assembly). This allowed types to be resolved from System.Runtime in library mode, whereas with this change we only look in System.Private.CoreLib. We probably should clarify the type resolution policy for XML - I'll give it some more thought.
There was a problem hiding this comment.
I made a fix to use System.Runtime as the system assembly in library mode.
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CustomAttributeTypeNameParser.cs
Show resolved
Hide resolved
- Test array/byref/pointer that's not assembly-qualified Also: - Fix ILCompiler.Trimming.Tests build - Clean up usings
- Fix check for array/pointer/byref types, update tests
- Fix ILCompiler.Trimming.Tests build - Replace IL2105 suppressions with IL2122 - Fix trimming tests that were not using fully-qualified type names
| @@ -53,7 +53,7 @@ public DebuggerProxy(MyClass instance) | |||
| } | |||
| } | |||
|
|
|||
| [DebuggerTypeProxy("Program+MyClassWithProxyStringProxy")] | |||
| [DebuggerTypeProxy("Program+MyClassWithProxyStringProxy, project")] | |||
There was a problem hiding this comment.
@eerhardt PTAL. This change will start producing trim warnings for non-assembly-qualified string passed to DAM-annotated locations.
There was a problem hiding this comment.
I think I'm fine with this behavior. I searched a few places that use Type.GetType currently, and they all appear to be using assembly qualified names. If we see new warnings, we should fix them.
There was a problem hiding this comment.
Note that a string passed directly to Type.GetType is ok - the new warning is only for when it's passed to a DAM-annotated location. That's because at that point we don't know where to look up a non-qualified type (since it could flow to Type.GetType in another assembly).
src/coreclr/tools/Common/TypeSystem/Common/Utilities/CustomAttributeTypeNameParser.cs
Outdated
Show resolved
Hide resolved
| // Library mode builds against System.Runtime ref assembly | ||
| Context.SystemModuleName = "System.Runtime"; |
There was a problem hiding this comment.
We should know what the core assembly is (it's the one that defines System.Object). Why is this extra configuration needed? I think I didn't fully understand #104060 (comment).
There was a problem hiding this comment.
We just needed some way to let "library mode" resolve types from System.Runtime (the XML parsing logic relies on it). I think your suggestion to look where System.Object is defined works better, thanks!
- Resolve object to find core library
|
@sbomer is it possible IL2122 might still appear even if
We noticed this here: |
With #104060 there will be trim warnings whenever a non-qualified type name is used with this API, so the call to `_type.AssemblyGetType` is effectively unreachable in a trimmed app with no trim warnings and it is safe to suppress.
Fixes Type.GetType("TypeName") will pick the "first" such type found #98955
We will now produce a warning when a non-assembly-qualified type flows into a string location annotated with DynamicallyAccessedMembers, and we don't try to look up or mark the type (since we don't know which assemblies will be searched at runtime by the Type.GetType call).
Fixes ILLink handling of
Type.GetTypedoesn't resolve types from corelib #103906The ILLink intrinsic handling for
Type.GetTypewill now look in corelib for generic arguments, matching native AOT.This replaces the existing warning IL2105. I left that one alone instead of repurposing it, because we already documented IL2105 and older versions of ILLink will produce it. Best to avoid any confusion about them.