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

Jump to bottom

Correctly handle conversions in null coalescing #52009

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Correctly handle conversions in null coalescing #52009

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a3e9ea8
Correctly handle conversions in null coalescing
333fred Mar 20, 2021
8a1787c
Correct conversion state handling.
333fred Mar 20, 2021
354acb7
Remove incorrect MemberNotNullAttribute, simplify and inline logic.
333fred Mar 26, 2021
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
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using System.Diagnostics.CodeAnalysis;

namespace Microsoft.CodeAnalysis.CSharp
{
Expand Down Expand Up @@ -634,6 +635,7 @@ public bool IsReference
/// <remarks>
/// Implicit and explicit user-defined conversions are described in section 6.4 of the C# language specification.
/// </remarks>
[MemberNotNullWhen(true, nameof(Method), nameof(MethodSymbol))]
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 22, 2021 •
edited by 333fred
Loading

Choose a reason for hiding this comment

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

nit: it feels like it could be good to change the implementation to something like:

var isUserDefined = Kind.IsUserDefinedConversion();
Debug.Assert(!isUserDefined || (Method is not null && MethodSymbol is not null))
return isUserDefined;

#Resolved

Copy link
Member Author

@333fred 333fred Mar 22, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

I don't believe this is necessary. When construction occurs, these conditions are validated. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

I was a bit thrown off by the use of MemberNotNullWhen here too because this method doesn't initialize or check Method or MethodSymbol.
Adding the assertion would make sense to me, but I'm not sure it would hold true.
Seems like we can create user-defined conversions that return null Method:

        internal Conversion(UserDefinedConversionResult conversionResult, bool isImplicit)
        {
            _kind = conversionResult.Kind == UserDefinedConversionResultKind.NoApplicableOperators
                ? ConversionKind.NoConversion
                : isImplicit ? ConversionKind.ImplicitUserDefined : ConversionKind.ExplicitUserDefined;

            _uncommonData = new UncommonData(
                isExtensionMethod: false,
                isArrayIndex: false,
                conversionResult: conversionResult,
                conversionMethod: null, // this is used for `Method`
                nestedConversions: default);
        }

In reply to: 599078854 [](ancestors = 599078854)

public bool IsUserDefined
{
get
Expand Down
57 changes: 37 additions & 20 deletions src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4146,7 +4146,7 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
TypeWithState rightResult = VisitOptionalImplicitConversion(rightOperand, targetType, useLegacyWarnings: UseLegacyWarnings(leftOperand, targetType), trackMembers: false, AssignmentKind.Assignment);
TrackNullableStateForAssignment(rightOperand, targetType, leftSlot, rightResult, MakeSlot(rightOperand));
Join(ref this.State, ref leftState);
TypeWithState resultType = GetNullCoalescingResultType(rightResult, targetType.Type);
TypeWithState resultType = GetNullCoalescingResultType(rightResult, TypeWithState.Create(targetType.Type, NullableFlowState.NotNull));
333fred marked this conversation as resolved.
Show resolved Hide resolved
SetResultType(node, resultType);
return null;
}
Expand Down Expand Up @@ -4180,8 +4180,6 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
SetUnreachable();
}

// https://github.com/dotnet/roslyn/issues/29955 For cases where the left operand determines
// the type, we should unwrap the right conversion and re-apply.
rightResult = VisitRvalueWithState(rightOperand);

Join(ref this.State, ref whenNotNull);
Expand All @@ -4202,47 +4200,66 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
var leftResultType = leftResult.Type;
var rightResultType = rightResult.Type;

var resultType = node.OperatorResultKind switch
var resultTypeWithLeftState = node.OperatorResultKind switch
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
BoundNullCoalescingOperatorResultKind.NoCommonType => node.Type,
BoundNullCoalescingOperatorResultKind.NoCommonType => TypeWithState.Create(node.Type, NullableFlowState.NotNull),
BoundNullCoalescingOperatorResultKind.LeftType => getLeftResultType(leftResultType!, rightResultType!),
BoundNullCoalescingOperatorResultKind.LeftUnwrappedType => getLeftResultType(leftResultType!.StrippedType(), rightResultType!),
BoundNullCoalescingOperatorResultKind.RightType => getRightResultType(leftResultType!, rightResultType!),
BoundNullCoalescingOperatorResultKind.LeftUnwrappedRightType => getRightResultType(leftResultType!.StrippedType(), rightResultType!),
BoundNullCoalescingOperatorResultKind.RightDynamicType => rightResultType!,
BoundNullCoalescingOperatorResultKind.RightType => getResultStateWithRightType(leftResultType!, rightResultType!),
BoundNullCoalescingOperatorResultKind.LeftUnwrappedRightType => getResultStateWithRightType(leftResultType!.StrippedType(), rightResultType!),
BoundNullCoalescingOperatorResultKind.RightDynamicType => TypeWithState.Create(rightResultType!, NullableFlowState.NotNull),
_ => throw ExceptionUtilities.UnexpectedValue(node.OperatorResultKind),
};

SetResultType(node, GetNullCoalescingResultType(rightResult, resultType));
SetResultType(node, GetNullCoalescingResultType(rightResult, resultTypeWithLeftState));
return null;

TypeSymbol getLeftResultType(TypeSymbol leftType, TypeSymbol rightType)
TypeWithState getLeftResultType(TypeSymbol leftType, TypeSymbol rightType)
{
Debug.Assert(rightType is object);
// If there was an identity conversion between the two operands (in short, if there
// is no implicit conversion on the right operand), then check nullable conversions
// in both directions since it's possible the right operand is the better result type.
if ((node.RightOperand as BoundConversion)?.ExplicitCastInCode != false &&
GenerateConversionForConditionalOperator(node.LeftOperand, leftType, rightType, reportMismatch: false).Exists)
GenerateConversionForConditionalOperator(node.LeftOperand, leftType, rightType, reportMismatch: false) is { Exists: true } conversion)
{
return rightType;
Debug.Assert(!conversion.IsUserDefined);
return TypeWithState.Create(rightType, NullableFlowState.NotNull);
}

GenerateConversionForConditionalOperator(node.RightOperand, rightType, leftType, reportMismatch: true);
return leftType;
conversion = GenerateConversionForConditionalOperator(node.RightOperand, rightType, leftType, reportMismatch: true);
Debug.Assert(!conversion.IsUserDefined);
return TypeWithState.Create(leftType, NullableFlowState.NotNull);
}

TypeSymbol getRightResultType(TypeSymbol leftType, TypeSymbol rightType)
TypeWithState getResultStateWithRightType(TypeSymbol leftType, TypeSymbol rightType)
{
GenerateConversionForConditionalOperator(node.LeftOperand, leftType, rightType, reportMismatch: true);
return rightType;
var conversion = GenerateConversionForConditionalOperator(node.LeftOperand, leftType, rightType, reportMismatch: true);
if (conversion.IsUserDefined)
{
return VisitConversion(
conversionOpt: null,
node.LeftOperand,
conversion,
TypeWithAnnotations.Create(rightType),
// When considering the conversion on the left node, it can only occur in the case where the underlying
// execution returned non-null
TypeWithState.Create(leftType, NullableFlowState.NotNull),
checkConversion: false,
fromExplicitCast: false,
useLegacyWarnings: false,
AssignmentKind.Assignment,
reportTopLevelWarnings: false,
reportRemainingWarnings: false);
}

return TypeWithState.Create(rightType, NullableFlowState.NotNull);
}
}

private static TypeWithState GetNullCoalescingResultType(TypeWithState rightResult, TypeSymbol resultType)
private static TypeWithState GetNullCoalescingResultType(TypeWithState rightResult, TypeWithState resultTypeWithLeftState)
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
NullableFlowState resultState = rightResult.State;
return TypeWithState.Create(resultType, resultState);
return TypeWithState.Create(resultTypeWithLeftState.Type, rightResult.State.Join(resultTypeWithLeftState.State));
}

public override BoundNode? VisitConditionalAccess(BoundConditionalAccess node)
Expand Down
234 changes: 234 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49924,6 +49924,240 @@ static object F((I<object>, I<object?>)? x, (I<object?>, I<object>) y)
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "y").WithArguments("(I<object?>, I<object>)", "(I<object>, I<object?>)").WithLocation(10, 21));
}

[Fact, WorkItem(51975, "https://github.com/dotnet/roslyn/issues/51975")]
public void NullCoalescingOperator_09()
{
var source =
@"C? c = new C();
D d = c ?? new D();

d.ToString();

class C {}

class D
{
public static implicit operator D?(C c) => default;
}
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (2,7): warning CS8600: Converting null literal or possible null value to non-nullable type.
// D d = c ?? new D();
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "c ?? new D()").WithLocation(2, 7),
// (4,1): warning CS8602: Dereference of a possibly null reference.
// d.ToString();
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "d").WithLocation(4, 1)
);
}

[Fact, WorkItem(51975, "https://github.com/dotnet/roslyn/issues/51975")]
public void NullCoalescingOperator_10()
{
var source =
@"C? c = new C();
D d = c ?? new D();

d.ToString();

class C
{
public static implicit operator D?(C c) => default;
}

class D {}
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (2,7): warning CS8600: Converting null literal or possible null value to non-nullable type.
// D d = c ?? new D();
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "c ?? new D()").WithLocation(2, 7),
// (4,1): warning CS8602: Dereference of a possibly null reference.
// d.ToString();
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "d").WithLocation(4, 1)
);
}

[Fact, WorkItem(51975, "https://github.com/dotnet/roslyn/issues/51975")]
public void NullCoalescingOperator_11()
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
var source =
@"C? c = new C();
D d = c ?? new D();

d.ToString();

struct C
{
public static implicit operator D?(C c) => default;
}

class D {}
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (2,7): warning CS8600: Converting null literal or possible null value to non-nullable type.
// D d = c ?? new D();
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "c ?? new D()").WithLocation(2, 7),
// (4,1): warning CS8602: Dereference of a possibly null reference.
// d.ToString();
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "d").WithLocation(4, 1)
);
}

[Fact, WorkItem(51975, "https://github.com/dotnet/roslyn/issues/51975")]
public void NullCoalescingOperator_12()
{
var source =
@"C? c = new C();
C c2 = c ?? new D();

c2.ToString();

class C
{
public static implicit operator C?(D c) => default;
}

class D {}
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (2,8): warning CS8600: Converting null literal or possible null value to non-nullable type.
// C c2 = c ?? new D();
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "c ?? new D()").WithLocation(2, 8),
// (4,1): warning CS8602: Dereference of a possibly null reference.
// c2.ToString();
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "c2").WithLocation(4, 1)
);
}

[Fact, WorkItem(29955, "https://github.com/dotnet/roslyn/issues/29955")]
public void NullCoalescingOperator_13()
{
var source =
@"C<string?> c = new C<string?>();
C<string?> c2 = c ?? new D<string>();

c2.ToString();

class C<T>
{
public static implicit operator C<T>(D<T> c) => default!;
}

class D<T> {}
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (2,22): warning CS8619: Nullability of reference types in value of type 'D<string>' doesn't match target type 'D<string?>'.
// C<string?> c2 = c ?? new D<string>();
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "new D<string>()").WithArguments("D<string>", "D<string?>").WithLocation(2, 22)
);
}

[Fact, WorkItem(29955, "https://github.com/dotnet/roslyn/issues/29955")]
public void NullCoalescingOperator_14()
{
var source =
@"using System;
Action<string?> a1 = param => {};
Action<string?> a2 = a1 ?? ((string s) => {});
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (3,29): warning CS8622: Nullability of reference types in type of parameter 's' of 'lambda expression' doesn't match the target delegate 'Action<string?>' (possibly because of nullability attributes).
// Action<string?> a2 = a1 ?? ((string s) => {});
Diagnostic(ErrorCode.WRN_NullabilityMismatchInParameterTypeOfTargetDelegate, "(string s) => {}").WithArguments("s", "lambda expression", "System.Action<string?>").WithLocation(3, 29)
);
}

[Fact, WorkItem(29955, "https://github.com/dotnet/roslyn/issues/29955")]
public void NullCoalescingOperator_15()
{
var source =
@"using System;
Action<string?> a1 = param => {};
Action<string?> a2 = a1 ?? (Action<string>)((string s) => {});
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (3,22): warning CS8619: Nullability of reference types in value of type 'Action<string>' doesn't match target type 'Action<string?>'.
// Action<string?> a2 = a1 ?? (Action<string>)((string s) => {});
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "a1 ?? (Action<string>)((string s) => {})").WithArguments("System.Action<string>", "System.Action<string?>").WithLocation(3, 22)
);
}

[Fact, WorkItem(29955, "https://github.com/dotnet/roslyn/issues/29955")]
public void NullCoalescingOperator_16()
{
var source =
@"using System;
Func<string> a1 = () => string.Empty;
Func<string> a2 = a1 ?? (() => null);
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (3,32): warning CS8603: Possible null reference return.
// Func<string> a2 = a1 ?? (() => null);
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "null").WithLocation(3, 32)
);
}

[Fact, WorkItem(29955, "https://github.com/dotnet/roslyn/issues/29955")]
public void NullCoalescingOperator_17()
{
var source =
@"using System;
Func<string> a1 = () => string.Empty;
Func<string> a2 = a1 ?? (Func<string?>)(() => null);
";

var comp = CreateCompilation(source, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics(
// (3,19): warning CS8619: Nullability of reference types in value of type 'Func<string?>' doesn't match target type 'Func<string>'.
// Func<string> a2 = a1 ?? (Func<string?>)(() => null);
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "a1 ?? (Func<string?>)(() => null)").WithArguments("System.Func<string?>", "System.Func<string>").WithLocation(3, 19)
);
}

[Fact, WorkItem(51975, "https://github.com/dotnet/roslyn/issues/51975")]
public void NullCoalescingOperator_18()
{
var source =
@"using System.Diagnostics.CodeAnalysis;
C? c = new C();
D d1 = c ?? new D();
d1.ToString();

D d2 = ((C?)null) ?? new D();
d2.ToString();

c = null;
D d3 = c ?? new D();
d3.ToString();

class C {}

class D
{
[return: NotNullIfNotNull(""c"")]
public static implicit operator D?(C c) => default!;
}
";

var comp = CreateCompilation(new[] { source, NotNullIfNotNullAttributeDefinition }, options: WithNullableEnable(TestOptions.ReleaseExe));
comp.VerifyDiagnostics();
}
333fred marked this conversation as resolved.
Show resolved Hide resolved

[Fact]
public void IdentityConversion_NullCoalescingOperator_01()
{
Expand Down