dotnet / roslyn-analyzers

MIT License
1.58k stars 463 forks source link

Analyzer to report arrays that can be converted to ImmutableArray<T> #5607

Open NewellClark opened 2 years ago

NewellClark commented 2 years ago

Brief description:

Find array fields who's contents are never mutated and suggest using an ImmutableArray<T> when possible. Intended for internal use in dotnet/roslyn.

Languages applicable:

Both C# and Visual Basic.

Code example that the analyzer should report:

https://github.com/dotnet/roslyn/blob/b88948f10a1085980df7080f0b765b1a8c614436/src/Workspaces/Core/Portable/PatternMatching/PatternMatcher.cs#L27 The elements of this array are never mutated, and should never be mutated. It should be converted to an ImmutableArray<char> to enforce this invariant.

Additional information:

See discussion on this PR for details. Not a duplicate of this issue, since it applies to arrays of all types. Also, because ImmutableArray<T> shares more apis with T[] than ReadOnlySpan<T> does and lacks the ref struct restrictions, we can report diagnostics more aggressively.

If this gets accepted, I'm happy to implement the analyzer. I'm already working on a prototype, since @CyrusNajmabadi wants arrays converted to immutable arrays when possible.

Evangelink commented 2 years ago

What about methods arguments and local variables?

NewellClark commented 2 years ago

I think notifying for local variables would be unnecessary, since the scope where a local variable can be modified is much more limited, so the odds of someone accidentally mutating it are much lower. Ditto for method arguments. A field, on the other hand, can be mutated anywhere in its class, and the Roslyn codebase has some really large classes, so I think using the type system to enforce the invariant that fields not be mutated is valuable.

Youssef1313 commented 2 years ago

@stephentoub Can you move to dotnet/runtime? Thanks

stephentoub commented 2 years ago

Can you move to dotnet/runtime?

The original post states "Intended for internal use in dotnet/roslyn." If this is destined to be an "RS" analyzer only for use in Roslyn, it doesn't need triaging in dotnet/runtime.