Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
NewellClark opened this issue Oct 2, 2021 · 4 comments
Open
Labels
Area-Roslyn.Diagnostics.Analyzers Community Feature Request help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@NewellClark
Copy link
Contributor

NewellClark commented Oct 2, 2021

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.

@jinujoseph jinujoseph transferred this issue from dotnet/roslyn Oct 6, 2021
@Evangelink
Copy link
Member

What about methods arguments and local variables?

@NewellClark
Copy link
Contributor Author

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
Copy link
Member

@stephentoub Can you move to dotnet/runtime? Thanks

@stephentoub
Copy link
Member

stephentoub commented Jan 24, 2022

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.

@mavasani mavasani added Area-Roslyn.Diagnostics.Analyzers help wanted The issue is up-for-grabs, and can be claimed by commenting labels Jan 25, 2022
@mavasani mavasani added this to the Unknown milestone Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Roslyn.Diagnostics.Analyzers Community Feature Request help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Development

No branches or pull requests

5 participants