Add NETH005 analyzer: redundant Span<T>.ToArray() at call sites

[asdacap]

Summary

• New Roslyn analyzer NETH004 flags Span<T>.ToArray() / ReadOnlySpan<T>.ToArray() arguments at call sites whose target method or constructor has an overload accepting Span<T> / ReadOnlySpan<T> at the same parameter position. Pass the span directly instead of allocating.
• Handles non-generic, generic, and constructor cases. Respects the Span -> ReadOnlySpan implicit conversion direction. Skips params, element-type mismatches, and the canonical Foo(ReadOnlySpan<T> s) : this(s.ToArray()) self-delegation pattern.
• Cleans up the 11 call sites the analyzer caught across the solution (Core, Evm, Blockchain.Test, Shutter.Test, State.Flat, StateComposition.Test).

Changes

• Nethermind.Analyzers/SpanToArrayAtCallSiteAnalyzer.cs — analyzer.
• Nethermind.Analyzers/AnalyzerReleases.Unshipped.md — NETH004 release tracking row.
• Nethermind.Analyzers.Test/SpanToArrayAtCallSiteAnalyzerTests.cs — 12 unit tests covering positives, negatives, constructors, generic methods, ROS->Span direction, params, element-type mismatch, and self-delegation.
• 8 call-site cleanups (one-line edits each).

Test plan

• dotnet test --project Nethermind.Analyzers.Test/Nethermind.Analyzers.Test.csproj — 55/55 passing.
• Full solution build (dotnet build -c release Nethermind.slnx) — 0 warnings, 0 errors.

🤖 Generated with Claude Code

[claude]

Claude finished @asdacap's task in 5m 41s —— View job

---

PR Review: NETH004 Span.ToArray() Analyzer

• Gathering context
• Reading analyzer implementation
• Reading test coverage
• Reviewing call-site cleanups
• Posting findings

---

Summary

The analyzer logic is sound for the common case, but there are two Medium issues — one correctness bug (accessibility not checked) and one coding-style violation (LINQ) — plus a few lower-severity gaps worth calling out.

---

Medium Findings

1. Candidate overload accessibility is not checked — potential false positives

containingType.GetMembers(method.Name) returns all members regardless of accessibility, including private and internal ones. If a span overload exists but is inaccessible from the call site, the analyzer will still flag ToArray() — but applying the suggested fix would be a compile error.

Example false-positive scenario:

// In some external assembly or subclass
class Foo {
    public void M(byte[] x) { }
    private void M(Span<byte> x) { }  // inaccessible from outside
}
// External call site — analyzer incorrectly flags this:
foo.M(span.ToArray()); // Suggests "pass span directly", but M(Span<byte>) is private

The candidate lookup should filter using context.Compilation.IsSymbolAccessibleWithin(candidate, context.ContainingSymbol?.ContainingType ?? currentMethod.ContainingType) or ISemanticModel.IsAccessible.

Fix this →

---

2. LINQ usage violates coding-style.md

coding-style.md is explicit: "No LINQ when a simple for/foreach works." Analyzers run on every compilation in the IDE, so unnecessary LINQ allocations compound quickly.

Two violations:

Line 75 — OfType<IMethodSymbol>() materializes a LINQ lazy chain from an ImmutableArray<ISymbol>:

// Current (allocates IEnumerable wrapper + IEnumerable state machine)
IEnumerable<IMethodSymbol> overloads = containingType.GetMembers(method.Name).OfType<IMethodSymbol>();

// Fix: pass ImmutableArray<ISymbol> directly and cast with `is` inside the loop

Line 132 — candidateOverloads.ToArray() via System.Linq:

overloads ??= candidateOverloads.ToArray(); // LINQ ToArray on IEnumerable

For invocations, the enumerable is a lazy LINQ chain (from OfType); for constructors, boxing ImmutableArray<T> into IEnumerable<T> and calling LINQ's ToArray() copies it. Replace with a loop that builds a List<IMethodSymbol> or use a helper that works directly on ImmutableArray<ISymbol>.

---

Low Findings

3. Self-delegation test only covers constructors — no test for regular method chaining

The test Span_overload_chaining_to_array_overload_no_diagnostic tests:

public Box(ReadOnlySpan<byte> s) : this(s.ToArray()) { }

But the same pattern can appear in regular methods:

void Foo(byte[] x) { /* impl */ }
void Foo(Span<byte> s) { Foo(s.ToArray()); } // should NOT be flagged

The code paths are identical (containingSymbol check applies to both), but adding this test case would increase confidence.

---

4. Extension-method overloads are not detected (false negatives)

containingType.GetMembers() only returns direct members. Span overloads defined as extension methods in another class are invisible to the analyzer. This is a known limitation that's reasonable for a first version but worth noting:

static class SpanExtensions {
    public static void Write(this Stream s, Span<byte> buf) { }
}
// stream.Write(span.ToArray()) — not flagged, even though Write(Span<byte>) exists

---

5. Test ReferenceAssemblies lags behind project target

Tests use ReferenceAssemblies.Net.Net80 (line 205 in SpanToArrayAtCallSiteAnalyzerTests.cs) while the project targets net10.0. This is likely because Net.Net100 isn't available in the testing package yet. For span semantics this is inconsequential, but it's worth tracking for upgrade.

---

Call-site cleanups — correctness verified ✓

All 8 one-line changes were checked against the actual constructor signatures:

File	Change	Verification
Address.cs:38	hash.Bytes.Slice(12, Size) → Address(ReadOnlySpan<byte>)	Hash256.Bytes is Span<byte> — implicit → ROS ✓
Address.cs:40	hash.BytesAsSpan.Slice(12, Size) → Address(ReadOnlySpan<byte>)	ValueHash256.BytesAsSpan is Span<byte> ✓
Address.cs:402	new(Bytes) in AddressStructRef.ToAddress()	Bytes is ReadOnlySpan<byte> ✓
PublicKey.cs:51	Hash.Bytes[12..]	Hash256.Bytes is Span<byte>, used inside lambda body (not captured) — valid C# 11+ ✓
PublicKey.cs:80	hash[12..] where hash: Span<byte>	Address(ReadOnlySpan<byte>) via implicit conversion ✓
ICodeInfoRepository.cs	code[DelegationHeader.Length..]	Passes through Address(ReadOnlySpan<byte>) ✓
TraceStack.cs	Peek(index)[12..] where Peek returns ReadOnlySpan<byte>	✓
FlatTrieVerifier.cs	flatKey.Bytes[..20]	✓
ShutterEventSimulator.cs	identityPreimage[32..]	✓
TrieDiffWalkerTests.cs	Keccak.Compute(...).Bytes[..20]	Hash256.Bytes is Span<byte>, range returns new Span<byte> ✓
TransactionProcessorEip7702Tests.cs	new Address(cell) where cell: ReadOnlySpan<byte>	✓

All behavioral equivalence is maintained — the Address(ReadOnlySpan<byte>) constructor calls .ToArray() internally, so the heap copy still happens exactly once.

---

Verdict: Two Medium issues (accessibility check missing, LINQ violation) should be addressed before merge.

[claude]

LINQ violation (coding-style.md): OfType<IMethodSymbol>() produces a LINQ lazy chain from ImmutableArray<ISymbol>. Analyzers run on every keystroke in the IDE — this allocates an enumerator on every invocation that hits this path.

Suggested change

	IEnumerable<IMethodSymbol> overloads = containingType.GetMembers(method.Name).OfType<IMethodSymbol>();
	ImmutableArray<ISymbol> overloadSymbols = containingType.GetMembers(method.Name);
	AnalyzeArguments(context, spanTypes, invocation.Arguments, method, overloadSymbols);

Then change AnalyzeArguments / FindSpanOverloadKind to accept ImmutableArray<ISymbol> and use is IMethodSymbol m inline, eliminating the LINQ dependency entirely.

[claude]

Missing accessibility check — potential false positive. GetMembers() returns private/internal members regardless of whether they're accessible at the call site. If a Span<T> overload exists but is private, the diagnostic fires — but applying the fix would not compile.

Add a guard like:

if (!IsAccessibleAt(candidate, context, currentMethod))
    continue;

where IsAccessibleAt checks candidate.DeclaredAccessibility against the enclosing type of the call site (context.ContainingSymbol?.ContainingType), or use compilation.IsSymbolAccessibleWithin(candidate, fromWithin).

[asdacap]

Addressed both medium findings from the review:

1. Accessibility (Medium Feature/jsonrpc #1): FindSpanOverloadKind now calls compilation.IsSymbolAccessibleWithin(candidate, withinAccessibility) to skip overloads that are not visible from the call site. Added regression test Inaccessible_span_overload_no_diagnostic.
2. LINQ (Medium Feature/networking #2): Replaced OfType<IMethodSymbol>() and IEnumerable<>.ToArray() with manual loops over ImmutableArray<ISymbol> / ImmutableArray<IMethodSymbol>. Method overloads are now collected via CollectMethodOverloads using an ImmutableArray.Builder; constructor overloads use containingType.InstanceConstructors directly. No System.Linq reference remains.

Also addressed Low #3 (added Method_level_span_overload_chaining_to_array_no_diagnostic test for method-level self-delegation).

Low #4 (extension methods) and Low #5 (test reference assemblies) are noted as known limitations and intentionally deferred.

Pushed in 34f2fe3. Tests: 14/14 passing; full solution build clean.

— Claude Opus 4.7 (1M context)

[claude]

Claude finished @asdacap's task in 3m 27s —— View job

---

PR Review: NETH004 Span.ToArray() Analyzer (Follow-up)

• Gathering context
• Reading analyzer implementation and rules files
• Reviewing test coverage
• Reviewing call-site cleanups (all rounds)
• Checking previous Medium findings are resolved
• Posting final review

---

Summary

Both Medium findings from the prior review have been addressed. The round-2 call-site cleanups are all correct. No new Medium or High issues found.

---

Previous Medium Issues — Resolved ✓

Medium #1: Accessibility check

Fixed in 34f2fe3. FindSpanOverloadKind now passes each candidate through compilation.IsSymbolAccessibleWithin(candidate, withinAccessibility) before considering it. The withinAccessibility chain (containingSymbol?.ContainingType ?? containingSymbol ?? currentMethod.ContainingType) is sound — the final fallback always resolves to a named type.

New regression test Inaccessible_span_overload_no_diagnostic confirms a private span overload is not flagged from an external class.

Medium #2: LINQ removal

Fixed in 34f2fe3 / ecb60a6. OfType<IMethodSymbol>() replaced by CollectMethodOverloads (manual foreach with is IMethodSymbol inline). Constructors now use containingType.InstanceConstructors directly. No System.Linq dependency remains in the analyzer.

---

Round-2 Call-site Cleanups — Correctness Verified ✓

File	Change	Verification
Address.cs:306 (AddressStructRef)	keccak.BytesAsSpan.Slice(12, ByteLength)	AddressStructRef(ReadOnlySpan<byte>) exists at line 341 — Span<byte> implicit → ROS ✓
Rlp.cs:946 (bloom decoder)	new Bloom(bloomBytes)	Bloom(ReadOnlySpan<byte>) does bytes.CopyTo(Bytes) — semantics unchanged ✓
BitArrayTests.cs (16 sites)	HashUtility.Chunk(...).ToArray() → HashUtility.Chunk(...)	HashUtility.Hash(ReadOnlySpan<byte>, ReadOnlySpan<byte>) exists; outer .ToArray() at TestCaseData boundary correctly kept ✓
RlpTests.cs (2 sites)	new(Rlp.Encode(value, stackalloc byte[9]))	ValueDecoderContext is a ref struct with scoped in ReadOnlySpan<byte> ctor; Roslyn lifetime analysis guarantees context can't outlive the stackalloc buffer ✓
ShutterCryptoTests.cs	msg.SequenceEqual(decryptedMessage)	Resolves to MemoryExtensions.SequenceEqual(ReadOnlySpan<byte>, ReadOnlySpan<byte>) instead of LINQ — semantically equivalent, more efficient ✓

---

Remaining Known Limitations (Low — carry-overs from prior review)

Low #4 (unchanged): Extension method overloads are not detected (containingType.GetMembers() only returns direct members). Reasonable for a first version.

Low #5 (unchanged): Tests use ReferenceAssemblies.Net.Net80 (SpanToArrayAtCallSiteAnalyzerTests.cs:298) while the project targets net10.0 — no impact on span semantics, worth tracking for upgrade when the testing package supports it.

Low #6 (new): Inherited overloads are not detected. If a base class declares M(Span<byte>) and a derived class only has M(byte[]), a call derived.M(span.ToArray()) will not be flagged. Same fundamental limitation as extension methods — acceptable for a v1 analyzer.

---

Verdict: All Critical, High, and Medium findings resolved. No new blockers. This is ready to merge.

[LukaszRozmej]

Maybe resolve low issues?

[LukaszRozmej]

collapse to testcases?

[LukaszRozmej]

@claude would any of those ToArray be explicitly intended to do a copy? Also do a review.

[claude]

Claude finished @LukaszRozmej's task in 7m 6s —— View job

---

PR Review: NETH005 Span.ToArray() Analyzer

• Gathering context
• Analyzing whether any ToArray() removals were intentional copies
• Reviewing analyzer implementation
• Reviewing test coverage
• Reviewing call-site cleanups

---

Were any removed ToArray() calls intentional copies?

Short answer: No. All removed .ToArray() calls were redundant intermediate allocations. Here's the breakdown by case:

All Address(ReadOnlySpan<byte>) call sites (Address.cs, PublicKey.cs, TraceStack.cs, ICodeInfoRepository.cs, FlatTrieVerifier.cs, ShutterEventSimulator.cs, TrieDiffWalkerTests.cs, TransactionProcessorEip7702Tests.cs): The Address(ReadOnlySpan<byte>) constructor at Address.cs:145 calls bytes.CopyTo(...) internally — it always copies. The pre-call .ToArray() was creating a redundant heap array that was immediately consumed and discarded.

AddressStructRef (Address.cs:312, 405): Two cases:

• ToAddress() (line 405): new(Bytes.ToArray()) → new(Bytes). Address(ReadOnlySpan<byte>) copies, so .ToArray() was redundant. ✓
• AddressStructRef(in ValueHash256 keccak) (line 312): The .ToArray() created a heap-backed span, whereas the new code stores a span directly over the ValueHash256 struct's bytes. This IS a semantic change in storage — but it's safe because AddressStructRef is a ref struct and C#'s span safety rules prevent it from outliving keccak. Notably, the sibling constructor AddressStructRef(Hash256StructRef keccak) (line 310) already used the direct-span pattern without .ToArray(), so this fix makes them consistent.

Bloom(bloomBytes) (Rlp.cs:956): Bloom(ReadOnlySpan<byte> bytes) calls bytes.CopyTo(Bytes) — always copies. Redundant. ✓

RlpTests.cs ValueDecoderContext (lines 154, 166): new(Rlp.Encode(value, stackalloc byte[9])) — the old .ToArray() created a heap copy of the stackalloc buffer before passing to ValueDecoderContext. The new code passes the span over the stackalloc directly. Since ValueDecoderContext is a ref struct with scoped in ReadOnlySpan<byte> constructor, the compiler constrains its lifetime to the stackalloc buffer's scope. Safe. ✓

HashUtility.Chunk(...).ToArray() → HashUtility.Chunk(...) (BitArrayTests.cs): HashUtility.Hash(ReadOnlySpan<byte>, ReadOnlySpan<byte>) already accepts spans. The .ToArray() was a dead conversion. HashUtility.Chunk returns a span over a heap-allocated byte[32], so no lifetime issue. ✓

msg.SequenceEqual(decryptedMessage) (ShutterCryptoTests.cs): Old: LINQ SequenceEqual<byte>. New: MemoryExtensions.SequenceEqual(ReadOnlySpan<byte>, ReadOnlySpan<byte>). Semantically equivalent, more efficient. ✓

---

Analyzer Review

All prior Medium findings resolved (accessibility check, LINQ removal from the earlier review rounds).

New findings

Low: PR description says "NETH004" but code correctly uses "NETH005"

The PR body still says "New Roslyn analyzer NETH004..." but NETH004 was taken by the BannedConcurrentDictionary analyzer. The AnalyzerReleases.Unshipped.md and the code correctly use NETH005. Purely cosmetic.

---

Low: Test structure — Lukasz's [TestCase] suggestion

The existing test class uses 14 separate [Test] methods. The adjacent BannedConcurrentDictionaryKeysValuesAnalyzerTests.cs uses [TestCase] for cases that vary by a single parameter. The "reports" tests here vary by span type (Span<T> vs ReadOnlySpan<T>) and could be parameterized. For example, these three tests share structure:

• Span_to_array_with_Span_overload_reports
• ReadOnlySpan_to_array_with_ReadOnlySpan_overload_reports
• Span_to_array_with_only_ReadOnlySpan_overload_reports

Similarly, the "no diagnostic" tests could share a single [TestCase] method. However, most tests have meaningfully different source strings, so full parameterization would require a [TestCaseSource] with tuples — more boilerplate than it's worth for 14 cases. A partial collapse (just the first 3 positive cases) is feasible.

---

Low (carry-over): Extension method and inherited overloads not detected (false negatives)

Already noted in prior reviews as accepted limitations.

---

Low (carry-over): ReferenceAssemblies.Net.Net80 in tests vs. project targeting net10.0

No span-semantic impact, but worth tracking.

---

Correctness check of the analyzer logic

No new issues found. The key paths all look correct:

• ContainingType.GetMembers(name) + CollectMethodOverloads using ImmutableArray.Builder — correct and allocation-efficient. ✓
• compilation.IsSymbolAccessibleWithin guard — prevents false positives from private span overloads. ✓
• Self-delegation skip (containingOriginal check) — prevents flagging Foo(Span<T>) { Foo(s.ToArray()); } patterns. ✓
• params guard — correct. ✓
• Element-type and type-parameter-ordinal matching — handles both concrete and generic cases. ✓
• ReadOnlySpan<T> caller can't substitute for Span<T> param — correctly enforced. ✓

---

Verdict: All Critical, High, and Medium findings resolved. Ready to merge.