Skip to content
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

Suppress trim warning caused by recent attribute removal changes #62023

Merged
merged 1 commit into from
Nov 26, 2021

Conversation

vitek-karas
Copy link
Member

This fixes a new warning generated by trimming some apps which was introduced in #54056.
The ComVisibleAttribute in this case is referenced, but if it's removed it doesn't change functionality in any way.

This fixes the failure seen in dotnet/sdk#22668 (comment)

This fixes a new warning generated by trimming some apps which was introduced in dotnet#54056.
The `ComVisibleAttribute` in this case is referenced, but if it's removed it doesn't change functionality in any way.
@vitek-karas vitek-karas added the linkable-framework Issues associated with delivering a linker friendly framework label Nov 24, 2021
@vitek-karas vitek-karas added this to the 7.0.0 milestone Nov 24, 2021
@vitek-karas vitek-karas self-assigned this Nov 24, 2021
@ghost
Copy link

ghost commented Nov 24, 2021

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes a new warning generated by trimming some apps which was introduced in #54056.
The ComVisibleAttribute in this case is referenced, but if it's removed it doesn't change functionality in any way.

This fixes the failure seen in dotnet/sdk#22668 (comment)

Author: vitek-karas
Assignees: vitek-karas
Labels:

linkable-framework

Milestone: 7.0.0

@vitek-karas
Copy link
Member Author

I verified that this fixes the SDK test failure by running the test locally with modified dll from runtime.

@@ -69,14 +69,23 @@ internal sealed partial class ReflectTypeDescriptionProvider : TypeDescriptionPr

// These are attributes that, when we discover them on interfaces, we do
// not merge them into the attribute set for a class.
private static readonly Type[] s_skipInterfaceAttributeList = new Type[]
private static readonly Type[] s_skipInterfaceAttributeList = InitializeSkipInterfaceAttributeList();
Copy link
Member

@am11 am11 Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add UnconditionalSuppressMessage to this field directly, without adding InitializeSkipInterfaceAttributeList method? It seems to target all applicable constructs.

[AttributeUsage(AttributeTargets.All, Inherited = false, AllowMultiple = true)]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that won't work. The warning is coming from a static constructor - which in this case is generated by the compiler. Currently the trimmer has no logic to try to map the warning from a static constructor back to the field (not even sure if it's possible in most cases).
First I simply added an explicit static .cctor, but Roslyn doesn't like it - probably due to beforefieldinit optimization which is really only possible when no explicit .cctor is specified. So the static method is a workaround to that.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider just getting rid of the warning in the trimmer. The warning is a half-hearted attempt to tell the user that the trimmer might have broken their app. It doesn't cover many other cases where stripping the attribute might have broken the app (that cannot be detected because there's no typeof). I believe pretty strongly we need to fix https://github.com/dotnet/linker/issues/2078 and then the warning is unnecessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.ComponentModel linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants