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

Invalid trim warning suppression in BindConverter #57016

Open
1 task done
MichalStrehovsky opened this issue Jul 26, 2024 · 0 comments
Open
1 task done

Invalid trim warning suppression in BindConverter #57016

MichalStrehovsky opened this issue Jul 26, 2024 · 0 comments
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. feature-trimming
Milestone

Comments

@MichalStrehovsky
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

ParserDelegateCache.Get method body suppresses trim analysis warnings with the following justification:

[UnconditionalSuppressMessage(
"ReflectionAnalysis",
"IL2060:MakeGenericMethod",
Justification = "The referenced methods don't have any DynamicallyAccessedMembers annotations. See https://github.com/mono/linker/issues/1727")]
[UnconditionalSuppressMessage(
"ReflectionAnalysis",
"IL2075:MakeGenericMethod",
Justification = "The referenced methods don't have any DynamicallyAccessedMembers annotations. See https://github.com/mono/linker/issues/1727")]
public static BindParser<T> Get<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>()

This is a lie. At minimum, the following code:

else if (typeof(T).IsArray)
{
var method = _makeArrayTypeConverter ??= typeof(ParserDelegateCache).GetMethod(nameof(MakeArrayTypeConverter), BindingFlags.NonPublic | BindingFlags.Static)!;
var elementType = typeof(T).GetElementType()!;
parser = (Delegate)method.MakeGenericMethod(elementType).Invoke(null, null)!;
}

is doing MakeGenericMethod on a method that is annotated. I don't see anything obvious that would ensure this invariant holds:

private static BindParser<T[]?> MakeArrayTypeConverter<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>()

Trim warning suppressions:

  • Should be reviewed with the same level of suspicion as unsafe. Assume it's a bug unless proven otherwise.
  • Should never be used on a 130 line method that does tons of reflection. Extract the suppressed block to a small helper method and suppress there.

Expected Behavior

No response

Steps To Reproduce

No response

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-blazor Includes: Blazor, Razor Components label Jul 26, 2024
@MackinnonBuck MackinnonBuck added this to the .NET 10 Planning milestone Jul 29, 2024
@MackinnonBuck MackinnonBuck added bug This issue describes a behavior which is not expected - a bug. feature-trimming labels Jul 29, 2024
@danroth27 danroth27 modified the milestones: .NET 10 Planning, Backlog Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components bug This issue describes a behavior which is not expected - a bug. feature-trimming
Projects
None yet
Development

No branches or pull requests

3 participants