Analyzer: Detect the problem of using a System.Type overload instead of the generic overload

[buyaa-n]

Related to #67444 (comment)

Passing a System.Type as a parameter is not AOT friendly, when APIs offers overloads with runtime Type parameter and generic type parameter, the former should only used when the type is not known at compile time. But it happens that people use the runtime Type overload using typeof operator when the type is known, using the generic overload in this case will be AOT friendly

Example:

public void M1()
{
    // Detect:
    int size = Marshal.SizeOf(typeof(SampleType));

    // Suggest fix:
    int size = Marshal.SizeOf<SampleType>());
}

Suggested Category: Usage
Suggested Severity: info

CC @carlossanlop

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Related to #67444 (comment)

Passing a System.Type as a parameter is not AOT friendly, when APIs offers overloads with runtime Type parameter and generic type parameter, the former should only used when the type is not known at compile time. But it happens that people use the runtime Type overload using typeof operator when the type is known, using the generic overload in this case will be AOT friendly

Example:

public void M1()
{
    // Detect:
    int size = Marshal.SizeOf(typeof(SampleType));

    // Suggest fix:
    int size = Marshal.SizeOf<SampleType>());
}

Suggested Category: Reliability
Suggested Severity: info

CC @carlossanlop

Author:	buyaa-n
Assignees:	-
Labels:	api-suggestion, area-System.Reflection, code-analyzer, code-fixer
Milestone:	-

[MichalStrehovsky]

We run a dataflow analysis in the compiler for the purposes of reflection so Marshal.SizeOf(typeof(SampleType)) is not a huge problem (we handle this API as an intrinsic and special case in the AOT compiler). The only problem is if someone does int SizeOfWrapper<T>() => Marshal.SizeOf(typeof(T)) - we don't run dataflow analysis on instantiated methods and this case would currently be missed (calling generic SizeOf would fix it). But adding more intrinsics into the AOT compiler is not a great path.

We already have a Roslyn analyzer that warns for this - the RequiresDynamicCode analyzer (the API is marked RequiresDynamicCode). But the analyzer is not enabled by default - only when the user specifies EnableAotAnalyzer. We don't want the analyzer enabled by default because for most people AOT warnings are just noise.

We could add a new analyzer just for this that is enabled by default (because the fix is really simple and everyone should just do it), but it duplicates part of what the existing analyzer already does.

The ideal fix is really not to introduce new APIs that requires this kind of special casing...

Cc @tlakollo

[buyaa-n]

The ideal fix is really not to introduce new APIs that requires this kind of special casing...

From your comment We can now mark AOT unfriendly APIs with the RequiresDynamicCode attribute so if we were to add it, we can at least annotate it as such. I thought you are OK with adding the API 😄. For me as the workaround suggested is also not AOT friendly adding more convenient API than the workaround would be helpful

We could add a new analyzer just for this that is enabled by default (because the fix is really simple and everyone should just do it), but it duplicates part of what the existing analyzer already does.

I was guessing there is a general purpose linker analyzer for this, though I believe this analyzer will be quite different from that one. It will only flag if there is an generic overload with matching parameter count exist and only if the type is created using the typeof operator, plus it has a fixer 😄

CC @eerhardt

[MichalStrehovsky]

I thought you are OK with adding the API 😄

It limits the damage, but it's not great (instead of a runtime exception in a random nuget one cannot debug into, one gets a compile-time warning in the same random nuget that one cannot fix anyway because it's how nuget works).

If we have an analyzer that always flags it, I'm fine with that, just noting the duplication.

[tlakollo]

Current analyzer delivered with the ILLink.RoslynAnalyzer package has the following attributes:

• Analyzer generates a warning every time a method/type has the RequiresDynamicCodeAttribute and is referenced by another member.
• There are two code fixers related to this analyzer, one that suggests marking the caller with RequiresDynamicCodeAttribute, and another one that suggests marking the caller with UnconditionalSuppressMessageAttribute.
• This analyzer is dependent on having the MSBuild parameter EnableAotAnalyzer enabled.
• There is a PR enabling PublishAot, similar to PublishTrimmed it describes that the user has the intent to publish the application as AOT, enabling PublishAot by default enables EnableAotAnalyzer

This new analyzer would be different in almost all the aspects

• Will generate a warning on reference + it has to have a generic safe overload
• The codefixer would act on this new set of rules and suggest a different overload beside the other two options
• The analyzer will be enabled by default and not restained to EnableAotAnalyzer

[buyaa-n]

This new analyzer would be different in almost all the aspects

Completely agree, thank you for explaining how that ILLink analyzer works, good to know

[terrajobst]

Video

Looks good as proposed, that is handling any method, not just Marshal.SizeOf. One thing we need to do is honor the generic constraints so we don't suggest code that wouldn't compile. One way might be to do a speculative binding of the new expression and see whether it produces any diagnostics.

Category: Usage
Severity: Info

[buyaa-n]

Estimates:

• Analyzer: Medium
• Fixer: Medium

[steveharter]

Per triage @buyaa-n moving to Future

[mpidash]

Would be happy to tackle this one next!