Runtime: Do not use default constructor of ImmutableArray<T>

Created on 25 Mar 2020  路  5Comments  路  Source: dotnet/runtime

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

C# 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.

api-suggestion area-System.Collections code-analyzer code-fixer

Most helpful comment

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.

All 5 comments

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

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

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.

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.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

chunseoklee picture chunseoklee  路  3Comments

Timovzl picture Timovzl  路  3Comments

aggieben picture aggieben  路  3Comments

EgorBo picture EgorBo  路  3Comments

btecu picture btecu  路  3Comments