Mark facades containing forwarders referenced via type name strings #1854
Conversation
mateoatr
requested review from
marek-safar,
sbomer,
vitek-karas and
MichalStrehovsky
mateoatr
force-pushed
the
dynAccExpTypes
branch
from
39411d5 to
31261d2
Compare
src/linker/Linker/MarkingHelpers.cs
Outdated
| if (reason.Kind == DependencyKind.ModuleOfExportedType) { | ||
| var facadeAssembly = (reason.Source as ModuleDefinition).Assembly; | ||
| TypeDefinition resolvedType = exportedType.Resolve (); | ||
| while (facadeAssembly.FullName != resolvedType.Module.Assembly.FullName) { |
There was a problem hiding this comment.
Wondering if instead of trying to fix the forwarder chain we could get away with marking just the forwarder and then when we're writing out the output, we would "fix" it to point to a thing that exists (so if we're saving a forwarder, we would check whether the thing its pointing to is marked and follow that until we find the marked thing).
That way e.g. referencing System.Object from netstandard won't bring System.Runtime.dll.
There was a problem hiding this comment.
I think we do something almost like this at https://github.com/mono/linker/blob/main/src/linker/Linker.Steps/SweepStep.cs#L683 - it will fix up forwarders so that they point directly at a resolved type - but it won't handle cases where the type can't be resolved. So if you have a -> b -> c -> (unresolved) where a and c are marked forwarders, it won't rewrite a to point to c. But I don't think it will necessarily keep b either - it would be great to add a testcase for this.
src/linker/Linker/MarkingHelpers.cs
Outdated
| if (reason.Kind == DependencyKind.ModuleOfExportedType) { | ||
| var facadeAssembly = (reason.Source as ModuleDefinition).Assembly; | ||
| TypeDefinition resolvedType = exportedType.Resolve (); | ||
| while (facadeAssembly.FullName != resolvedType.Module.Assembly.FullName) { |
There was a problem hiding this comment.
I think we do something almost like this at https://github.com/mono/linker/blob/main/src/linker/Linker.Steps/SweepStep.cs#L683 - it will fix up forwarders so that they point directly at a resolved type - but it won't handle cases where the type can't be resolved. So if you have a -> b -> c -> (unresolved) where a and c are marked forwarders, it won't rewrite a to point to c. But I don't think it will necessarily keep b either - it would be great to add a testcase for this.
Add SweepCollectionExportedTypes
| @@ -1325,7 +1300,8 @@ void MarkEntireAssembly (AssemblyDefinition assembly) | |||
| MarkCustomAttributes (assembly.MainModule, new DependencyInfo (DependencyKind.AssemblyOrModuleAttribute, assembly.MainModule), null); | |||
|
|
|||
| if (assembly.MainModule.HasExportedTypes) { | |||
| // TODO: This needs more work accross all steps | |||
| foreach (var exportedType in assembly.MainModule.ExportedTypes) | |||
| _context.MarkingHelpers.MarkExportedType (exportedType, assembly.MainModule, new DependencyInfo (DependencyKind.ModuleOfExportedType, assembly.MainModule)); | |||
There was a problem hiding this comment.
Same feedback I left in #1781 (comment):
This looks like a significant behavior change:
- We didn't used to keep type forwards in copy assemblies - this could result in some extra facade dlls being kept that weren't needed previously. It's probably uncommon, since facades are usually going to be copyused as part of the framework, but it's unclear how copyused is supposed to behave now. Should a marked copyused assembly also have exported types kept? (This would be a bigger issue since copyused are more likely to pull in facades).
- I think this will prevent SweepStep from sweeping assembly refs from copy assemblies, so it will never update scopes for Copy assemblies. Same question - how should this work for copyused assemblies?
If we go this direction and mark all copy type forwarders, I think we can simplify a few cases in SweepStep:
- get rid of the copy case from UpdateAssembliesReferencingRemovedAssembly
- get rid of the SweepTypeForwarders handling for copy assemblies
There was a problem hiding this comment.
To clarify, I see two options:
- Ensure that copy means copy. The file is not modified, we keep exported types in it, we don't rewrite typerefs - so we need to keep referenced forwards in other assemblies to ensure the typerefs remain valid.
- We keep the behavior that copy has today - it may modify the file to rewrite type refs, sweep unmarked exported types in it - and later introduce an additional option (copyoriginal?) that has the behavior of 1.
There was a problem hiding this comment.
This is trying to accomplish the second option you mention. The copy action should behave the way it does today (mostly) -- the relevant change is we now keep the facade of marked exported types. I'm adding the relevant tests for this.
test/Mono.Linker.Tests.Cases/LinkXml/CanPreserveExportedTypesUsingRegex.cs
Show resolved
Hide resolved
test/Mono.Linker.Tests.Cases/LinkXml/UsedNonRequiredExportedTypeIsKept.cs
Show resolved
Hide resolved
|
Closed in favor of #1869. |
Reachable facade assemblies are currently dropped by the linker even if its forwarders are explicitly referenced in user annotations (note however that the user can work around this by rooting the facades).
This PR changes linker behavior so that type name strings pointing to a forwarded type cause the linker to mark the facade containing the forwarder as well as all other forwarders in the transitive chain pointing to the resolved type.
Fixes #1542
@eerhardt