Attribute removal should not kick in for assemblies which are not linked - #1344
Merged
marek-safar merged 2 commits intoJul 10, 2020
Merged
Conversation
MarkStep go over all assemblies (Regardless if they're being linked or just copied) and marks everything as usual. But in the end it will not "trim" anything from the copy assembly. So we must make sure that everything the assembly needs is marked. Attribute removal needs to comply with this behavior as well. This change fixes several other issues around link attribute XML files: - If the XML refers to an attribute which will in turn bring in new assemblies to the closure it would not correctly register all of them - the step needs to behave just like any other step which can bring in new assembly to the closure - Several places in MarkStep didn't use the CustomAttributeSource to detect presense of attributes - meaning that injecting attributes via the XML on a member which had no other (IL based) attributes didn't actually do anything. - Also fixed the annotation's handling of the same issue - presence of custom attributes needs to be determined via the CustomAttributeSource in any place where attributes which are recognized by the linker as special are handled. Added tests for all of the changes above
This was referenced Jul 10, 2020
marek-safar
reviewed
Jul 10, 2020
| void MarkCustomAttributes (ICustomAttributeProvider provider, in DependencyInfo reason, IMemberDefinition sourceLocationMember) | ||
| { | ||
| if (!provider.HasCustomAttributes) | ||
| if (!_context.CustomAttributes.HasAttributes (provider)) |
Contributor
There was a problem hiding this comment.
Wouldn't be better to have if (provider.HasCustomAttributes) { ... } for the section which iterates provider.CustomAttributes. The logic after that will work as it's
| void LazyMarkCustomAttributes (ICustomAttributeProvider provider) | ||
| { | ||
| if (!provider.HasCustomAttributes) | ||
| if (!_context.CustomAttributes.HasAttributes (provider)) |
Contributor
There was a problem hiding this comment.
No need to change this
Member
Author
There was a problem hiding this comment.
You're right - for now... I reverted the change.
Member
|
I pulled this change and verified it fixes the error I am seeing when trying to use the changes in dotnet/runtime#39000 to link a Blazor application. |
marek-safar
approved these changes
Jul 10, 2020
Closed
tkapin
pushed a commit
to tkapin/runtime
that referenced
this pull request
Jan 31, 2023
…ked (dotnet/linker#1344) * Attribute removal should not kick in for assemblies which are not linked MarkStep go over all assemblies (Regardless if they're being linked or just copied) and marks everything as usual. But in the end it will not "trim" anything from the copy assembly. So we must make sure that everything the assembly needs is marked. Attribute removal needs to comply with this behavior as well. This change fixes several other issues around link attribute XML files: - If the XML refers to an attribute which will in turn bring in new assemblies to the closure it would not correctly register all of them - the step needs to behave just like any other step which can bring in new assembly to the closure - Several places in MarkStep didn't use the CustomAttributeSource to detect presense of attributes - meaning that injecting attributes via the XML on a member which had no other (IL based) attributes didn't actually do anything. - Also fixed the annotation's handling of the same issue - presence of custom attributes needs to be determined via the CustomAttributeSource in any place where attributes which are recognized by the linker as special are handled. Added tests for all of the changes above * PR Feedback and test fix Commit migrated from dotnet/linker@f42b2d7
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
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.
MarkStep go over all assemblies (Regardless if they're being linked or just copied) and marks everything as usual. But in the end it will not "trim" anything from the copy assembly. So we must make sure that everything the assembly needs is marked.
Attribute removal needs to comply with this behavior as well.
This change fixes several other issues around link attribute XML files:
Added tests for all of the changes above
Fixes #1341.