Skip to content

chore: Reduce messages that reported by analyzers - #3234

Open
filzrev wants to merge 12 commits into
dotnet:masterfrom
filzrev:chore-reduce-analyzer-message
Open

chore: Reduce messages that reported by analyzers#3234
filzrev wants to merge 12 commits into
dotnet:masterfrom
filzrev:chore-reduce-analyzer-message

Conversation

@filzrev

@filzrev filzrev commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

This PR intended to reduce messages that are reported by analyzers.
(Currently 2313 messages are reported)

Almost of changes are based on analyzer's code fixer.

And following changes are manually edited.

1. chore: fix ca2012 messages

Add helper methods to wait value task.
Because GetAwaiter().GetResult() is not guaranteed to wait ValueTask completion.

2. chore: fix cs9236 messages

Unroll LINQ query to loops. to avoid following messages.

CS9236: Compiling requires binding the lambda expression at least 1000 times. Consider declaring the lambda expression with explicit parameter types, or if the containing method call is generic, consider using explicit type arguments.

Comment thread tests/BenchmarkDotNet.IntegrationTests/JitListenerTests.cs Outdated
@timcassell

Copy link
Copy Markdown
Collaborator

Thanks for doing this — it's a large mechanical cleanup and it holds up well. I reviewed the diff (merge-base 709e2010 .. ce8baff5, 45 files, +98/−84), built the full BenchmarkDotNet.slnx across all TFMs including net472 (0 warnings, 0 errors, with TreatWarningsAsErrors on repo-wide), and ran the unit suite on net10.0 (1054 passed, 0 failed, 4 skipped).

One finding, and everything else checks out.

Contains(char) binds to LINQ on netstandard2.0

src/BenchmarkDotNet/Helpers/SectionsHelper.cs:13, src/BenchmarkDotNet/Helpers/UserInteractionHelper.cs:22, src/BenchmarkDotNet/Disassemblers/DataContracts.cs:255

string.Contains(char) doesn't exist on netstandard2.0 — only the string overload does. So on that TFM these three rewrites don't call a string method at all: with ImplicitUsings pulling in System.Linq, they bind to Enumerable.Contains<char>. I confirmed this with a scratch netstandard2.0 compile (without the char overload in scope the call still compiles, which it could only do via LINQ).

Two consequences on the assembly .NET Framework consumers get:

  • string is not ICollection<char>, so Enumerable.Contains takes the foreach path — a CharEnumerator allocation per call plus a per-element loop, replacing what was an intrinsic vectorized scan in string.IndexOf(char).
  • More importantly, at UserInteractionHelper.cs:22 a null input now throws ArgumentNullException under netstandard2.0 but NullReferenceException under net8+. That's a cross-TFM behavior divergence introduced by a change that reads as cosmetic.

Rather than reverting to IndexOf or suppressing CA1847 per-TFM, I'd suggest a polyfill, matching the existing src/BenchmarkDotNet/Extensions/Polyfills/ pattern:

#if NETSTANDARD2_0
namespace System;

internal static partial class StringExtensions
{
    extension(string s)
    {
        public bool Contains(char value) => s.IndexOf(value) >= 0;
    }
}
#endif

I verified this actually wins overload resolution against Enumerable.Contains<char> (tracer build with the polyfill marked [Obsolete(..., error: true)]error CS0619 at the call site, so the compiler does select it). It wins because the receiver conversion is identity (stringstring) versus an implicit reference conversion (stringIEnumerable<char>). On the modern TFMs the real instance method wins over any extension, and the #if guard means the polyfill isn't even compiled there.

This keeps the clean Contains spelling at all three call sites with no other changes, keeps the analyzer satisfied without a suppression, matches semantics exactly (IndexOf(char) and Contains(char) are both ordinal), and restores NullReferenceException on both TFMs. Happy for this to land as a follow-up instead if you'd rather keep this PR purely mechanical.

Checked and found behavior-preserving

Recording these so the review is reproducible — each was re-derived rather than eyeballed:

  • ImmutableConfigBuilder — dropping the if (!builder.Contains(x)) guard is safe: ImmutableHashSet<T>.Builder.Add returns NoChangeRequired and keeps the existing element on an equality hit (unlike a Dictionary indexer), so first-wins ordering is preserved for both the TypeComparer<IDiagnoser> and default-comparer IAnalyser builders.
  • Disassemblers/DisassemblyDiagnoser.cs — the nested-Sum → explicit-loop rewrite is a genuine fix, not cosmetics. InstructionLength is int, so the old triple Sum accumulated in int (checked → OverflowException past 2 GiB of native code) before widening to long; the new loop accumulates directly into long.
  • Reports/DisplayPrecisionManager.cs — the ContainsKeyTryGetValue + early-return rewrite is exactly equivalent, including on the recursive GetPrecision(parentColumn) path.
  • String rewritesIndexOf(x, StringComparison) >= 0Contains(x, StringComparison) is definitionally identical, and no culture change is introduced anywhere: ContainsWithIgnoreCase keeps InvariantCultureIgnoreCase, and CoreRuntime/CodeGenerator/IsLinqPad keep OrdinalIgnoreCase.
  • Enum.GetNames(typeof(T))Enum.GetNames<T>() — same array, same value order, resolving through the repo's own Polyfills/EnumExtensions.cs on netstandard2.0.
  • JitListenerTests.GetAwaiter().GetResult()AwaitHelper.GetResult() on ValueTasks that complete synchronously; AwaitHelper is strictly the safer blocking path. No threading regression.
  • BenchmarkRunner.Run(typeof(X), …)Run<X>(…)RunAsync<T> is literally => RunAsync(typeof(T), …).
  • ContainsKeyTryGetValue at ConsoleLogger, BenchmarkRunnerClean.AddLogger, TraceLogParser.OnPmcEvent — all plain Dictionary<,>, and the || short-circuit keeps the possibly-null out value from being dereferenced.
  • .Any().Length/.Count != 0/.IsEmpty — all on already-materialized arrays, lists, or ImmutableHashSet, so no re-enumeration, ordering, or laziness change; the negations are correct.

This review was generated by Claude (Opus 5) via Claude Code and posted by @timcassell. The findings were reproduced against a local build, but please treat the analysis as a starting point rather than a verdict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants