Skip to content

Commit

Permalink
Fix partial trimming of assemblies on referenced in metadata
Browse files Browse the repository at this point in the history
runtime-extra-platforms are on the floor again after the rebrand to .NET 9 TFM PR.

They're failing with an obscure:

```
Unhandled exception. System.InvalidOperationException: Unable to get member, please check input for IsNativeAot.
   at Microsoft.DotNet.XUnitExtensions.DiscovererHelpers.Evaluate(Type calleeType, String[] conditionMemberNames) + 0xb6
   at Microsoft.DotNet.XUnitExtensions.DiscovererHelpers.<EvaluateArguments>d__13.MoveNext() + 0x209
```

This is because `PlatformDetection.IsNativeAot` is not visible from reflection.

Root caused this to:

* `PlatformDetection` type is only referenced from `ConditionalFact` attribute. This attribute is not annotated with dataflow annotations, so the only thing we keep is the _metadata_ for the `PlatformDetection` type, since there's nothing else to keep (no methods, no MethodTable).
* This is the _only_ reference to something from TestUtilities assembly.
* Before the 9.0 rebrand PR, some nullable junk attributes were also generated into TestUtilities assembly. This basically helped us get _some runtime artifacts_ for _something_ in TestUtilities to be generated - when we were scanning attributes on PlatformDetection we'd say: oh this attribute needs a methodtable and a body for the constructor. This in turn triggered partial trimming logic to make _everything_ in TestUtilities reflection rooted.
* After the 9.0 rebrand PR those attributes live in CoreLib, so nothing in TestUtilities gets a runtime artifact. Partial trimming logic is not looked at from TypeMetadata node.

I'm fixing it by forcing a MethodTable whenever we have metadata for a `<Module>` type (this type is always generated).

But also I'm indifferent about this fix. There are many similar corner cases where we'd miss that an assembly was used (e.g. declare a local of some type and then never do anything with it). I don't think we need to fix them. This could also be fixed by just rooting TestUtilities in our testing infra. I'm open to that too.
  • Loading branch information
MichalStrehovsky committed Sep 7, 2023
1 parent cc9fcf1 commit 9f2079f
Showing 1 changed file with 12 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ public override IEnumerable<DependencyListEntry> GetStaticDependencies(NodeFacto
}
}

if (_type.IsModuleType)
{
// Include a runtime artifact when metadata for module is created.
// This triggers other "module use" logic such as module initializers or partial trimming.
// This is our insurance in case the only thing from a module that was used was its metadata.
// This can pretty much only happen if the only reference to the module is a `typeof` in
// a custom attribute and the custom attribute doesn't have any dataflow annotations on it
// that would bring a reflectable runtime artifact from the type. The only thing we'd generate
// is the metadata for the type, but no MethodTable or method body.
dependencies.Add(factory.ReflectedType(_type), "Reflectable <Module> type");
}

// If the user asked for complete metadata to be generated for all types that are getting metadata, ensure that.
if ((mdManager._generationOptions & UsageBasedMetadataGenerationOptions.CompleteTypesOnly) != 0)
{
Expand Down

0 comments on commit 9f2079f

Please sign in to comment.