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

Let null contribute its nullability to method type re-inference #43571

Closed
wants to merge 1 commit into from
Closed

Let null contribute its nullability to method type re-inference #43571

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
37 changes: 23 additions & 14 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 @@ -4397,19 +4397,19 @@ private ImmutableArray<VisitArgumentResult> VisitArguments(
Debug.Assert(!arguments.IsDefault);
bool shouldReturnNotNull = false;

(ImmutableArray<BoundExpression> argumentsNoConversions, ImmutableArray<Conversion> conversions) = RemoveArgumentConversions(arguments, refKindsOpt);
(ImmutableArray<BoundExpression> argumentsMostlyNoConversions, ImmutableArray<Conversion> conversions) = RemoveArgumentConversions(arguments, refKindsOpt);

// Visit the arguments and collect results
ImmutableArray<VisitArgumentResult> results;
(results, argumentsNoConversions, argsToParamsOpt, refKindsOpt) = VisitArgumentsEvaluate(node.Syntax, argumentsNoConversions, refKindsOpt, parametersOpt, argsToParamsOpt, expanded);
(results, argumentsMostlyNoConversions, argsToParamsOpt, refKindsOpt) = VisitArgumentsEvaluate(node.Syntax, argumentsMostlyNoConversions, refKindsOpt, parametersOpt, argsToParamsOpt, expanded);

// Re-infer method type parameters
if ((object)method != null && method.IsGenericMethod)
{
if (HasImplicitTypeArguments(node))
{
var binder = (node as BoundCall)?.BinderOpt ?? (node as BoundCollectionElementInitializer)?.BinderOpt ?? throw ExceptionUtilities.UnexpectedValue(node);
method = InferMethodTypeArguments(binder, method, GetArgumentsForMethodTypeInference(results, argumentsNoConversions), refKindsOpt, argsToParamsOpt, expanded);
method = InferMethodTypeArguments(binder, method, GetArgumentsForMethodTypeInference(results, argumentsMostlyNoConversions), refKindsOpt, argsToParamsOpt, expanded);
parametersOpt = method.Parameters;
}
if (ConstraintsHelper.RequiresChecking(method))
Expand All @@ -4434,11 +4434,11 @@ private ImmutableArray<VisitArgumentResult> VisitArguments(
continue;
}

var argumentNoConversion = argumentsNoConversions[i];
var argument = i < arguments.Length ? arguments[i] : argumentsNoConversions[i];
var argumentMostlyNoConversion = argumentsMostlyNoConversions[i];
var argument = i < arguments.Length ? arguments[i] : argumentMostlyNoConversion;
VisitArgumentConversionAndInboundAssignmentsAndPreConditions(
GetConversionIfApplicable(argument, argumentNoConversion),
argumentNoConversion,
GetConversionIfApplicable(argument, argumentMostlyNoConversion, avoidTypelessExpressions: true),
argumentMostlyNoConversion,
conversions.IsDefault || i >= conversions.Length ? Conversion.Identity : conversions[i],
GetRefKind(refKindsOpt, i),
parameter,
Expand Down Expand Up @@ -4498,8 +4498,8 @@ private ImmutableArray<VisitArgumentResult> VisitArguments(
// so just assume that the conversions have the same nullability as the underlying result
var argument = arguments[i];
var result = results[i];
var argumentNoConversion = argumentsNoConversions[i];
TrackAnalyzedNullabilityThroughConversionGroup(TypeWithState.Create(argument.Type, result.RValueType.State), argument as BoundConversion, argumentNoConversion);
var argumentMostlyNoConversion = argumentsMostlyNoConversions[i];
TrackAnalyzedNullabilityThroughConversionGroup(TypeWithState.Create(argument.Type, result.RValueType.State), argument as BoundConversion, argumentMostlyNoConversion);
}
}

Expand Down Expand Up @@ -5111,7 +5111,7 @@ void learnFromPostConditions(BoundExpression argument, FlowAnalysisAnnotations p
if (refKind == RefKind.None)
{
var before = argument;
(argument, conversion) = RemoveConversion(argument, includeExplicitConversions: false);
(argument, conversion) = RemoveConversion(argument, includeExplicitConversions: false, avoidTypelessExpressions: true);
if (argument != before)
{
SnapshotWalkerThroughConversionGroup(before, argument);
Expand Down Expand Up @@ -5395,7 +5395,7 @@ private void CheckMethodConstraints(SyntaxNode syntax, MethodSymbol method)
/// (Currently, the only visit method that passes `includeExplicitConversions: true`
/// is VisitConversion. All other callers are handling implicit conversions only.)
/// </summary>
private static (BoundExpression expression, Conversion conversion) RemoveConversion(BoundExpression expr, bool includeExplicitConversions)
private static (BoundExpression expression, Conversion conversion) RemoveConversion(BoundExpression expr, bool includeExplicitConversions, bool avoidTypelessExpressions = false)
{
ConversionGroup group = null;
while (true)
Expand All @@ -5404,7 +5404,16 @@ private static (BoundExpression expression, Conversion conversion) RemoveConvers
{
break;
}

var conversion = (BoundConversion)expr;

if (avoidTypelessExpressions &&
conversion.ConversionKind == ConversionKind.ImplicitReference &&
conversion.Operand is BoundLiteral { Type: null })
Copy link
Member

Choose a reason for hiding this comment

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

It feels like the current behavior of null is expected: we pass an untyped BoundLiteral to MethodTypeInferrer. But the behavior of default is unexpected: we pass a typed BoundDefaultExpression to MethodTypeInferrer where the type was only determined after running MethodTypeInferrer in initial binding.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, you're saying we should keep the current behavior for null (Infer("", null) would infer string! and warn, rather than string?)?

Copy link
Member

@cston cston Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not necessarily. I'm suggesting the arguments passed to MethodTypeInferrer in NullableWalker should match the typed/untyped-ness of the arguments passed to MethodTypeInferrer in initial binding. So null and default would both be passed as untyped arguments. Could NullableWalker fix up the nullability of the inferred type arguments after MethodTypeInferrer returns? Such an approach would also need to handle lambda return types so it might get complicated.

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

{
break;
}

if (group != conversion.ConversionGroupOpt && group != null)
{
// E.g.: (C)(B)a
Expand Down Expand Up @@ -6044,13 +6053,13 @@ private bool HasTopLevelNullabilityConversion(TypeWithAnnotations source, TypeWi
/// Gets the conversion node for passing to <see cref="VisitConversion(BoundConversion, BoundExpression, Conversion, TypeWithAnnotations, TypeWithState, bool, bool, bool, AssignmentKind, ParameterSymbol, bool, bool, bool, Optional{LocalState}, bool, Location)"/>,
/// if one should be passed.
/// </summary>
private static BoundConversion GetConversionIfApplicable(BoundExpression conversionOpt, BoundExpression convertedNode)
private static BoundConversion GetConversionIfApplicable(BoundExpression conversionOpt, BoundExpression convertedNode, bool avoidTypelessExpressions = false)
{
Debug.Assert(conversionOpt is null
|| convertedNode == conversionOpt // Note that convertedNode itself can be a BoundConversion, so we do this check explicitly
// because the below calls to RemoveConversion could potentially strip that conversion.
|| convertedNode == RemoveConversion(conversionOpt, includeExplicitConversions: false).expression
|| convertedNode == RemoveConversion(conversionOpt, includeExplicitConversions: true).expression);
|| convertedNode == RemoveConversion(conversionOpt, includeExplicitConversions: false, avoidTypelessExpressions).expression
|| convertedNode == RemoveConversion(conversionOpt, includeExplicitConversions: true, avoidTypelessExpressions).expression);
return conversionOpt == convertedNode ? null : (BoundConversion)conversionOpt;
}

Expand Down
154 changes: 149 additions & 5 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 @@ -4045,6 +4045,147 @@ void M<T>(T t)
);
}

[Fact, WorkItem(43536, "https://github.com/dotnet/roslyn/issues/43536")]
public void MethodTypeInference_WithDefaultAndNull()
{
var source = @"
class C
{
void M(string s)
{
Infer(s, default).ToString(); // 1
Infer(s, null).ToString(); // 2
}

T Infer<T>(T t1, T t2) => throw null!;
}";

var comp = CreateNullableCompilation(source);
comp.VerifyDiagnostics(
// (6,9): warning CS8602: Dereference of a possibly null reference.
// Infer(s, default).ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(s, default)").WithLocation(6, 9),
// (7,9): warning CS8602: Dereference of a possibly null reference.
// Infer(s, null).ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(s, null)").WithLocation(7, 9)
);
}

[Fact, WorkItem(43536, "https://github.com/dotnet/roslyn/issues/43536")]
public void MethodTypeInference_WithNull_TwoConversions()
{
var source = @"
class C
{
void M(string s)
Copy link
Member

Choose a reason for hiding this comment

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

string s [](start = 11, length = 8)

s is not used.

{
M2(null); // 1
}

void M2(Optional<object> o) => throw null!;
}

public readonly struct Optional<T>
{
public Optional(T value) => throw null!;
Copy link
Member

Choose a reason for hiding this comment

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

public Optional(T value) => throw null!; [](start = 4, length = 40)

Not used.

public static implicit operator Optional<T>(T value) => throw null!;
}
";
var comp = CreateNullableCompilation(source);
comp.VerifyDiagnostics(
// (6,12): warning CS8625: Cannot convert null literal to non-nullable reference type.
// M2(null); // 1
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(6, 12)
);
}

[Fact, WorkItem(43536, "https://github.com/dotnet/roslyn/issues/43536")]
public void MethodTypeInference_WithLambdaReturningDefaultAndNull()
{
var source = @"
class C
{
void M(string s1, string? s2)
{
Infer(s1, () => default); // 1
Infer(s1, () => null); // 2

Infer(s2, () => default);
Infer(s2, () => null);
}

T Infer<T>(T t1, System.Func<T> t2) => throw null!;
}";

var comp = CreateCompilationWithIndexAndRange(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (6,25): warning CS8603: Possible null reference return.
// Infer(s1, () => default); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "default").WithLocation(6, 25),
// (7,25): warning CS8603: Possible null reference return.
// Infer(s1, () => null); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReturn, "null").WithLocation(7, 25)
);
}

[Fact, WorkItem(43536, "https://github.com/dotnet/roslyn/issues/43536")]
public void MethodTypeInference_WithNullAndNestedNullability()
{
var source = @"
class C<U>
{
void M(C<string> c1, C<string?> c2)
{
Infer(c1, default).ToString();
Infer(c1, null).ToString();

Infer(c2, default).ToString(); // 1
Infer(c2, null).ToString(); // 2
}

T Infer<T>(C<T>? t1, C<T>? t2) => throw null!;
}";

var comp = CreateCompilationWithIndexAndRange(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (9,9): warning CS8602: Dereference of a possibly null reference.
// Infer(c2, default).ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(c2, default)").WithLocation(9, 9),
// (10,9): warning CS8602: Dereference of a possibly null reference.
// Infer(c2, null).ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(c2, null)").WithLocation(10, 9)
);
}

[Fact, WorkItem(43536, "https://github.com/dotnet/roslyn/issues/43536")]
public void MethodTypeInference_WithNullAndNestedNullability2()
{
var source = @"
class C<U>
{
void M(string c1, string? c2)
{
Infer(c1, default).ToString();
Infer(c1, null).ToString();

Infer(c2, default).ToString(); // 1
Infer(c2, null).ToString(); // 2
}

T Infer<T>(T t1, C<T>? t2) => throw null!;
}";

var comp = CreateCompilationWithIndexAndRange(source, options: WithNonNullTypesTrue());
comp.VerifyDiagnostics(
// (9,9): warning CS8602: Dereference of a possibly null reference.
// Infer(c2, default).ToString(); // 1
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(c2, default)").WithLocation(9, 9),
// (10,9): warning CS8602: Dereference of a possibly null reference.
// Infer(c2, null).ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "Infer(c2, null)").WithLocation(10, 9)
);
}

[Fact]
public void SpeakableInference_MethodTypeInference_WithTuple()
{
Expand Down Expand Up @@ -29801,8 +29942,8 @@ public class C
{
public void Main()
{
Copy(null, out string s); // warn
s/*T:string!*/.ToString(); // ok
Copy(null, out string s); // 1
s/*T:string?*/.ToString(); // 2
}
public static void Copy<T>(T key, out T value) => throw null!;
}
Expand All @@ -29811,9 +29952,12 @@ public void Main()
VerifyOutVar(c, "string!");
c.VerifyTypes();
c.VerifyDiagnostics(
// (6,14): warning CS8625: Cannot convert null literal to non-nullable reference type.
// Copy(null, out string s); // warn
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(6, 14)
// (6,24): warning CS8600: Converting null literal or possible null value to non-nullable type.
// Copy(null, out string s); // 1
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "string s").WithLocation(6, 24),
// (7,9): warning CS8602: Dereference of a possibly null reference.
// s/*T:string?*/.ToString(); // 2
Diagnostic(ErrorCode.WRN_NullReferenceReceiver, "s").WithLocation(7, 9)
);
}

Expand Down