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 all 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
1 change: 1 addition & 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
60 changes: 37 additions & 23 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 = TypeWithState.Create(targetType.Type, rightResult.State);
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,63 @@ private static BoundExpression SkipReferenceConversions(BoundExpression possibly
var leftResultType = leftResult.Type;
var rightResultType = rightResult.Type;

var resultType = node.OperatorResultKind switch
var (resultType, leftState) = node.OperatorResultKind switch
{
BoundNullCoalescingOperatorResultKind.NoCommonType => node.Type,
BoundNullCoalescingOperatorResultKind.NoCommonType => (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 => (rightResultType!, NullableFlowState.NotNull),
_ => throw ExceptionUtilities.UnexpectedValue(node.OperatorResultKind),
};

SetResultType(node, GetNullCoalescingResultType(rightResult, resultType));
SetResultType(node, TypeWithState.Create(resultType, rightResult.State.Join(leftState)));
return null;

TypeSymbol getLeftResultType(TypeSymbol leftType, TypeSymbol rightType)
(TypeSymbol ResultType, NullableFlowState LeftState) 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 (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 (leftType, NullableFlowState.NotNull);
}

TypeSymbol getRightResultType(TypeSymbol leftType, TypeSymbol rightType)
(TypeSymbol ResultType, NullableFlowState LeftState) 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)
{
var conversionResult = 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);
Debug.Assert(conversionResult.Type is not null);
return (conversionResult.Type!, conversionResult.State);
}

private static TypeWithState GetNullCoalescingResultType(TypeWithState rightResult, TypeSymbol resultType)
{
NullableFlowState resultState = rightResult.State;
return TypeWithState.Create(resultType, resultState);
return (rightType, NullableFlowState.NotNull);
}
}

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