Revise compound assignment for nullable

[RikkiGibson]

• Closes Incorrect nullable analysis of compound assignment due to double visit of operands #70646.
• Closes Compilator can't find the matching overload for operator+ when implementing nullability to project #65546.
• Closes Incorrect nullability analysis in compound assignment #29962.

Addresses several nullable issues, mainly related to compound assignment. I made an effort to share code for visiting a normal binary operator with the code for visiting a compound assignment.

VisitCompoundAssignmentOperator has been pretty much rewritten only using the original code as a reference. I would recommend that you review the old and new versions of the methods separately as the diff may be a little noisy.

Other minor adjustments in the PR include:

• simplifies disabling nullable warnings on default arguments. Eliminates a rather crufty condition in ReportNullableAssignmentIfNecessary and instead ensures that we pass along the default arguments BitVector so that we set _disableDiagnostics in the right places.
• changed the nullable state of an assignment expression to a property with [AllowNull]. Now, any "sanitizing" of the value going into the property will not apply until the property is actually read again. This more precisely reflects the semantics of this scenario.

[cston]

as BoundConversion

Is as BoundConversion necessary? What else might this be other than a BoundConversion or null?

[RikkiGibson]

roslyn/src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

Lines 546 to 547 in 003a34f

	<!-- Could be a BoundConversion or, in some cases of an identity conversion, the LeftPlaceholder, or null for no conversion case -->
	<Field Name="LeftConversion" Type="BoundExpression?" SkipInVisitor="true" Null="allow"/>

The other possibility is that it is a BoundPlaceholder, which to me didn't make sense to visit as if the operand is really being operated on by that placeholder. However, I don't think I tested to see what happens when we remove the as BoundConversion.

[jcouv]

nit: No test is affected by the removal of as BoundConversion. It'd be good to hunt one down.

[RikkiGibson]

Opened #70751 to try and find a scenario which could be used as a starting point. I probably will not block the PR on it, though.

[cston]

Debug.Assert(MakeSlot(node) == -1);

Just curious: Why assert this?

[RikkiGibson]

I wanted to indicate that the value stored in the LHS of the compound-assignment effectively drops any "sub-slot" information in either operand of the compound-assignment, i.e. the nullable state of fields of the operands. Showing that the entire compound-assignment never has a slot felt useful. See test CompoundAssignment_SubSlots.

[cston]

In that case, shouldn't the assert use node.Left rather than node? (I don't think we ever create slots for binary operators.)

[RikkiGibson]

A regular assignment can have a slot.

It's possible a compound assignment should have a slot for the same reason but I feel comfortable punting on it.

https://sharplab.io/#v2:CYLg1APgAgTAjAWAFBQMwAJboMLoN7LpGYZQAs6AsgBRRwAMA/OgB4A0mDzAnh3U+gBeASnyFiEgJYAzdNWot0AXnTdRAQhUA7AK4AbPaJYA6ACoB7AMoAXAE6StAc2rCA3OIlETFm/acv3JAkAX2RgoA===

[cston]

#65546

70646?

[cston]

}

Should we test the following as well?

class C
{
    public string f1, f2, f3;
}
C c1 = new() { f1 = null, f2 = null }; // 1, 2
C c2 = new() { f1 = null, f3 = null }; // 3, 4
c1 += c2;
c1.f1.ToString();
c1.f2.ToString();
c1.f3.ToString();

[jcouv]

LGTM Thanks (iteration 3)