CA2263: Prefer generic overload when type is known

[mpidash]

Fixes dotnet/runtime#68243.

Category: Usage
Severity: Info

This analyzer detects when a System.Type overload is called when a suitable generic overload is available and the type is known at compile-time. The fixer replaces it with the generic call and removes redundant casts and parentheses.

To validate if a generic overload is applicable, the following rules are checked:

1. The arity of the generic candidate must match the number of type arguments
2. The number of parameters must match the number of the other arguments (arguments minus type arguments) of the original call.
3. The candidate must not be the same as the enclosing symbol of the original call (see below for why)
4. The return types must be compatible (i.e., the return type of the candidate method must be assignable to the return type of the original call). For expression statements, the return type check is skipped.
5. The other arguments must be compatible (i.e. the other arguments of the original call must be assignable to the parameters of the generic candidate).
6. A speculative binding of the modified call must succeed. This is to check if any generic type constraints are violated.

---

Some notes and corner cases found during testing:

Rule 3. is due to the following case used in the runtime repository:

public static IntPtr OffsetOf<T>(string fieldName) => OffsetOf(typeof(T), fieldName);

If we did not check the enclosing symbol, the fixer would generate an endless recursion.

Rule 4. is due to the following case:

public override ImmutableArray<Type> SyntaxNodeTypes { get; } = ImmutableArray.Create(typeof(NameSyntax));

Here, the intention is to create a ImmutableArray<Type> with one element typeof(NameSyntax). If the analyzer would not check the return value, this would lead to a diagnostic and would be fixed to ImmutableArray.Create<NameSyntax>() which returns an empty ImmutableArray<NameSyntax>.

Rule 6. prevents compile errors due to type constraints, for example:

internal static T[] GetValues<T>() where T : struct
{
    return (T[])Enum.GetValues(typeof(T));
}

We cannot change to return Enum.GetValues<T>() as there is only a struct type constraint, but the generic version of Enum.GetValues also constrains T to the Enum type (in addition to struct).

The tests show all other corner cases that I've encountered during testing.

---

I have one open question: How should we handle different runtime targets? For example the following

internal static T[] Foo<T>() where T : struct, Enum
{
    return (T[])Enum.GetValues(typeof(T));
}

would produce a diagnostic when targeting net7.0, but not when targeting net472, as Enum.GetValues is only available since net5. Is this ok because the default severity is info?

---

I've also added all findings from roslyn-analyzer, roslyn and runtime (set severity to none for tests in runtime, rule ID is still CA2262 in the files):
roslyn-analyzer.txt
roslyn-cs.txt
roslyn-vb.txt
runtime-clr-mono-libs-libs.tests.txt

[codecov]

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (35c0109) 96.44% compared to head (f4bf6c7) 96.45%.

Additional details and impacted files

@@            Coverage Diff            @@
##             main    #6857     +/-   ##
=========================================
  Coverage   96.44%   96.45%             
=========================================
  Files        1415     1422      +7     
  Lines      338229   340539   +2310     
  Branches    11191    11230     +39     
=========================================
+ Hits       326192   328453   +2261     
- Misses       9216     9247     +31     
- Partials     2821     2839     +18     

[buyaa-n]

Rule 3. is due to the following case used in the runtime repository:

Good catch 👍

4. The return types must be compatible (i.e., the return type of the candidate method must be assignable to the return type of the original call). For expression statements, the return type check is skipped.
5. The other arguments must be compatible (i.e. the other arguments of the original call must be assignable to the parameters of the generic candidate).

Shouldn't we restrict to exact match not compatible to avoid false positives? Is there a specific scenario that needs to be compatible?

How should we handle different runtime targets?

By my understanding the analyzer compilation only have the current target info, could not give info for other targets. Now all similar analyzers cause this issue and in runtime repo we if-def the resolution in case the analyzer is set for warning. I'm not aware of any solution for this, @mavasani might know if there is any solution

[buyaa-n]

Wonder if this scenario cause more false positives, why do you think this should be covered?

[mpidash]

IIRC this test was motivated by the following (from roslyn-cs.txt):
W:\roslyn\src\Compilers\CSharp\Test\Semantic\SourceGeneration\GeneratorDriverTests.cs(3102,78): info CA2262: Prefer the generic overload 'System.Enum.GetName(TEnum)' instead of 'System.Enum.GetName(System.Type, object)'

[buyaa-n]

System.Enum.GetName<TEnum>(TEnum) instead of System.Enum.GetName(System.Type, object) is a good example, thank you! We can make it strict if a false positives reported, and the severity is info level, not breaking

[buyaa-n]

The PR looks great, thank you @mpidash!

Test coverage looks good, many hits in runtime, but with a quick check did not see a false positive, the fixer works as expected, good to go