Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 12, 2025

Implementing Span/ReadOnlySpan Parameter Analyzer (CA1876)

This PR implements an analyzer and fixer to detect when Span<T> or Memory<T> parameters could be ReadOnlySpan<T> or ReadOnlyMemory<T> instead, based on whether the parameter is ever written to.

Plan:

  • Add diagnostic messages to MicrosoftNetCoreAnalyzersResources.resx
  • Create PreferReadOnlySpanOverSpan analyzer (CA1876)
  • Create PreferReadOnlySpanOverSpan fixer
  • Add unit tests for the analyzer and fixer
  • Build and test the implementation
  • Run code review and address feedback
  • Address PR review comments

Implementation Details:

  • Rule ID: CA1876
  • Category: Performance
  • Default Severity: Info
  • Default Scope: Internal and Private members only
  • Supports: Span, Memory, ReadOnlySpan, ReadOnlyMemory
  • Excludes: overridden methods, explicit AND implicit interface implementations
  • Detects writes through: direct assignments, ref/out parameters, property writes, indexer writes, method argument passing

Changes from Review Feedback:

  • Changed from ImmutableDictionary to ConcurrentDictionary for proper mutation behavior
  • Lazy initialization of candidateParameters dictionary to avoid allocation in common case
  • Consolidated multiple if blocks into single condition with OR logic
  • Added check for parameter passing to methods that might write to them
  • Using existing readOnlySpan/readOnlyMemory symbols instead of looking them up again
  • Removed VisualBasic support from code fixer (C# only)
  • Simplified GetReadOnlyTypeName to prepend "ReadOnly" instead of reconstructing
  • Removed unused newTypeName parameter from ChangeParameterTypeAsync

Test Results:

  • All 12 tests passing
  • Covers: basic diagnostics, writes detection, override exclusion, interface implementation exclusion, multiple parameters
Original prompt

This section details on the original issue you should resolve

<issue_title>[Analyzer Proposal]: Span parameter that could be a ReadOnlySpan parameter</issue_title>
<issue_description>If a Span<T> parameter is never written to by a method body, it's better for the parameter to be a ReadOnlySpan<T> rather than Span<T>. Doing so:

  • Prevents inadvertent mistakes by preventing the method from erroneously writing to the memory (short of using unsafe code)
  • Advertizes to the caller the semantics of the callee (that it won't be mutating the argument)
  • Can have better performance for the caller, e.g. if the caller has an array of reference types, constructing a ReadOnlySpan<T> is cheaper than constructing a Span<T>, as the latter needs to perform a variance check and the former does not.

We could extend such an analyzer to locals as well. This can also handle {ReadOnly}Memory.

And we should probably allow it to be configurable around visibility, e.g. default to only applying to internal/private members but allow it to be applied to public, too.
</issue_description>

<agent_instructions>Implement an analyzer and fixer in the Microsoft.CodeAnalysis.NetAnalyzers project, following the same approach and style as is used in the rest of the project.</agent_instructions>

Comments on the Issue (you are @copilot in this section)

@ Tagging subscribers to this area: @dotnet/area-system-memory See info in [area-owners.md](https://github.com/dotnet/runtime/blob/main/docs/area-owners.md) if you want to be subscribed.
Issue Details

If a Span<T> parameter is never written to by a method body, it's better for the parameter to be a ReadOnlySpan<T> rather than Span<T>. Doing so:

  • Prevents inadvertent mistakes by preventing the method from erroneously writing to the memory (short of using unsafe code)
  • Advertizes to the caller the semantics of the callee (that it won't be mutating the argument)
  • Can have better performance for the caller, e.g. if the caller has an array of reference types, constructing a ReadOnlySpan<T> is cheaper than constructing a Span<T>, as the latter needs to perform a variance check and the former does not.

We could extend such an analyzer to locals as well.

And we should probably allow it to be configurable around visibility, e.g. default to only applying to internal/private members but allow it to be applied to public, too.

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.Memory, untriaged

Milestone: -
@stephentoub > Presumably this would apply to `Memory` as well?

It could, yes.</comment_new>
<comment_new>@stephentoub

Tangential, but is there a system that allows you to tag a project as "internal" such that public members - for the sake of analyzers and whatnot - are classified as internal and vice versa. For me personally, there's no difference other than hiding stuff from other projects in the same solution for if I use public vs internal, but many analyzers works differently based on it.

Not to my knowledge. You could open an issue in roslyn-analyzers about such configuration.</comment_new>
<comment_new>@bartonjs
Video

  • Detect when an input parameter or a local could be ReadOnlySpan or ReadOnlyMemory instead of Span or Memory
  • The analyzer shouldn't analyze parameters on overridden members
  • The analyzer will probably not provide analysis for a Span that gets used in a fixed statement, rather than decide if it has read semantics or write semantics on the pointer.
  • It should be on by default for non-visible members, with an opt-in for public.
    • Pipedream: It should run on "new code", if we could determine that.

Fixer: Obviously
Category: Reliability
Severity: Info</comment_new>

Fixes dotnet/runtime#96587


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 3 commits October 12, 2025 20:14
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
…ndexer writes

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Add analyzer for using ReadOnlySpan instead of Span Add analyzer CA1876: Prefer ReadOnlySpan<T> over Span<T> for parameters that are not written to Oct 12, 2025
@Copilot Copilot AI requested a review from stephentoub October 12, 2025 20:32
Copilot finished work on behalf of stephentoub October 12, 2025 20:32
…ions, handle argument passing, use existing type symbols, remove VB support, simplify type name generation

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Copilot finished work on behalf of stephentoub October 13, 2025 16:07
@Copilot Copilot AI requested a review from stephentoub October 13, 2025 16:07
…erage, handle extension methods on Span

- Use pattern matching in RegisterCodeFixesAsync and GetReadOnlyTypeName for cleaner code
- Add 15 new test cases covering: local copies, foreach loops, out arguments, property access, LINQ, indexer reads, ternary expressions, generic methods, return statements, CopyTo/TryCopyTo, slice operations, conditional access, multiple references
- Add invocation operation handler to detect extension methods that require writable Span/Memory (like Clear, Fill)

Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
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.

[Analyzer Proposal]: Span parameter that could be a ReadOnlySpan parameter

2 participants