Track nullability of unconstrained type parameters#28956
Conversation
| } | ||
| } | ||
|
|
||
| private TypeSymbolWithAnnotations GetAdjustedType(TypeSymbolWithAnnotations initialType) |
| } | ||
| } | ||
| else if (!operatorKind.IsDynamic() && resultType.IsReferenceType == true) | ||
| else if (!operatorKind.IsDynamic() && (resultType.IsReferenceType || resultType.IsUnconstrainedTypeParameter())) |
There was a problem hiding this comment.
Can this be simplified to && !resultType.IsValueType)? Same comment elsewhere.
If not, please extract a IsReferenceTypeOrUnconstrainedTypeParameter() helper.
#Resolved
| ", NonNullTypesTrue, NonNullTypesAttributesDefinition }, parseOptions: TestOptions.Regular8); | ||
|
|
||
| c.VerifyDiagnostics( | ||
| // (22,22): warning CS8625: Cannot convert null literal to non-nullable reference or unconstrained type parameter. |
There was a problem hiding this comment.
// (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
| " + EnsuresNotNullAttributeDefinition, parseOptions: TestOptions.Regular8); | ||
| ", EnsuresNotNullAttributeDefinition, NonNullTypesAttributesDefinition, NonNullTypesTrue }, parseOptions: TestOptions.Regular8); | ||
|
|
||
| // PROTOTYPE(NullableReferenceTypes): We're not tracking unconstrained generic types |
There was a problem hiding this comment.
// PROTOTYPE(NullableReferenceTypes) [](start = 12, length = 36)
Can remove comment. #Closed
| // (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. |
There was a problem hiding this comment.
// (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? |
There was a problem hiding this comment.
prototype [](start = 19, length = 9)
PROTOTYPE #Pending
There was a problem hiding this comment.
|
@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. |
There was a problem hiding this comment.
Why not return IsNullable == true from the original TypeSymbolWithAnnotations? #Resolved
There was a problem hiding this comment.
Because the original TWSA is from initial binding, and IsNullable does not return true.
In reply to: 206707381 [](ancestors = 206707381)
There was a problem hiding this comment.
Why not set treatUnconstrainedGenericsAsNullable: true in initial binding?
In reply to: 206722195 [](ancestors = 206722195,206707381)
| @"class C1<T1> | ||
| { | ||
| static object? NullableObject() => null; | ||
| static T1 F1() => default; // warn: return type T1 may be non-null |
There was a problem hiding this comment.
// warn: return type T1 may be non-null [](start = 31, length = 39)
Please add these comments back. #Closed
| 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()); |
There was a problem hiding this comment.
treatUnconstrainedGenericsAsNullable [](start = 79, length = 36)
Perhaps treatUnconstrainedTypeParameterAsNullable, since this applies to explicit type parameter instances, not nested type parameters. #Resolved
|
@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) |
There was a problem hiding this comment.
treatUnconstrainedGenericsAsNullable [](start = 116, length = 36)
treatUnconstrainedTypeParameterAsNullable #Resolved
| 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 |
There was a problem hiding this comment.
// 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; | |||
| }"; | |||
There was a problem hiding this comment.
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
| static T3 F3() => new T3(); | ||
| static void F4() | ||
| { | ||
| T3 t = (T3)NullableObject(); // warn: T3 may be non-null |
There was a problem hiding this comment.
// warn: T3 may be non-null [](start = 37, length = 27)
Should be a warning. Please add the comment back. #Closed
| 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 |
There was a problem hiding this comment.
// 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 |
There was a problem hiding this comment.
// [](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
|
|
||
| TypeSymbolWithAnnotations valueType = VisitRvalueWithResult(initializer); | ||
| TypeSymbolWithAnnotations type = local.Type; | ||
| TypeSymbolWithAnnotations type = GetAdjustedType(local.Type); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Hum, ignore foreach variables for now. They have bigger problems. #Resolved
| var parameter = methodParameters[i]; | ||
| var parameterType = signatureParameters[i].Type; | ||
| _variableTypes[parameter] = parameterType; | ||
| _variableTypes[parameter] = GetAdjustedType(parameterType); |
There was a problem hiding this comment.
GetAdjustedType [](start = 44, length = 15)
Consider moving this into EnterParameter #Closed
| { | ||
| var method = (MethodSymbol)_member; | ||
| var returnType = (_useMethodSignatureReturnType ? _methodSignatureOpt : method).ReturnType; | ||
| returnType = GetAdjustedType(returnType); |
There was a problem hiding this comment.
GetAdjustedType [](start = 25, length = 15)
This change doesn't seem obvious to me. Why do we need to adjust here? #Closed
There was a problem hiding this comment.
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)
Not blocking this PR: consider making 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) && |
There was a problem hiding this comment.
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
| { | ||
| bool isTrackableStructType = EmptyStructTypeCache.IsTrackableStructType(type); | ||
| if (type.IsReferenceType || isTrackableStructType) | ||
| if (!type.IsValueType || isTrackableStructType) |
There was a problem hiding this comment.
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
| //if (this.State.Reachable) // PROTOTYPE(NullableReferenceTypes): Consider reachability? | ||
| { | ||
| _resultType = method.ReturnType; | ||
| _resultType = GetAdjustedType(method.ReturnType); |
There was a problem hiding this comment.
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?
}
``` #ResolvedThere was a problem hiding this comment.
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)
Just to confirm: Why do we adjust in Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:3444 in 5117748. [](commit_id = 5117748, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 6).
… 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
| returnType; | ||
| } | ||
|
|
||
| // Consider making these local functions. |
There was a problem hiding this comment.
Consider [](start = 11, length = 8)
nit: PROTOTYPE instead of comment, or remove #Resolved
|
|
||
| // 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 |
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
treat [](start = 23, length = 5)
nit: missing space after "treat" Never mind, I'm slow. #Closed
| return _variableTypes.TryGetValue(local, out TypeSymbolWithAnnotations type) ? | ||
| type : | ||
| local.Type; | ||
| //UnconstrainedTypeParametersAsNullable(local.Type); |
There was a problem hiding this comment.
//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 |
There was a problem hiding this comment.
TypeParameters [](start = 41, length = 14)
TypeParameter (singular) here and below. #Resolved
Let's add a test like this ( Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:8555 in 46c71ef. [](commit_id = 46c71ef, deletion_comment = False) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 8).
| 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. |
There was a problem hiding this comment.
accessing [](start = 26, length = 9)
nit: dereferencing.
@jcouv @cston @dotnet/roslyn-compiler for review.