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

Small nullability fixes #49279

Merged
merged 7 commits into from
Nov 14, 2020
Merged

Small nullability fixes #49279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
df60b06
Small nullability fixes
jcouv Nov 11, 2020
37c5d73
Remove unused method
jcouv Nov 12, 2020
92791ef
nameof
jcouv Nov 13, 2020
e59706c
Move logic into VisitConversion
jcouv Nov 13, 2020
c24481e
Revert "Move logic into VisitConversion"
jcouv Nov 13, 2020
d19baee
Comment
jcouv Nov 13, 2020
96e503f
Simplify condition
jcouv Nov 14, 2020
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/FlowAnalysis/NullableWalker.DebugVerifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ private void VerifyExpression(BoundExpression expression, bool overrideSkippedEx
{
Visit(node.IterationVariableType);
Visit(node.AwaitOpt);
Visit(node.EnumeratorInfoOpt?.DisposeAwaitableInfo);
Visit(node.Expression);
// https://github.com/dotnet/roslyn/issues/35010: handle the deconstruction
//this.Visit(node.DeconstructionOpt);
Expand Down
66 changes: 53 additions & 13 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 @@ -2739,7 +2739,6 @@ private void VisitLocalFunctionUse(LocalFunctionSymbol symbol)
public override BoundNode? VisitForEachStatement(BoundForEachStatement node)
{
DeclareLocals(node.IterationVariables);
Visit(node.AwaitOpt);
return base.VisitForEachStatement(node);
}

Expand Down Expand Up @@ -5328,6 +5327,17 @@ private void VisitArgumentConversionAndInboundAssignmentsAndPreConditions(
case RefKind.In:
{
// Note: for lambda arguments, they will be converted in the context/state we saved for that argument
if (conversion is { Kind: ConversionKind.ImplicitUserDefined })
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

@cston cston Nov 14, 2020 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

if (conversion is { Kind: ConversionKind.ImplicitUserDefined }) [](start = 24, length = 63)

It's not clear to me why this logic is specific to argument conversions. Won't we have the same requirements for assignments? #Resolved

Copy link
Member Author

@jcouv jcouv Nov 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In an assignment, we know the target-type (if there is to be a user-defined conversion). That target-type doesn't change because of nullable analysis, so the user-defined conversion is still the right one.
In a call, the target-type comes from re-inferring the type arguments for the method as part of nullable analysis. #Resolved

{
var argumentResultType = resultType.Type;
conversion = GenerateConversion(_conversions, argumentNoConversion, argumentResultType, parameterType.Type, fromExplicitCast: false, extensionMethodThisArgument: false);
if (!conversion.Exists && !argumentNoConversion.IsSuppressed)
{
Debug.Assert(argumentResultType is not null);
ReportNullabilityMismatchInArgument(argumentNoConversion.Syntax, argumentResultType, parameter, parameterType.Type, forOutput: false);
}
}

var stateAfterConversion = VisitConversion(
conversionOpt: conversionOpt,
conversionOperand: argumentNoConversion,
Expand Down Expand Up @@ -6392,17 +6402,6 @@ private void TrackNullableStateOfNullableValue(int containingSlot, TypeSymbol co
}
}

private void TrackNullableStateOfNullableValue(BoundExpression node, BoundExpression operand, TypeSymbol convertedType, TypeWithAnnotations underlyingType)
{
int valueSlot = MakeSlot(operand);
if (valueSlot > 0)
{
int containingSlot = GetOrCreatePlaceholderSlot(node);
Debug.Assert(containingSlot > 0);
TrackNullableStateOfNullableValue(containingSlot, convertedType, operand, underlyingType.ToTypeWithState(), valueSlot);
}
}

private void TrackNullableStateOfTupleConversion(
BoundConversion? conversionOpt,
BoundExpression convertedNode,
Expand Down Expand Up @@ -8571,6 +8570,41 @@ protected override void VisitForEachExpression(BoundForEachStatement node)
currentPropertyGetterTypeWithState = ApplyUnconditionalAnnotations(
currentPropertyGetter.ReturnTypeWithAnnotations.ToTypeWithState(),
currentPropertyGetter.ReturnTypeFlowAnalysisAnnotations);

// Analyze `await MoveNextAsync()`
if (node.AwaitOpt is { AwaitableInstancePlaceholder: BoundAwaitableValuePlaceholder moveNextPlaceholder } awaitMoveNextInfo)
{
var moveNextAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, node.EnumeratorInfoOpt.MoveNextMethod);

EnsureAwaitablePlaceholdersInitialized();
var result = new VisitResult(GetReturnTypeWithState(moveNextAsyncMethod), moveNextAsyncMethod.ReturnTypeWithAnnotations);
_awaitablePlaceholdersOpt.Add(moveNextPlaceholder, (moveNextPlaceholder, result));
Visit(awaitMoveNextInfo);
_awaitablePlaceholdersOpt.Remove(moveNextPlaceholder);
}

// Analyze `await DisposeAsync()`
if (node.EnumeratorInfoOpt is { NeedsDisposal: true, DisposeAwaitableInfo: BoundAwaitableInfo awaitDisposalInfo })
{
var disposalPlaceholder = awaitDisposalInfo.AwaitableInstancePlaceholder;
bool addedPlaceholder = false;
if (node.EnumeratorInfoOpt.DisposeMethod is not null) // no statically known Dispose method if doing a runtime check
{
Debug.Assert(disposalPlaceholder is not null);
var disposeAsyncMethod = (MethodSymbol)AsMemberOfType(reinferredGetEnumeratorMethod.ReturnType, node.EnumeratorInfoOpt.DisposeMethod);
EnsureAwaitablePlaceholdersInitialized();
var result = new VisitResult(GetReturnTypeWithState(disposeAsyncMethod), disposeAsyncMethod.ReturnTypeWithAnnotations);
_awaitablePlaceholdersOpt.Add(disposalPlaceholder, (disposalPlaceholder, result));
addedPlaceholder = true;
}

Visit(awaitDisposalInfo);

if (addedPlaceholder)
{
_awaitablePlaceholdersOpt!.Remove(disposalPlaceholder!);
}
}
}

SetResultType(expression: null, currentPropertyGetterTypeWithState);
Expand Down Expand Up @@ -8921,7 +8955,7 @@ private TypeWithState InferResultNullabilityOfBinaryLogicalOperator(BoundExpress
var placeholder = awaitableInfo.AwaitableInstancePlaceholder;
Debug.Assert(placeholder is object);

_awaitablePlaceholdersOpt ??= PooledDictionary<BoundAwaitableValuePlaceholder, (BoundExpression AwaitableExpression, VisitResult Result)>.GetInstance();
EnsureAwaitablePlaceholdersInitialized();
_awaitablePlaceholdersOpt.Add(placeholder, (node.Expression, _visitResult));
Visit(awaitableInfo);
_awaitablePlaceholdersOpt.Remove(placeholder);
Expand Down Expand Up @@ -9512,6 +9546,12 @@ protected override void VisitCatchBlock(BoundCatchBlock node, ref LocalState fin
return null;
}

[MemberNotNull(nameof(_awaitablePlaceholdersOpt))]
private void EnsureAwaitablePlaceholdersInitialized()
{
_awaitablePlaceholdersOpt ??= PooledDictionary<BoundAwaitableValuePlaceholder, (BoundExpression AwaitableExpression, VisitResult Result)>.GetInstance();
}

public override BoundNode? VisitAwaitableValuePlaceholder(BoundAwaitableValuePlaceholder node)
{
if (_awaitablePlaceholdersOpt != null && _awaitablePlaceholdersOpt.TryGetValue(node, out var value))
Expand Down
178 changes: 165 additions & 13 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 @@ -60761,7 +60761,7 @@ static void G(A<string> a2, B b2)
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "b2").WithArguments("B", "IOut<object>").WithLocation(19, 13));
}

[Fact]
[Fact, WorkItem(29898, "https://github.com/dotnet/roslyn/issues/29898")]
public void ImplicitConversions_07()
{
var source =
Expand All @@ -60781,13 +60781,107 @@ static void Main(object? x)
var y = F(x);
G(y);
if (x == null) return;
var z = F(x);
var z = F(x);
G(z); // warning

var z2 = F(x);
G(z2!);
}
}";
var comp = CreateCompilation(new[] { source }, options: WithNonNullTypesTrue());
// https://github.com/dotnet/roslyn/issues/29898: Report warning for `G(z)`?
comp.VerifyDiagnostics();
comp.VerifyDiagnostics(
// (18,11): warning CS8620: Argument of type 'B<object>' cannot be used for parameter 'a' of type 'A<object?>' in 'void C.G(A<object?> a)' due to differences in the nullability of reference types.
// G(z); // warning
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "z").WithArguments("B<object>", "A<object?>", "a", "void C.G(A<object?> a)").WithLocation(18, 11)
);
}

[Fact]
public void ImplicitConversion_Params()
{
var source =
@"class A<T>
{
}
class B<T>
{
public static implicit operator A<T>(B<T> b) => throw null!;
}
class C
{
static B<T> F<T>(T t) => throw null!;
static void G(params A<object>[] a) => throw null!;

static void Main(object? x)
{
var y = F(x);
G(y); // 1

if (x == null) return;
var z = F(x);
G(z);
}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (16,11): warning CS8620: Argument of type 'B<object?>' cannot be used for parameter 'a' of type 'A<object>' in 'void C.G(params A<object>[] a)' due to differences in the nullability of reference types.
// G(y); // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "y").WithArguments("B<object?>", "A<object>", "a", "void C.G(params A<object>[] a)").WithLocation(16, 11)
);
}

[Fact]
public void ImplicitConversion_Typeless()
{
var source = @"
public struct Optional<T>
{
public static implicit operator Optional<T>(T value) => throw null!;
}

class C
{
static void G1(Optional<object> a) => throw null!;
static void G2(Optional<object?> a) => throw null!;

static void M()
{
G1(null); // 1
G2(null);
}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (14,12): warning CS8625: Cannot convert null literal to non-nullable reference type.
// G1(null); // 1
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(14, 12)
);
}

[Fact]
public void ImplicitConversion_Typeless_WithConstraint()
{
var source = @"
public struct Optional<T> where T : class
{
public static implicit operator Optional<T>(T value) => throw null!;
}

class C
{
static void G(Optional<object> a) => throw null!;

static void M()
{
G(null);
}
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (13,11): warning CS8625: Cannot convert null literal to non-nullable reference type.
// G(null);
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(13, 11)
);
}

[Fact, WorkItem(41763, "https://github.com/dotnet/roslyn/issues/41763")]
Expand Down Expand Up @@ -82657,10 +82751,11 @@ static void F3((B?, B) t)
}";
var comp = CreateCompilation(source, options: WithNonNullTypesTrue());
// https://github.com/dotnet/roslyn/issues/32599: Handle tuple element conversions.
// Diagnostics on user-defined conversions need improvement: https://github.com/dotnet/roslyn/issues/31798
comp.VerifyDiagnostics(
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
// (16,12): warning CS8620: Argument of type '(A?, A)?' cannot be used for parameter 't' of type '(A, A?)?' in 'void Program.F2((A, A?)? t)' due to differences in the nullability of reference types.
// (16,12): warning CS8620: Argument of type 'S' cannot be used for parameter 't' of type '(A, A?)?' in 'void Program.F2((A, A?)? t)' due to differences in the nullability of reference types.
// F2(s); // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "s").WithArguments("(A?, A)?", "(A, A?)?", "t", "void Program.F2((A, A?)? t)").WithLocation(16, 12));
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "s").WithArguments("S", "(A, A?)?", "t", "void Program.F2((A, A?)? t)").WithLocation(16, 12));
}

[Fact]
Expand Down Expand Up @@ -117092,9 +117187,9 @@ static void Main()
// (12,15): warning CS8619: Nullability of reference types in value of type 'A<object?>' doesn't match target type 'A<object>'.
// B b = a; // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "a").WithArguments("A<object?>", "A<object>").WithLocation(12, 15),
// (13,11): warning CS8620: Nullability of reference types in argument of type 'A<object?>' doesn't match target type 'A<object>' for parameter 'b' in 'void Program.F(B b)'.
// (13,11): warning CS8620: Argument of type 'A<object?>' cannot be used for parameter 'b' of type 'B' in 'void Program.F(B b)' due to differences in the nullability of reference types.
// F(a); // 2
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "a").WithArguments("A<object?>", "A<object>", "b", "void Program.F(B b)").WithLocation(13, 11));
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "a").WithArguments("A<object?>", "B", "b", "void Program.F(B b)").WithLocation(13, 11));
}

[Fact]
Expand Down Expand Up @@ -117122,9 +117217,9 @@ static void Main()
// (12,24): warning CS8619: Nullability of reference types in value of type 'A<object>' doesn't match target type 'A<object?>'.
// A<object?> a = b; // 1
Diagnostic(ErrorCode.WRN_NullabilityMismatchInAssignment, "b").WithArguments("A<object>", "A<object?>").WithLocation(12, 24),
// (13,11): warning CS8620: Nullability of reference types in argument of type 'A<object>' doesn't match target type 'A<object?>' for parameter 'a' in 'void Program.F(A<object?> a)'.
// (13,11): warning CS8620: Argument of type 'B' cannot be used for parameter 'a' of type 'A<object?>' in 'void Program.F(A<object?> a)' due to differences in the nullability of reference types.
// F(b); // 2
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "b").WithArguments("A<object>", "A<object?>", "a", "void Program.F(A<object?> a)").WithLocation(13, 11));
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "b").WithArguments("B", "A<object?>", "a", "void Program.F(A<object?> a)").WithLocation(13, 11));
}

[WorkItem(31864, "https://github.com/dotnet/roslyn/issues/31864")]
Expand Down Expand Up @@ -132292,17 +132387,74 @@ static async Task Main()
await foreach (var o in Create(x)) // 2
{
}
await foreach (var o in Create(y))
await foreach (var o in Create(y)) // 3
{
}
}
}";
var comp = CreateCompilationWithTasksExtensions(new[] { s_IAsyncEnumerable, source });
// Should report warning for GetAwaiter().
comp.VerifyDiagnostics(
// (24,20): warning CS8600: Converting null literal or possible null value to non-nullable type.
// object y = null; // 1
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(24, 20));
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(24, 20),
// (25,33): warning CS8620: Argument of type 'StructAwaitable1<object>' cannot be used for parameter 's' of type 'StructAwaitable1<object?>' in 'TaskAwaiter<bool> Program.GetAwaiter(StructAwaitable1<object?> s)' due to differences in the nullability of reference types.
// await foreach (var o in Create(x)) // 2
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "Create(x)").WithArguments("StructAwaitable1<object>", "StructAwaitable1<object?>", "s", "TaskAwaiter<bool> Program.GetAwaiter(StructAwaitable1<object?> s)").WithLocation(25, 33),
// (28,33): warning CS8620: Argument of type 'StructAwaitable2<object?>' cannot be used for parameter 's' of type 'StructAwaitable2<object>' in 'TaskAwaiter Program.GetAwaiter(StructAwaitable2<object> s)' due to differences in the nullability of reference types.
// await foreach (var o in Create(y)) // 3
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "Create(y)").WithArguments("StructAwaitable2<object?>", "StructAwaitable2<object>", "s", "TaskAwaiter Program.GetAwaiter(StructAwaitable2<object> s)").WithLocation(28, 33)
);
}

[Fact]
[WorkItem(30956, "https://github.com/dotnet/roslyn/issues/30956")]
public void GetAwaiterExtensionMethod_AwaitForEach_InverseAnnotations()
{
var source =
@"#nullable enable
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
struct StructAwaitable1<T> { }
struct StructAwaitable2<T> { }
class Enumerable<T>
{
public Enumerator<T> GetAsyncEnumerator() => new Enumerator<T>();
}
class Enumerator<T>
{
public object Current => null!;
public StructAwaitable1<T> MoveNextAsync() => new StructAwaitable1<T>();
public StructAwaitable2<T> DisposeAsync() => new StructAwaitable2<T>();
}
static class Program
{
static TaskAwaiter<bool> GetAwaiter(this StructAwaitable1<object> s) => default;
static TaskAwaiter GetAwaiter(this StructAwaitable2<object?> s) => default;
static Enumerable<T> Create<T>(T t) => new Enumerable<T>();
static async Task Main()
{
object? x = new object();
object y = null; // 1
await foreach (var o in Create(x)) // 2
{
}
await foreach (var o in Create(y)) // 3
{
}
}
}";
var comp = CreateCompilationWithTasksExtensions(new[] { s_IAsyncEnumerable, source });
comp.VerifyDiagnostics(
// (24,20): warning CS8600: Converting null literal or possible null value to non-nullable type.
// object y = null; // 1
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(24, 20),
// (25,33): warning CS8620: Argument of type 'StructAwaitable2<object>' cannot be used for parameter 's' of type 'StructAwaitable2<object?>' in 'TaskAwaiter Program.GetAwaiter(StructAwaitable2<object?> s)' due to differences in the nullability of reference types.
// await foreach (var o in Create(x)) // 2
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "Create(x)").WithArguments("StructAwaitable2<object>", "StructAwaitable2<object?>", "s", "TaskAwaiter Program.GetAwaiter(StructAwaitable2<object?> s)").WithLocation(25, 33),
// (28,33): warning CS8620: Argument of type 'StructAwaitable1<object?>' cannot be used for parameter 's' of type 'StructAwaitable1<object>' in 'TaskAwaiter<bool> Program.GetAwaiter(StructAwaitable1<object> s)' due to differences in the nullability of reference types.
// await foreach (var o in Create(y)) // 3
Diagnostic(ErrorCode.WRN_NullabilityMismatchInArgument, "Create(y)").WithArguments("StructAwaitable1<object?>", "StructAwaitable1<object>", "s", "TaskAwaiter<bool> Program.GetAwaiter(StructAwaitable1<object> s)").WithLocation(28, 33)
);
}

[Fact]
Expand Down