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

[stephentoub]

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.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-memory
See info in 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:	-

[alexrp]

Presumably this would apply to Memory<T> as well?

[stephentoub]

Presumably this would apply to Memory<T> as well?

It could, yes.

[Clockwork-Muse]

default to only applying to internal/private members but allow it to be applied to public, too

Argh.
On the one hand, during initial design time you want it to apply to public members.
During maintenance, not necessarily so much....

[KennethHoff]

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.

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.

If there is no such system, would you consider it? Doing it manually for each analyzer is in no way sustainable ( some only function for internal, some only for public, and so on)

[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.

[Neme12]

This actually seems like it would be useful for ref parameters too to make them ref readonly (or in). But it's really hard to determine whether it's actually written to and whether the semantics will be the same with readonly (no additional copies), as the compiler doesn't provide any APIs to determine where structs are copied during their invocation as far as I recall.