-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Implement trimming support for the Interop Type Map #116555
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/illink |
…ttributes. We now trim the type map attributes centrally.
…immer. Only emit it from the analyzer
74ad0a4
to
63c3153
Compare
@@ -90,6 +92,8 @@ internal DynamicallyAccessedMembersTypeHierarchy DynamicallyAccessedMembersTypeH | |||
} | |||
} | |||
|
|||
internal TypeMapHandler TypeMapHandler => _typeMapHandler; |
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 think this is unused
@@ -742,6 +742,11 @@ public bool IsWarningSuppressed (int warningCode, string subcategory, MessageOri | |||
if (Suppressions == null) | |||
return false; | |||
|
|||
// Don't warn for RUC on assembly attributes. | |||
// There's no way to suppress it. | |||
if (warningCode == 2026 && origin.Provider is AssemblyDefinition) |
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.
if (warningCode == 2026 && origin.Provider is AssemblyDefinition) | |
if (warningCode == DiagnosticId.RequiresUnreferencedCode && origin.Provider is AssemblyDefinition) |
|
||
public void SetEntryPointAssembly(AssemblyDefinition asmDef) | ||
{ | ||
Debug.Assert (entry_assembly is null); |
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.
There might be multiple root assemblies. Could we just give all root assemblies this treatment?
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 specifically want to have the "assembly with the entrypoint main method" be the one that roots type map discovery. We don't want to scan type maps for assemblies that are separately fully rooted.
RecordTargetTypeSeen (definition, _unmarkedExternalTypeMapEntries, _referencedExternalTypeMaps, _pendingExternalTypeMapEntries); | ||
} | ||
|
||
void RecordTargetTypeSeen (TypeDefinition targetType, Dictionary<TypeDefinition, Dictionary<TypeReference, List<CustomAttributeWithOrigin>>> unmarkedTypeMapAttributes, HashSet<TypeReference> referenceTypeMapGroups, Dictionary<TypeReference, List<CustomAttributeWithOrigin>> typeMapAttributesPendingUniverseMarking) |
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.
nit: format these with one argument per line
return; | ||
} | ||
if (attr.Attribute.ConstructorArguments is [_, { Value: TypeReference target }]) { | ||
// This is a TypeMapAssemblyTargetAttribute, which has a single type argument. |
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 comment doesn't seem to match the check
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.
Currently it looks like the two forms of the external type map attribute both use the same logic for trimTarget
and target
- is that intentional? My understanding was that the trimTarget
variant was supposed to be the only one that participates in the conditional logic, and the (string, Type target)
variant was supposed to be kept whether or not the target
type is otherwise marked.
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 implemented it in this way with the NativeAOT implementation and updated the design doc (commited in that PR) with the adjusted rule.
I believe that with the expanded rules for including trimTarget
entries, it's impossible to tell the difference in a correctly-written app if the (string, Type)
overload always preserves or only does when the Type
is marked.
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.
If there's an instance of the (string, Type)
attribute with an otherwise unused type, I am expecting the external type mapping to contain this entry. Whereas for an instance of the (string, Type target, Type trimTarget)
overload with an otherwise unused trimTarget, I expect it not to contain this entry. Is that right? @AaronRobinsonMSFT
void AddProxyTypeMapEntry (TypeReference group, CustomAttributeWithOrigin attr) | ||
{ | ||
if (attr.Attribute.ConstructorArguments is [{ Value: TypeReference sourceType }, _]) { | ||
// This is a TypeMapAssociationAttribute, which has a single type argument. |
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.
nit: comment is slightly misleading since the attribute has two type arguments
@@ -13,5 +13,9 @@ public abstract class MarkContext | |||
public abstract void RegisterMarkTypeAction (Action<TypeDefinition> action); | |||
|
|||
public abstract void RegisterMarkMethodAction (Action<MethodDefinition> action); | |||
|
|||
public virtual void RegisterMarkInstantiatedAction (Action<TypeDefinition> action) |
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.
unused I think
Depends on #116355
Completes #110691