S3655: Boxing

[antonioaversa]

Part 5 of task 6 of #6794

Previous task: #6989

[Tim-Pohlmann]

LGTM, only minor nitpicks.

[martin-strecker-sonarsource]

What is our strategy here? With the "possible null reference" rule we honored the user's wish to overrule us. If this should be the case here too, then this needs to be marked as FP.
What is the "empty" comment referring to? Please use "null" here and below instead.

[antonioaversa]

My understanding, deriving from a discussion we had during one of our sync meeting, is the following.

In the rule checking about possible null references, we trigger an issue every time we find a dereference of something which can be null. If we don't know for certain (i.e. unknown value), we trigger, because the possibility of dereference is there.

In this rule, however, we only trigger when we know for certain that there is a path where the variable is null. In the case above, for example, i is assigned to be null right before value accessing, so we know for certain that an InvalidOperationException will be produced as a result of calling Value on i, no matter if the null forgiving operator is used or not.

So we basically ignore the presence of ! and ask the user to protect against the scenario where the variable surely get a null.

An objection to this line of reasoning may be that a simple check of HasValue would trigger the rule:

void Method(int? i)
{
    if (i.HasValue) { }
    _ = i.Value; // Noncompliant, there's a path where i is empty
}

whereas

void Method(int? i)
{
    _ = i.Value; // Compliant, unknown
}

My understanding (and I would ask @pavel-mikula-sonarsource to back up or dismiss the reasoning) is that if the user has made a previous call to i.HasValue, he has somewhat acknowledged the fact that i can be empty. So we then ask him to be consistent with that observation, and protect his access to i.Value with a check i.HasValue or i != null.

Concerning the use of , empty after Noncompliant: it refers to the nullable, which is empty. I have used the word "empty", rather than "null", because the rule is called EmptyNullableValueAccess and also because I find a bit misleading to say that a struct is null. It's rather the opposite: null is converted by the compiler into a Nullable<T> struct with hasValue == false.

[Tim-Pohlmann]

We should honor the null forgiving operator. In some cases, our engine is wrong, and the user should be able to make the rule shut up.

[pavel-mikula-sonarsource]

To correct @antonioaversa understanding: Both S3655 and S2259 behave the same in principle. If user indicated possible null (for example by asking nullable.HasValue or arg is null on the other rule), we assume possible null and will raise. S2259 does not raise if we don't know to reduce noise.

As ! means "trust me, it works (and I won't blame you)", I'm fine with keeping the behavior symmetrical and support bang here too.

[antonioaversa]

Great, thanks for the explanation. Support of this will come in a dedicated PR: #7015

[martin-strecker-sonarsource]

Looks good. One roundtrip example should be added.

[martin-strecker-sonarsource]

We already have this example at another place right:

        foreach (int boxed in new int?[] { null })

[antonioaversa]

Checking whether we actually had the very same example was an interesting exercise and a good learning experience!

I don't see such a case being defined anywhere else, at least in this rule.

There are similar scenarios using a custom struct private struct AStruct { }, where the emphasis is put on the implicit cast made by the foreach instruction, rather than the boxing/unboxing of struct in a collection:

foreach (AStruct x in new AStruct?[] { null }) ;                 // FN

I agree, however, that a the end of the day int is a struct, pretty much like AStruct, so foreach (int boxed in new int?[] { null }) is not that different from foreach (AStruct x in new AStruct?[] { null }). int is still a well-known type, whereas AStruct isn't, so it's maybe worth keeping both.

Another example that is very similar to foreach (var boxed in new object[] { null as int? }) is the example in the method above (ForeachImplictConversion): foreach (object boxed in new int?[] { null }).
The goal of foreach (object boxed in new int?[] { null }), however, was to have the foreach boxing the int? coming from new int?[] { null }).

The ldelem done to access the i-th item of the array takes the element from *(&MemoryMarshal.GetArrayDataReference(sourceArray) + sizeof(int) * i), and it doesn't need to box it, unless that is required by the looping variable.

Therefore, having foreach (object boxed in new int?[] { null }) and foreach (var boxed in new int?[] { null }) are two different things:

• the first does a boxing while iterating over the array: IL_0030 here
• the second doesn't: IL_0027 to IL_003a here.

There is yet another interesting case, that I missed:

foreach (object boxed in stackalloc int?[] { null as int?, 42, 43 });

It changes memory layout and array item loading, which is done via call instance of Span<int?>.GetItem followed by ldobj, so I have added it.

What to you think?

[martin-strecker-sonarsource]

I think we should have test cases for for each for the following cases:

explicit conversion from nullable to non-nullable

• explicit conversion from int? to int (e.g. foreach(int i in new int?[] { 1, null }))
• lifted explicit conversions (e.g. foreach(int i in new long?[] { 1, null })). Here there are actually two explicit conversions at stake: one form nullable to non-nullable and one from wider type (long) to narrow type (int).

These are the cases I asked for originally.

boxing conversions from nullable to object

foreach (object o in new int?[] { null })

These are implicit conversions. This kind of operation should always succeed, so I don't see the need for much test coverage here.

unboxing into nullable

foreach (int? i in new object[] { null, 1, "Boom" })

This has nothing to do with the rule itself. No need to cover it with tests.

stackalloc

I don't see how this makes a difference. It is just another kind of location to enumerate. You can also use other kinds of pattern-based foreach collections, like ImmutableArrays Enumerator. It doesn't make a difference for the test cases.

In general, I would look at the C# spec rather than the IL generated. The IL generated needs to follow the spec behavior-wise. I don't see any violations from that in the examples you have given.

The sections needed are

• Conversion from a nullable value type to an underlying type
• Lifted operators
• foreach

[antonioaversa]

Ok, good, it's clear now, thanks for this analysis!

So:

explicit conversion from nullable to non-nullable

Added a new method ForeachExplicitConversion with the two examples

boxing conversions from nullable to object

The conversion indeed succeeds, but the cast in the body of the foreach does not.
(int)boxed fails with NullReferenceException at the first item of the collection which is null.
I find non-obvious at first glance that:

• if boxing happens, you end up with a NullReferenceException, raised by the runtime in the boxing operation, and S3655 does not kick in;
• whereas, if boxing doesn't happen, you end up with an InvalidOperationException, raised by the framework in Nullable<T>.Value, and S3655 does kick in.
  I wanted to document this difference in behavior, by keeping these two test scenarios ForeachImplictConversion and ForeachExplicitConversion.

unboxing into nullable

Agreed, I haven't added it.

stackalloc

Indeed, it just change the memory layout.
It just seemed interesting, since stack allocation in general can affect boxing.
Removed.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[martin-strecker-sonarsource]

LGTM