Update dependencies from dotnet/roslyn-analyzers, config new analyzers

[buyaa-n]

• Reapply Update dependencies from dotnet/roslyn-analyzers
• Configure new analyzers severity in src and test projects

Related to #99327

[ghost]

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

Issue Details

---

• Reapply Update dependencies from dotnet/roslyn-analyzers
• Configure new analyzers severity in src and test projects

Related to #99327

Author:	buyaa-n
Assignees:	buyaa-n
Labels:	area-Infrastructure-libraries
Milestone:	-

[stephentoub]

Is there a problem / difficulty with this diagnostic that prevents us from having it be a warning?

[buyaa-n]

There is a some cases where the suggested API is calling the type overload internally, I assume we would not like to flag them, let me know if you have other suggestion. There also some that looks could be more efficient and should be fixed, I will update them with this PR, overall around 200 findings with duplicates
CA2263 findings.txt

[stephentoub]

I think we should go through and prepare the change to fix them, and then evaluate any cases we believe are problematic. Two reasons:

1. One of the reasons we aggressively consume new analyzers into dotnet/runtime is to vet the new analyzers. dotnet/runtime is a large codebase, and is a good first line of defense against problems with an analyzer's implementation or even high-level goal. It'd be good to understand which of those 200 warnings are real things to be fixed and which are noise that might demand changes to the analyzer.
2. We're hopefully only adding analyzers we believe lead to making code better. We want that betterness in dotnet/runtime.

[stephentoub]

(That can/should certainly be separate from this PR.)

[buyaa-n]

I think we should go through and prepare the change to fix them, and then evaluate any cases we believe are problematic. Two reasons:

Sure, I did evaluate them and concluded that those are not issue with the analyzer and also not worth fixing in the runtime, here is the reasons, please let me know if you have any comment/suggestion:
Excluded these cases from fixing:

1. Excluded fixing the cases when the project builds for netstandard or .NET framework where the generic overload doesn't exit
2. Cases where the generic overload implementation just calling the Type overload, like: public T CreateDelegate<T>() where T : Delegate => (T)CreateDelegate(typeof(T));. I think there is nothing to fix in the analyzer, unless we want to check the implementation, which I did not think worth a fix, because the analyzer suggestion is more about AOT compliance, external applications should change to use the generic overload if they care about AOT or similar tooling
3. The generic overload implementation using typeof(T) internally (this is kind of similar to the case 2 but the implementation not exactly calling the Type overload). For example: Marshal.SizeOf<T>(), Marshal.PtrToStructure<T>(nint) etc. By my evaluation these doesn't look worth fixing, but not that sure with my evaluation. No fix needed for the analyzer with the same reason as above, from AOT perspective this is not false positive.

Here are the all cases that I did not fix: CA2263 findings_NotFixed.txt

(That can/should certainly be separate from this PR.)

I have fixed the ones that worth fixing, PTAL

[buyaa-n]

This analyzer is first and foremost a code style analyzer/fixer. I do not think it is useless, but I am not sure whether it should be enabled by default. Do we have a prior art for analyzers like that?

Then this rule should start with "IDE" prefix https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/categories#style-rules. I think "IDE" rules should not added in roslyn-analyzers repo, probably in roslyn?

Not sure we could move there ...

[jkotas]

In some cases, the fix suggested by the analyzer may result in a minor performance improvement. Example: Replacing Enum.GetName with EnumGetName<TEnum> will avoid boxing. I am not aware of anything else.

[buyaa-n]

In some cases, the fix suggested by the analyzer may result in a minor performance improvement. Example: Replacing Enum.GetName with EnumGetName<TEnum> will avoid boxing. I am not aware of anything else.

Yes, and that is why I fixed those cases with this PR, thought the cases where the generic overload implementation just calling the Type overload, will have some perf degradation, for other case like Marshal.SizeOf<T>() and Marshal.SizeOf(Type) the perf impact was not obvious, so did not fix. Anyway, I don't think with the analyzer we could evaluate the perf impact.

Based on your and @stephentoub's feedback I put up a PR that fixes all cases excluding the projects that target down-level platforms and System.Linq.Expression.

[buyaa-n]

This analyzer is first and foremost a code style analyzer/fixer.

If that's really the case, we shouldn't have it at all; we're generally not in the business of including style analyzers in this library.

But is that really the case?

I think that is the case, so what we should do with the analyzer now?

[stephentoub]

I think that is the case, so what we should do with the analyzer now?

The fact that it catches things like changing Enum calls from non-generic to generic makes me still excited about it. I was just fixing up an internal codebase and found it useful specifically for that.