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.

Sign up for GitHub

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 treat Nullable null value as a constant for the purpose of IL generation. #64789

Merged
merged 1 commit into from
Oct 18, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ private void EmitExpression(BoundExpression expression, bool used)
return;
}

if ((object)expression.Type == null || expression.Type.SpecialType != SpecialType.System_Decimal)
if ((object)expression.Type == null ||
(expression.Type.SpecialType != SpecialType.System_Decimal &&
!expression.Type.IsNullableType()))
Copy link
Member

@jcouv jcouv Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jcouv jcouv Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@AlekseyTs AlekseyTs Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AlekseyTs AlekseyTs Oct 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
EmitConstantExpression(expression.Type, constantValue, used, expression.Syntax);
return;
Expand Down
62 changes: 62 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29041,5 +29041,67 @@ static void verifyDefaultFieldType(FieldSymbol tupleField, int i, string nullabi
tupleField.CorrespondingTupleField.ToTestDisplayString(includeNonNullable: true));
}
}

[Fact]
[WorkItem(64777, "https://github.com/dotnet/roslyn/issues/64777")]
public void NameMismatchInUserDefinedConversion()
{
var source = @"
class C
{
static void Main()
{
System.Console.WriteLine(""---"");
System.Console.WriteLine(Test1().Property is null);
System.Console.WriteLine(Test2().Property is null);
System.Console.WriteLine(""---"");
}

static ImplicitConversionTargetType<(int, bool)?> Test1() => ((int, bool)?) null;
static ImplicitConversionTargetType<(int SomeInt, bool SomeBool)?> Test2()=> ((int, bool)?) null;
}

public class ImplicitConversionTargetType<T>
{
public T Property { get; }

public ImplicitConversionTargetType(T property) { Property = property; }

public static implicit operator ImplicitConversionTargetType<T>(T operand) => new(operand);
}
";

var verifier = CompileAndVerify(source + trivial2uple, expectedOutput:
@"
---
True
True
---
").VerifyDiagnostics();

verifier.VerifyIL("C.Test1", @"
{
// Code size 15 (0xf)
.maxstack 1
.locals init (System.ValueTuple<int, bool>? V_0)
IL_0000: ldloca.s V_0
IL_0002: initobj ""System.ValueTuple<int, bool>?""
IL_0008: ldloc.0
IL_0009: call ""ImplicitConversionTargetType<System.ValueTuple<int, bool>?> ImplicitConversionTargetType<System.ValueTuple<int, bool>?>.op_Implicit(System.ValueTuple<int, bool>?)""
IL_000e: ret
}");

verifier.VerifyIL("C.Test2", @"
{
// Code size 15 (0xf)
.maxstack 1
.locals init (System.ValueTuple<int, bool>? V_0)
IL_0000: ldloca.s V_0
IL_0002: initobj ""System.ValueTuple<int, bool>?""
IL_0008: ldloc.0
IL_0009: call ""ImplicitConversionTargetType<System.ValueTuple<int, bool>?> ImplicitConversionTargetType<System.ValueTuple<int, bool>?>.op_Implicit(System.ValueTuple<int, bool>?)""
IL_000e: ret
}");
}
}
}
83 changes: 83 additions & 0 deletions src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenTuples.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23300,6 +23300,89 @@ End Module
Dim comp1 = CreateCompilation(source0, options:=TestOptions.DebugExe)
CompileAndVerify(comp1, expectedOutput:="Done")
End Sub

<Fact>
<WorkItem(64777, "https://github.com/dotnet/roslyn/issues/64777")>
Public Sub NameMismatchInUserDefinedConversion()

Dim source =
"
class C
shared Sub Main()
System.Console.WriteLine(""---"")
System.Console.WriteLine(Test1().Property is nothing)
System.Console.WriteLine(Test2().Property is nothing)
System.Console.WriteLine(""---"")
End Sub

Shared Function Test1() As ImplicitConversionTargetType(Of (Integer, Boolean)?)
return CType(Nothing, (Integer, Boolean)?)
End Function

Shared Function Test2() As ImplicitConversionTargetType(Of (SomeInt As Integer, SomeBool As Boolean)?)
return CType(Nothing, (Integer, Boolean)?)
End Function
End Class

public class ImplicitConversionTargetType(Of T)
public readonly Property [Property] As T

public Sub New([property] As T)
Me.Property = [property]
End Sub

public shared widening operator CType(operand As T) As ImplicitConversionTargetType(Of T)
return new ImplicitConversionTargetType(Of T)(operand)
End Operator
End Class
"

Dim compilation = CreateCompilation(source + s_trivial2uple, options:=TestOptions.DebugExe)

Dim verifier = CompileAndVerify(compilation, expectedOutput:=
"
---
True
True
---
").VerifyDiagnostics()

verifier.VerifyIL("C.Test1", <![CDATA[
{
// Code size 20 (0x14)
.maxstack 1
.locals init (ImplicitConversionTargetType(Of (Integer, Boolean)?) V_0, //Test1
(Integer, Boolean)? V_1)
IL_0000: nop
IL_0001: ldloca.s V_1
IL_0003: initobj "(Integer, Boolean)?"
IL_0009: ldloc.1
IL_000a: call "Function ImplicitConversionTargetType(Of (Integer, Boolean)?).op_Implicit((Integer, Boolean)?) As ImplicitConversionTargetType(Of (Integer, Boolean)?)"
IL_000f: stloc.0
IL_0010: br.s IL_0012
IL_0012: ldloc.0
IL_0013: ret
}
]]>)

verifier.VerifyIL("C.Test2", <![CDATA[
{
// Code size 20 (0x14)
.maxstack 1
.locals init (ImplicitConversionTargetType(Of (SomeInt As Integer, SomeBool As Boolean)?) V_0, //Test2
(Integer, Boolean)? V_1)
IL_0000: nop
IL_0001: ldloca.s V_1
IL_0003: initobj "(Integer, Boolean)?"
IL_0009: ldloc.1
IL_000a: call "Function ImplicitConversionTargetType(Of (SomeInt As Integer, SomeBool As Boolean)?).op_Implicit((SomeInt As Integer, SomeBool As Boolean)?) As ImplicitConversionTargetType(Of (SomeInt As Integer, SomeBool As Boolean)?)"
IL_000f: stloc.0
IL_0010: br.s IL_0012
IL_0012: ldloc.0
IL_0013: ret
}
]]>)
End Sub
End Class

End Namespace
Expand Down