Use DAM binder for MarkEntireType #2206
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This should warn similar to DynamicDependency even though it goes through MarkEntireType.
vitek-karas
requested changes
Aug 12, 2021
Comment on lines
+319
to
+322
| // Calling MarkType here would result in extra warnings for cctors, ctors, delegate methods, | ||
| // serialization ctors, and overrides of preserved abstract methods. | ||
| // Instead, we only do extra type marking not already covered by the other cases in MarkEntireType. | ||
| void MarkTypeOnly (TypeDefinition type, in DependencyInfo reason) |
Member
There was a problem hiding this comment.
This is something we should also solve one day... there are already some cases where it's very hard to prove that marking entire type does the same thing as mark type in the end. For example calling MarkEntireType(typeof(SomeGeneric<TestType>)) will not correctly mark the generic arguments (for example it doesn't apply DAM) - it's just that there are no cases where this happens currently, so it's not a problem.
Obviously for another day...
Comment on lines
+352
to
+354
| case InterfaceImplementation iface: | ||
| MarkInterfaceImplementation (iface, new MessageOrigin (type)); | ||
| break; |
Member
There was a problem hiding this comment.
If I read the code in GetAllOnType correctly, it will not return any interfaces when declaredOnly:true.
Means we're probably missing a test as well.
| } | ||
|
|
||
| MarkGenericParameterProvider (type); | ||
| MarkTypeOnly (type, reason); |
Member
There was a problem hiding this comment.
This comment is about the previous change in this area:
We don't do anything with base type here - it's the behavior we have for copy assemblies, so that's fine, but I think it's worth adding a comment - reading the code it looks really weird.
Side note: the base type will end up being marked in pretty much all cases anyway, since we call MarkMethod which will in turn call MarkType on the declaring type, which will mark the base type as appropriate.
sbomer
added a commit
to sbomer/linker
that referenced
this pull request
Aug 13, 2021
dotnet#2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in dotnet#2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members.
sbomer
added a commit
that referenced
this pull request
Aug 13, 2021
* Fix warnings for DAMT on interfaces #2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in #2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members. * Remove debug output * Apply suggestions from code review Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: Vitek Karas <[email protected]>
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
* Fix warnings for DAMT on interfaces dotnet/linker#2191 addressed issues with redundant warnings from base types, but interfaces need to be handled specially. There were a few issues with the interface handling in that change: - The binding logic didn't include interfaces for DAM.All when declaredOnly was specified, as @vitek-karas pointed out in dotnet/linker#2206 (comment). This could have led to us not marking interfaces for types derived from a DAMT.All-annotated type. Either way, the binding logic should not select interface _members_ when declaredOnly is specified. - The DAMT-on-type logic had special handling for base types, but didn't separately mark interface members, which is needed when applying annotations DAMT.All or DAMT.Interfaces. This could have led to missing interface impls or interface members for types which have (explicit or inherited) DAMT.All or DAMT.Interfaces annotations. Separately, the bit operations on DAMT also didn't correctly handle cases where there are bits shared by different enum members. * Remove debug output * Apply suggestions from code review Co-authored-by: Vitek Karas <[email protected]> Co-authored-by: Vitek Karas <[email protected]> Commit migrated from dotnet/linker@c321df2
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes MarkEntireType to reuse the recursive logic in the DynamicallyAccessedMembers binder, as suggested here: #2191 (comment).
Ideally we could just call MarkType on the type and its nested types, but this would produce extra warnings in copy assemblies since MarkType can cause us to mark extra methods.
The cache is no longer necessary because we usually only call MarkEntireType once for a type. The exception is
PresereveDependencyAttributewhich is deprecated.