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

Remove BitmapSelector.Suffix property #54364

Merged
merged 1 commit into from
Jun 21, 2021

Conversation

eerhardt
Copy link
Member

With #22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it.

Removing this dead code because:

  1. It doesn't do anything.
  2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings.

I logged dotnet/winforms#8832 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app.

With dotnet#22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it.

Removing this dead code because:

1. It doesn't do anything.
2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings.

I logged https://github.com/dotnet/runtime/issues/54363 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app.
@ghost
Copy link

ghost commented Jun 17, 2021

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

With #22761 and dotnet/corefx#22833, BitmapSelector.Suffix will always be null. This means this feature is dead code, and users are unable to use it.

Removing this dead code because:

  1. It doesn't do anything.
  2. It is causing ILLink warnings, and it is easier to delete than to try to address the warnings.

I logged dotnet/winforms#8832 to follow up and either re-implement this feature, or obsolete the attributes that no longer have any effect on the app.

Author: eerhardt
Assignees: -
Labels:

area-System.Drawing

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Jun 17, 2021

It is causing ILLink warnings, and it is easier to delete than to try to address the warnings.

I am not objecting the changes here as it makes sense. Just wondering, if we decided to implement this feature, do you think the linker can complain at that time again? or it depends on what we'll do during then?

@eerhardt
Copy link
Member Author

if we decided to implement this feature, do you think the linker can complain at that time again? or it depends on what we'll do during then?

The linker was complaining about the following lines:

[RequiresUnreferencedCode("Calls Assembly.GetType which may be trimmed")]
private static bool DoesAssemblyHaveCustomAttribute(Assembly assembly, string typeName)
{
return DoesAssemblyHaveCustomAttribute(assembly, assembly.GetType(typeName));
}
private static bool DoesAssemblyHaveCustomAttribute(Assembly assembly, Type? attrType)
{
if (attrType != null)
{
var attr = assembly.GetCustomAttributes(attrType, false);
if (attr.Length > 0)
{
return true;
}
}
return false;
}
// internal for unit tests
internal static bool SatelliteAssemblyOptIn(Assembly assembly)
{
// Try 4.5 public attribute type first
if (DoesAssemblyHaveCustomAttribute(assembly, typeof(BitmapSuffixInSatelliteAssemblyAttribute)))
{
return true;
}
// Also load attribute type by name for dlls compiled against older frameworks
return DoesAssemblyHaveCustomAttribute(assembly, "System.Drawing.BitmapSuffixInSatelliteAssemblyAttribute");
}
// internal for unit tests
internal static bool SameAssemblyOptIn(Assembly assembly)
{
// Try 4.5 public attribute type first
if (DoesAssemblyHaveCustomAttribute(assembly, typeof(BitmapSuffixInSameAssemblyAttribute)))
{
return true;
}
// Also load attribute type by name for dlls compiled against older frameworks
return DoesAssemblyHaveCustomAttribute(assembly, "System.Drawing.BitmapSuffixInSameAssemblyAttribute");
}

What this was doing was looking for these 2 attributes in the same assembly as the Type. If the public attribute wasn't found, it fell back to looking for the attribute by name (assembly.GetType(typeName)), which is what caused the linker warning. These attributes have existed since .NET 4.5, but this was supporting assemblies built against older frameworks.

If we decided to implement this feature, we could decide to not support looking these attributes up by name in the assembly and only look for our publicly declared attributes. This wouldn't cause any warnings.

Alternatively, the linker will only trim attributes it is explicitly told to trim, for example here. So another approach could be to just suppress the linker warning, and assuming no one is trying to trim these attributes.

@eerhardt eerhardt merged commit 9199eb6 into dotnet:main Jun 21, 2021
@eerhardt eerhardt deleted the RemoveDeadDrawingCode branch June 21, 2021 17:10
@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants