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.

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

Track nullability of unconstrained type parameters #28956

Conversation

333fred
Copy link
Member

@333fred 333fred commented Jul 31, 2018

@jcouv @cston @dotnet/roslyn-compiler for review.

@333fred 333fred requested review from cston, jcouv and a team July 31, 2018 02:09
@333fred 333fred requested a review from a team as a code owner July 31, 2018 02:09
@@ -318,6 +318,22 @@ private void Populate(ref LocalState state, int start)
}
}

private TypeSymbolWithAnnotations GetAdjustedType(TypeSymbolWithAnnotations initialType)
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

static #Resolved

@@ -1379,7 +1387,7 @@ private TypeSymbolWithAnnotations InferResultNullability(BinaryOperatorKind oper
return methodOpt.ReturnType;
}
}
else if (!operatorKind.IsDynamic() && resultType.IsReferenceType == true)
else if (!operatorKind.IsDynamic() && (resultType.IsReferenceType || resultType.IsUnconstrainedTypeParameter()))
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

Can this be simplified to && !resultType.IsValueType)? Same comment elsewhere.

If not, please extract a IsReferenceTypeOrUnconstrainedTypeParameter() helper.
#Resolved

@@ -19865,11 +19839,7 @@ class CL0<T>
}
", NonNullTypesTrue, NonNullTypesAttributesDefinition }, parseOptions: TestOptions.Regular8);

c.VerifyDiagnostics(
// (22,22): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

// (22,22): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter. [](start = 16, length = 114)

This warning is correct. For instance if T is string, this is a warning for return (string)null;. #Resolved

@@ -11217,16 +11213,13 @@ void M<T>(T t)
}
public static void ThrowIfNull<T>([EnsuresNotNull] T s) => throw null;
}
" + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8);
", EnsuresNotNullAttributeDefinition, NonNullTypesAttributesDefinition, NonNullTypesTrue }, parseOptions: TestOptions.Regular8);

// PROTOTYPE(NullableReferenceTypes): We're not tracking unconstrained generic types
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

// PROTOTYPE(NullableReferenceTypes) [](start = 12, length = 36)

Can remove comment. #Closed

@cston
Copy link
Member

cston commented Jul 31, 2018

    // PROTOTYPE(NullableReferenceTypes): Should not warn for `default(T1)!.ToString()`.

Can remove comment. #Closed


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:29561 in f3a36b9. [](commit_id = f3a36b9, deletion_comment = False)

@@ -31488,12 +31442,6 @@ class C
// (6,39): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
// static async Task<string> F1() => null;
Diagnostic(ErrorCode.WRN_NullAsNonNullable, "null").WithLocation(6, 39),
// (8,43): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter.
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

// (8,43): warning CS8625: Cannot convert null literal [](start = 16, length = 54)

It looks like these three warnings were correct. #Closed

// (20,51): warning CS8600: Converting null literal or possible null value to non-nullable type.
// static U F18<T, U>(T t) where U : class, T => (U)t;
Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "(U)t").WithLocation(20, 51),
// prototype(NullableReferenceTypes): Should this be a warning, as there was already a nested warning about converting t to U?
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

prototype [](start = 19, length = 9)

PROTOTYPE #Pending

Copy link
Member

Choose a reason for hiding this comment

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

Still active.


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

@cston
Copy link
Member

cston commented Jul 31, 2018

    [Fact(Skip = "TODO")]

Please enable this test and the one below. #Resolved


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:36121 in f3a36b9. [](commit_id = f3a36b9, deletion_comment = False)

@333fred
Copy link
Member Author

333fred commented Jul 31, 2018

@cston addressed feedback. #Closed

private static TypeSymbolWithAnnotations GetAdjustedType(TypeSymbolWithAnnotations initialType)
{
// If the initial type was an unconstrained type parameter, we want to treat it as nullable, overriding
// the annotated state.
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

Why not return IsNullable == true from the original TypeSymbolWithAnnotations? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the original TWSA is from initial binding, and IsNullable does not return true.


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

Copy link
Member

Choose a reason for hiding this comment

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

Why not set treatUnconstrainedGenericsAsNullable: true in initial binding?


In reply to: 206722195 [](ancestors = 206722195,206707381)

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked with @cston offline. We'll defer doing this for a later PR, I'll add a prototype comment to track.


In reply to: 206750233 [](ancestors = 206750233,206722195,206707381)

@@ -35822,11 +35808,11 @@ public void UnconstrainedTypeParameter_MayBeNonNullable()
@"class C1<T1>
{
static object? NullableObject() => null;
static T1 F1() => default; // warn: return type T1 may be non-null
Copy link
Member

@cston cston Jul 31, 2018

Choose a reason for hiding this comment

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

// warn: return type T1 may be non-null [](start = 31, length = 39)

Please add these comments back. #Closed

@@ -37,7 +37,7 @@ internal abstract class TypeSymbolWithAnnotations : IFormattable
// PROTOTYPE(NullableReferenceTypes): consider removing this method and using Create below (which handles nullable value types).
internal static TypeSymbolWithAnnotations CreateUnannotated(INonNullTypesContext nonNullTypesContext, TypeSymbol typeSymbol, ImmutableArray<CustomModifier> customModifiers = default)
{
return Create(typeSymbol, nonNullTypesContext, isAnnotated: false, customModifiers.NullToEmpty());
return Create(typeSymbol, nonNullTypesContext, isAnnotated: false, treatUnconstrainedGenericsAsNullable: false, customModifiers.NullToEmpty());
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

treatUnconstrainedGenericsAsNullable [](start = 79, length = 36)

Perhaps treatUnconstrainedTypeParameterAsNullable, since this applies to explicit type parameter instances, not nested type parameters. #Resolved

@333fred
Copy link
Member Author

333fred commented Aug 1, 2018

@cston addressed feedback. #Closed

}

// PROTOTYPE(NullableReferenceTypes): [Obsolete("Use explicit NonNullTypes context")]
public static TypeSymbolWithAnnotations Create(TypeSymbol typeSymbol, bool? isNullableIfReferenceType)
public static TypeSymbolWithAnnotations Create(TypeSymbol typeSymbol, bool? isNullableIfReferenceType, bool treatUnconstrainedGenericsAsNullable = false)
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

treatUnconstrainedGenericsAsNullable [](start = 116, length = 36)

treatUnconstrainedTypeParameterAsNullable #Resolved

@@ -35826,7 +35812,7 @@ public void UnconstrainedTypeParameter_MayBeNonNullable()
static T1 F2() => default(T1); // warn: return type T1 may be non-null
static void F4()
{
T1 t1 = (T1)NullableObject(); // warn: T1 may be non-null
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

// warn: T1 may be non-null [](start = 38, length = 27)

Should be a warning in each case. #Closed

@@ -36173,60 +36156,74 @@ public void UnconstrainedTypeParameter_Return_03()
static U F20<T, U>(T t) where U : T, new() => (U)t;
}";
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

As we discussed offline, please add additional test cases when T and U are independent. Something like:

class C
{
    static U F1<T, U>(T t) => (U)(object)t;
    static U F2<T, U>(T t) where U : class => (U)(object)t;
    static U F3<T, U>(T t) where U : struct => (U)(object)t;
    static U F4<T, U>(T t) where T : class => (U)(object)t;
    static U F5<T, U>(T t) where T : struct => (U)(object)t;
}
``` #Resolved

@@ -35847,7 +35833,7 @@ class C3<T3> where T3 : new()
static T3 F3() => new T3();
static void F4()
{
T3 t = (T3)NullableObject(); // warn: T3 may be non-null
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

// warn: T3 may be non-null [](start = 37, length = 27)

Should be a warning. Please add the comment back. #Closed

@@ -35857,7 +35843,7 @@ class C4<T4> where T4 : I
static T4 F2() => default(T4); // warn: return type T4 may be non-null
static void F4()
{
T4 t4 = (T4)NullableObject(); // warn: T4 may be non-null
Copy link
Member

@cston cston Aug 1, 2018

Choose a reason for hiding this comment

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

// warn: T4 may be non-null [](start = 38, length = 27)

Should be a warning. Please add the comment back. #Closed

// PROTOTYPE(NullableReferenceTypes): Consider setting treatUnconstrainedTypeParameterAsNullable during initial binding, and removing this method.
private static TypeSymbolWithAnnotations GetAdjustedType(TypeSymbolWithAnnotations initialType)
{
// If the initial type was an unconstrained type parameter, we want to treat it as nullable, overriding
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

// [](start = 12, length = 2)

Perhaps make this an xml doc instead.
Also, consider renaming to "FixUnconstrainedTypeParameters", as it gives some indication of purpose, whereas "adjusted type" provides no clue. #Closed

@@ -975,7 +984,7 @@ public override BoundNode VisitLocalDeclaration(BoundLocalDeclaration node)
(initializer, conversion) = RemoveConversion(initializer, includeExplicitConversions: false);

TypeSymbolWithAnnotations valueType = VisitRvalueWithResult(initializer);
TypeSymbolWithAnnotations type = local.Type;
TypeSymbolWithAnnotations type = GetAdjustedType(local.Type);
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

GetAdjustedType [](start = 45, length = 15)

I see that parameters and locals are covered. Can you also check that out variables and other variables (using, foreach, deconstruction, patterns, ...) behave correctly?
The let in LINQ also introduces variables, but LINQ is known to be unhandled at the moment. #Resolved

Copy link
Member

@jcouv jcouv Aug 5, 2018

Choose a reason for hiding this comment

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

Hum, ignore foreach variables for now. They have bigger problems. #Resolved

@@ -758,7 +775,7 @@ private void EnterParameters()
{
var parameter = methodParameters[i];
var parameterType = signatureParameters[i].Type;
_variableTypes[parameter] = parameterType;
_variableTypes[parameter] = GetAdjustedType(parameterType);
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

GetAdjustedType [](start = 44, length = 15)

Consider moving this into EnterParameter #Closed

@@ -918,6 +926,7 @@ private TypeSymbolWithAnnotations GetReturnType()
{
var method = (MethodSymbol)_member;
var returnType = (_useMethodSignatureReturnType ? _methodSignatureOpt : method).ReturnType;
returnType = GetAdjustedType(returnType);
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

GetAdjustedType [](start = 25, length = 15)

This change doesn't seem obvious to me. Why do we need to adjust here? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to support T Foo<T>(T t) => t. We treat t as a T?, and there should be no warnings in this code, so we want to make sure that we're only warning on the places where t is being dereferenced. The scenario of T Foo<T>() => default(T) is covered by warning on all conversions of default to T.


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

@jcouv
Copy link
Member

jcouv commented Aug 3, 2018

    public override void VisitForEachIterationVariables(BoundForEachStatement node)

📝 foreach variables probably need to be handled #Closed


Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3534 in 5117748. [](commit_id = 5117748, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Aug 3, 2018

    private static bool IsNullable(TypeSymbolWithAnnotations typeOpt)

Not blocking this PR: consider making IsNullable and IsNonNullable local functions (or inlining them) in VisitReturnStatementNoAdjust since they only appear to be used there. #Closed


Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:936 in 5117748. [](commit_id = 5117748, deletion_comment = False)

bool reportedNullable = false;
if (returnTypeIsNonNullable || returnTypeIsUnconstrainedTypeParameter)
if (returnTypeIsNonNullable &&
!ReportNullAsNonNullableReferenceIfNecessary(node.ExpressionOpt) &&
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

ReportNullAsNonNullableReferenceIfNecessary [](start = 17, length = 43)

We have different handling for null literal (which will produce WRN_NullAsNonNullable) than for other nullable expressions (which produce WRN_NullReferenceReturn below).
@cston Do you still think that is valuable, or could we remove the call to ReportNullAsNonNullableReferenceIfNecessary here?

Never mind, we probably need this handling because of null being typeless? #Closed

@@ -1066,7 +1075,7 @@ private void VisitObjectOrDynamicObjectCreation(BoundExpression node, BoundExpre
if ((object)type != null)
{
bool isTrackableStructType = EmptyStructTypeCache.IsTrackableStructType(type);
if (type.IsReferenceType || isTrackableStructType)
if (!type.IsValueType || isTrackableStructType)
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

IsValueType [](start = 26, length = 11)

Not directly related to this PR: I see that we use .IsReferenceType in a number of remaining places in NullableWalker (usually in conjunction with IsUnconstrainedTypeParameter). It may be good to take a peek to see if they might need to be revised too. #Resolved

@@ -1881,7 +1890,7 @@ public override BoundNode VisitCall(BoundCall node)

//if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability?
{
_resultType = method.ReturnType;
_resultType = GetAdjustedType(method.ReturnType);
Copy link
Member

@jcouv jcouv Aug 3, 2018

Choose a reason for hiding this comment

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

GetAdjustedType [](start = 30, length = 15)

I'm not sure about this. Just to confirm, here's a scenario:

T Copy<T>(T t) ...
void M<U>(U u)
{
    if (u == null) throw;
    var x = Copy(u);
    x.ToString(); // do we expect a warning here?
}
``` #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I would not expect a warning here: we've done the null check on u, so it should be U!. It would be a different question if the T was constrained to be class?, but with unconstrained I think this should be fine.


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

@jcouv
Copy link
Member

jcouv commented Aug 3, 2018

    private TypeSymbolWithAnnotations GetDeclaredLocalResult(LocalSymbol local)

Just to confirm: Why do we adjust in GetDeclaredParameterResult (a few lines below) but not in GetDeclaredLocalResult? #Resolved


Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3444 in 5117748. [](commit_id = 5117748, deletion_comment = False)

@jcouv
Copy link
Member

jcouv commented Aug 3, 2018

                (operandType.IsReferenceType || operandType.IsUnconstrainedTypeParameter());

📝 here's an example #Closed


Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1587 in 5117748. [](commit_id = 5117748, deletion_comment = False)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 6).

333fred added 2 commits August 6, 2018 10:44
… into unconstrained-type-parameter-nullability

* dotnet/features/NullableReferenceTypes:
  Include nullability in CheckConstraints (dotnet#28959)
  Use struct for TypeSymbolWithAnnotations (dotnet#28943)
  Publish Linux Test Results
  Force retest
  Migrate Linux tests to VSTS
  Port determinism fixes
  Make sure TLS 1.2 is used to fetch from https://dot.net
  Move to language version 8
  Make mutex creation more robust
  Disable icacls use on Helix
  Disable leak detection tests on x64
  VSTS YAML file
return method.IsGenericTaskReturningAsync(compilation) ?
((NamedTypeSymbol)returnType.TypeSymbol).TypeArgumentsNoUseSiteDiagnostics.Single() :
returnType;
}

// Consider making these local functions.
Copy link
Member

@jcouv jcouv Aug 6, 2018

Choose a reason for hiding this comment

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

Consider [](start = 11, length = 8)

nit: PROTOTYPE instead of comment, or remove #Resolved

@@ -318,6 +320,27 @@ private void Populate(ref LocalState state, int start)
}
}

// PROTOTYPE(NullableReferenceTypes): Consider setting treatUnconstrainedTypeParameterAsNullable during initial binding, and removing this method.
/// <summary>
/// Adjust the given type, treating unconstrained type parameters as nullable if in the appropriate context. This overrides the current state
Copy link
Member

@cston cston Aug 6, 2018

Choose a reason for hiding this comment

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

unconstrained type parameters [](start = 44, length = 29)

This comment, and the method name, imply all unconstrained type parameters in initialType are adjusted, but we're only adjusting initialType if it is simply an unconstrained type parameter. Consider using singular here and in method name. #Resolved

}

[Fact]
public void UnconstrainedTypeParameter_OutVariable()
Copy link
Member

@jcouv jcouv Aug 6, 2018

Choose a reason for hiding this comment

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

UnconstrainedTypeParameter_OutVariable [](start = 20, length = 38)

Sorry I wasn't clear. I think the interesting case for out var is the call-site.

F3(t, out T t2);
// check null-state of t2 (do we reset to nullable, or do we use the state of variable `t`?)

We may already have such test. #Resolved

// parameters as annotated during initial binding without causing cycles.

// Initial binding leaves unconstrained type parameters unannotated, NullableWalker will
// set treatUnconstrainedTypeParametersAsNullable where appropriate
Copy link
Member

@jcouv jcouv Aug 6, 2018

Choose a reason for hiding this comment

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

treat [](start = 23, length = 5)

nit: missing space after "treat" Never mind, I'm slow. #Closed

@@ -3464,13 +3486,14 @@ private TypeSymbolWithAnnotations GetDeclaredLocalResult(LocalSymbol local)
return _variableTypes.TryGetValue(local, out TypeSymbolWithAnnotations type) ?
type :
local.Type;
//UnconstrainedTypeParametersAsNullable(local.Type);
Copy link
Member

@cston cston Aug 6, 2018

Choose a reason for hiding this comment

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

//UnconstrainedTypeParametersAsNullable(local.Type); [](start = 16, length = 52)

Does this need a comment? Or can it be removed? #Resolved

// parameters as annotated during initial binding without causing cycles.

// Initial binding leaves unconstrained type parameters unannotated, NullableWalker will
// set treatUnconstrainedTypeParametersAsNullable where appropriate
Copy link
Member

@cston cston Aug 6, 2018

Choose a reason for hiding this comment

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

TypeParameters [](start = 41, length = 14)

TypeParameter (singular) here and below. #Resolved

@jcouv
Copy link
Member

jcouv commented Aug 6, 2018

    public void VarLocal_FromGenericMethod_WithUnnecessarySuppression()

Let's add a test like this (T Copy<T>(T t)) with an argument that is still generic (untested, and tested for null).
That should illustrate the question in VisitCall.
Then we can do the same with a void Copy<T>(T t1, out T t2) and verify same behavior. #Resolved


Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:8555 in 46c71ef. [](commit_id = 46c71ef, deletion_comment = False)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 8).

@@ -211,6 +211,14 @@ var y = F1(maybeNullString); // List<string?> or List<string~> ?
var z = F2(obliviousString); // List<string~>! or List<string!>! ?
var w = F3(obliviousString); // List<string~>! or List<string?>! ?
```
A warning is reported for accessing variables of type T, where T is an unconstrained type parameter.
Copy link
Member

Choose a reason for hiding this comment

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

accessing [](start = 26, length = 9)

nit: dereferencing.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks! (iteration 10)

@333fred 333fred merged commit 109bb19 into dotnet:features/NullableReferenceTypes Aug 7, 2018
@333fred 333fred deleted the unconstrained-type-parameter-nullability branch August 7, 2018 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants