Do not treat Nullable null value as a constant for the purpose of IL generation.

[AlekseyTs]

Fixes #64777.

[jcouv]

I need a bit of help to understand this fix (it seems in a very central location and surprising).

From what I can tell, the problem is only occurring when there is an identity conversion between the implicit user-defined conversion and the null literal.
Below are the bound tree for nullable tuples scenario (broken) vs. nullable int scenario (works) for comparison.

Looking at those two trees, I can see that there is an additional identity conversion in the tuples scenario. It's a conversion of default to type (int SomeInt, bool SomeBool)?.
Why does that conversion node have a constant value, but the child default node doesn't?

// tuples
statementList
└─statements
  └─block
    └─statements
      └─sequencePoint
        └─statementOpt
          └─returnStatement
            ├─refKind: None
            ├─expressionOpt
            │ └─call
            │   ├─receiverOpt
            │   ├─method: ImplicitConversionTargetType<(int SomeInt, bool SomeBool)?>.implicit operator ImplicitConversionTargetType<(int SomeInt, bool SomeBool)?>((int SomeInt, bool SomeBool)?)
            │   ├─arguments
            │   │ └─conversion
            │   │   ├─operand
            │   │   │ └─defaultExpression
            │   │   │   ├─targetType
            │   │   │   ├─constantValueOpt
            │   │   │   └─type: (int, bool)?
            │   │   ├─conversion: Identity
            │   │   ├─isBaseConversion: False
            │   │   ├─@checked: False
            │   │   ├─explicitCastInCode: False
            │   │   ├─constantValueOpt: ConstantValueNull(null: Nothing) // <--- this seems questionable
            │   │   ├─conversionGroupOpt: Microsoft.CodeAnalysis.CSharp.ConversionGroup
            │   │   ├─originalUserDefinedConversionsOpt: {}
            │   │   └─type: (int SomeInt, bool SomeBool)?
            │   ├─argumentNamesOpt: (null)
            │   ├─argumentRefKindsOpt: (null)
            │   ├─isDelegateCall: False
            │   ├─expanded: False
            │   ├─invokedAsExtensionMethod: False
            │   ├─argsToParamsOpt: (null)
            │   ├─defaultArguments: Microsoft.CodeAnalysis.BitVector
            │   ├─resultKind: Viable
            │   ├─originalMethodsOpt: (null)
            │   └─type: ImplicitConversionTargetType<(int SomeInt, bool SomeBool)?>
            └─@checked: False

// int
statementList
└─statements
  └─block
    └─statements
      └─sequencePoint
        └─statementOpt
          └─returnStatement
            ├─refKind: None
            ├─expressionOpt
            │ └─call
            │   ├─receiverOpt
            │   ├─method: ImplicitConversionTargetType<int?>.implicit operator ImplicitConversionTargetType<int?>(int?)
            │   ├─arguments
            │   │ └─defaultExpression
            │   │   ├─targetType
            │   │   ├─constantValueOpt
            │   │   └─type: int?
            │   ├─argumentNamesOpt: (null)
            │   ├─argumentRefKindsOpt: (null)
            │   ├─isDelegateCall: False
            │   ├─expanded: False
            │   ├─invokedAsExtensionMethod: False
            │   ├─argsToParamsOpt: (null)
            │   ├─defaultArguments: Microsoft.CodeAnalysis.BitVector
            │   ├─resultKind: Viable
            │   ├─originalMethodsOpt: (null)
            │   └─type: ImplicitConversionTargetType<int?>
            └─@checked: False

#Closed

[jcouv]

Looking at the spec (constants), the identity conversion of null should be a constant. That suggests the conversion node is correct (has constantValueOpt) but the default expression nodes are missing constantValueOpt. Should we file an issue on that?

[AlekseyTs]

Why does that conversion node have a constant value, but the child default node doesn't?

Because, strictly speaking, the original conversion node, created by initial binding, shouldn't have one either. In my opinion this an old bug in compiler's behavior, see #24380. Unfortunately, the presence of the constant value on the conversion was taken advantage of in implementation of some features (patterns, for example) and there was a resistance to get this cleaned up. Given the situation, various parts of the compiler have to compensate for that. For example, LocalRewriter has this code to ignore nullable constant values:

        private BoundExpression? VisitExpressionImpl(BoundExpression node)
        {
            ConstantValue? constantValue = node.ConstantValue;
            if (constantValue != null)
            {
                TypeSymbol? type = node.Type;
                if (type?.IsNullableType() != true)
                {
                    return MakeLiteral(node.Syntax, constantValue, type);
                }
            }

The proposed fix is essentially doing the same thing in IL generation. If we get consensus around fixing the #24380, we can do that during MQ milestone.

[jcouv]

Thanks for the clarification.
The link you shared might be incorrect though (seems like an unrelated merged PR).

[AlekseyTs]

The link you shared might be incorrect though (seems like an unrelated merged PR).

The first link is correct, the second reference is missing zero at the end, I'll fix

[AlekseyTs]

Looking at the spec (constants), the identity conversion of null should be a constant. That suggests the conversion node is correct (has constantValueOpt) but the default expression nodes are missing constantValueOpt. Should we file an issue on that?

I actually disagree with this analysis and think that, according to the specification, values of a nullable type should never have a constant value. This applies to a nullable null, and to a nullable default value. I think I expressed the rationale behind this opinion in #24380.

[jcouv]

values of a nullable type should never have a constant value

Reading the thread in #24380, I'm convinced. The allowed conversions for constants are enumerated, but the conversion of null to nullable value type (described here) isn't listed.
Hopefully we have a special rule somewhere for default parameter values void M(int? i = null), since this is not quite a constant.

Then from an implementation perspective, the constant value (non-null constantValueOpt) indeed would be the root cause. I'm still okay with localized workaround for now. Thanks

[jcouv]

LGTM Thanks (iteration 1)

[AlekseyTs]

@dotnet/roslyn-compiler For the second review.