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

Do not use default constructor of ImmutableArray<T> #34096

Open
terrajobst opened this issue Mar 25, 2020 · 5 comments
Open

Do not use default constructor of ImmutableArray<T> #34096

terrajobst opened this issue Mar 25, 2020 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Milestone

Comments

@terrajobst
Copy link
Member

All structs have a default constructor, so we can't prevent people from writing code like this:

var x = new ImmutableArray<int>();
x.Add(42); // Null reference exception here

The reason this doesn't work is ImmutableArray<T> is just wrapping an array. By calling the default constructor the underlying field storing the array is initialized to null. For performance reason it's not viable to have null-checks in all methods that access this field.

Unfortunately we can't obsolete the default struct constructor because C# doesn't let us define one.

Proposal

All expressions of the form new ImmutableArray<T>() should be flagged. A code fixer should offer to replace those with ImmutableArray<T>.Empty.

@terrajobst terrajobst added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer labels Mar 25, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Mar 25, 2020
@GrabYourPitchforks
Copy link
Member

@Joe4evr
Copy link
Contributor

Joe4evr commented Mar 26, 2020

Wouldn't this analyzer be rendered superfluous if dotnet/csharplang#99 gets implemented, allowing the declaration of a no-arg constructor in ImmutableArray<T> that initializes its internal array?

Tho you'd still want a warning on the use of default(ImmutableArray<T>).

@terrajobst
Copy link
Member Author

Maybe we should just turn on the rule SA1129 ("Do not use default value type constructor"). Using default or default(T) makes it at least clear you don't run any constructor and just rely on all zeros to be a good initial state.

But as you pointed out: we should probably still warn people who do that on ImmutableArray<T>.

@stephentoub
Copy link
Member

Maybe we should just turn on the rule SA1129 ("Do not use default value type constructor"). Using default or default(T) makes it at least clear you don't run any constructor and just rely on all zeros to be a good initial state.

FWIW, that's already enabled in dotnet/runtime.

@Joe4evr
Copy link
Contributor

Joe4evr commented Mar 26, 2020

If you read further, there's a related proposal to add some flow analysis similar to NRTs that'll report for "Non-defaultable Value Types", because it'd be great if something like that can be applied generally instead of making the compiler aware of a single type like a special snowflake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections code-analyzer Marks an issue that suggests a Roslyn analyzer code-fixer Marks an issue that suggests a Roslyn code fixer
Projects
None yet
Development

No branches or pull requests

7 participants