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

Use speakable annotations in method type re-inference #31813

Merged
merged 25 commits into from
Dec 31, 2018

Conversation

jcouv
Copy link
Member

@jcouv jcouv commented Dec 15, 2018

Fixes #31557 (method, array and lambda inference should produce speakable types)

Fixes #30056 (renaming IsUnconstrainedTypeParameter)

Fixes #31718 (array inference with user-defined conversion)

Relates to #30480 (missing warning in lambda inference)

Relates to #32006 (lambda returning some typeless tuples). I will fix that in follow-up PR.

Relates to #30964 (ref-returning lambdas)

@@ -62003,6 +62004,137 @@ static void Test()

private readonly static NullableAnnotation[] s_AllNullableAnnotations = (NullableAnnotation[])Enum.GetValues(typeof(NullableAnnotation));

[Fact]
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

This is not visible in CodeFlow. #Closed

{ NullableAnnotation.Annotated, NullableAnnotation.Nullable, NullableAnnotation.Unknown, NullableAnnotation.NotAnnotated, NullableAnnotation.NotAnnotated },
};

AssertEqual(expected, getResult, inputs.Length);
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

I don't see this test as doing anything other than verifying that the method does what it does. How does this test give us any kind of confidence that anything is "correct"? #Resolved

Copy link
Member Author

@jcouv jcouv Dec 16, 2018

Choose a reason for hiding this comment

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

My goal here is to make the behavior of those methods visible in a table representation. This will help analyze the current behavior and as we make further changes. #Resolved


Func<int, int, NullableAnnotation> getResult =
(i, j) => NullableAnnotationExtensions.JoinForFlowAnalysisBranches<string>(
inputs[i].annotation, inputs[j].annotation, type: null, isPossiblyNullableReferenceTypeTypeParameter: _ => inputs[i].isPNTP);
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

The relation being tested here is not symmetric, as it always uses the flag from the left operand. #Resolved

@@ -1381,7 +1384,7 @@ private ArrayTypeSymbol VisitArrayInitializer(BoundArrayCreation node)
}
else
{
elementType = TypeSymbolWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(bestType, resultBuilder));
elementType = TypeSymbolWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(bestType, speakableResultBuilder));
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

I think the conversion to a speakable annotation is supposed to occur at the end of type inference, not on the inputs. #Resolved

Copy link
Member Author

@jcouv jcouv Dec 17, 2018

Choose a reason for hiding this comment

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

We had discussed both options in LDM last time. The benefit of doing it on inputs is it might help simplify some methods (that now need to handle fewer null-states), so I went with that.
I don't think there is much difference, so the question is how would we decide? #Resolved

@@ -1047,7 +1047,7 @@ private TypeSymbolWithAnnotations GetReturnType()

private static bool IsUnconstrainedTypeParameter(TypeSymbol typeOpt)
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

Maybe rename this too #Closed

@@ -2809,6 +2809,8 @@ private UnboundLambda GetUnboundLambda(BoundLambda expr, VariableState variableS
private MethodSymbol InferMethodTypeArguments(BoundCall node, MethodSymbol method, ImmutableArray<BoundExpression> arguments)
{
Debug.Assert(method.IsGenericMethod);
Debug.Assert(arguments.All(a => a.GetTypeAndNullability().NullableAnnotation.IsSpeakable()));
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

If all arguments are expressions with speakable annotations, that suggests that only speakable annotations are used at all (declared annotations are always speakable, and these are the flow states. Those are the only things annotations are used for). So why do we have the other states? #Resolved

@@ -2888,10 +2893,12 @@ BoundExpression getArgumentForMethodTypeInference(BoundExpression argument, Type
}
if (argument is BoundLocal local && local.DeclarationKind == BoundLocalDeclarationKind.WithInferredType)
{
// TODO: the condition seems too lose. This might be a regular local, not an out var... Need to test
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

You can add local.IsDeclaration. #WontFix

// 'out var' doesn't contribute to inference
return new BoundExpressionWithNullability(argument.Syntax, argument, NullableAnnotation.Unknown, type: null);
}
return new BoundExpressionWithNullability(argument.Syntax, argument, argumentType.NullableAnnotation, argumentType.TypeSymbol);
return new BoundExpressionWithNullability(argument.Syntax, argument, argumentType.GetSpeakableNullableAnnotation(), argumentType.TypeSymbol);
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

How could its declared annotation not be speakable? #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.

argumentType comes out of argumentResults which represents null-state of the arguments.


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

@@ -97,7 +97,7 @@ private void ReportUninitializedNonNullableReferenceTypeFields()
{
continue;
}
if (!fieldType.NullableAnnotation.IsAnyNotNullable() && !fieldType.TypeSymbol.IsUnconstrainedTypeParameter())
if (!fieldType.NullableAnnotation.IsAnyNotNullable() && !fieldType.TypeSymbol.IsTypeParameterDisallowingAnnotation())
Copy link
Member

@gafter gafter Dec 16, 2018

Choose a reason for hiding this comment

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

How could a field's declared nullability not be speakable? #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

How could a field's declared nullability not be speakable?

In an error scenario, a ? can be placed on a ``IsTypeParameterDisallowingAnnotation```


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

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

Looks OK, but regarding the new tests of the Join and Meet functions... I don't know how to have any confidence that they are correct or checking desired properties.

@jcouv jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 18, 2018
@jcouv jcouv removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 18, 2018
@cston
Copy link
Member

cston commented Dec 18, 2018

    //                                    its implementation is specialized for this feature.

Can the comment be removed and the issue resolved? #Resolved


Refers to: src/Compilers/CSharp/Portable/Symbols/TypeSymbolExtensions.cs:46 in 2370dee. [](commit_id = 2370dee5327818160a124840b886dc2066d6f7fa, deletion_comment = False)

for (int i = 0; i < n; i++)
{
(BoundExpression element, Conversion conversion) = RemoveConversion(elementBuilder[i], includeExplicitConversions: false);
elementBuilder[i] = element;
conversionBuilder.Add(conversion);
resultBuilder.Add(VisitRvalueWithResult(element));
var value = VisitRvalueWithResult(element);
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

value [](start = 20, length = 5)

Consider naming resultType instead since this is a type rather than a value. #Closed

@@ -2809,6 +2813,8 @@ private UnboundLambda GetUnboundLambda(BoundLambda expr, VariableState variableS
private MethodSymbol InferMethodTypeArguments(BoundCall node, MethodSymbol method, ImmutableArray<BoundExpression> arguments)
{
Debug.Assert(method.IsGenericMethod);
Debug.Assert(arguments.All(a => a.GetTypeAndNullability().NullableAnnotation.IsSpeakable()));
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

GetTypeAndNullability [](start = 46, length = 21)

GetTypeAndNullability() should be removed. Please use GetNullableAnnotation() instead. #Resolved

Copy link
Member Author

@jcouv jcouv Dec 19, 2018

Choose a reason for hiding this comment

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

This caught an issue (details below). Thanks

We call MethodTypeInferrer.Infer with GetNullableAnnotation(BoundExpression) as a delegate. But GetNullableAnnotation could return Nullable/NotNullable on some typeless expressions (such as null).
GetArgumentsForMethodTypeInference cannot do anything to replace such typeless expressions with placeholders (BoundExpressionWithNullability) so it must let unspeakable types through.
I've adjusted GetNullableAnnotation to compensate (with no apparent negative side-effects), but I'm not very confident. We can discuss tomorrow. #Resolved

case NullableAnnotation.NotNullable:
// Example of unspeakable types:
// - a "tight T", which is an unconstrained T which was null-tested already
// - a "tight int?"
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

Minor point: Consider avoiding introducing a new term. Perhaps just "// An unconstrained type parameter or nullable value type that is known to be not null". #Resolved

@@ -523,7 +566,7 @@ private static NullableAnnotation MergeNullableAnnotation(TypeSymbol type, Nulla
/// <summary>
/// The list of custom modifiers, if any, associated with the <see cref="TypeSymbol"/>.
/// </summary>
public ImmutableArray<CustomModifier> CustomModifiers => _extensions.CustomModifiers;
public ImmutableArray<CustomModifier> CustomModifiers => _extensions is null ? default : _extensions.CustomModifiers;
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

_extensions is null [](start = 65, length = 19)

Are there callers of type.CustomModifiers that are not checking type.IsNull? #Resolved

Copy link
Member Author

@jcouv jcouv Dec 19, 2018

Choose a reason for hiding this comment

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

The method I added (AsSpeakable) would run into this. #Resolved

Copy link
Member

@cston cston Dec 19, 2018

Choose a reason for hiding this comment

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

Consider having AsSpeakable(), or the callers of AsSpeakable(), check IsNull instead, to avoid having each of the members of TypeSymbolWithAnnotations having to handle default values. #Resolved

Copy link
Member Author

@jcouv jcouv Dec 19, 2018

Choose a reason for hiding this comment

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

I don't mind making that change, but I'm curious what is the benefit.
When we create a TSWA, we can pass default custom modifiers. Why throw instead of returning default back when asked for the customer modifiers? #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

CustomModifiers is never default for an initialized TypeSymbolWithAnnotations.

We could have these properties (and other TypeSymbolWithAnnotations members) handle uninitialized (IsNull) instances but that will add some complexity to these members and might hide bugs where the caller should have handled IsNull explicitly.


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

var source =
@"class Program
{
SIGNATURE
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

SIGNATURE [](start = 4, length = 9)

Consider writing two tests (with no text substitution), or two methods in this same test. #Resolved

var source =
@"class Program
{
SIGNATURE
Copy link
Member

@cston cston Dec 18, 2018

Choose a reason for hiding this comment

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

SIGNATURE [](start = 4, length = 9)

Consider writing two tests. #Closed

Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents: I like how theories can keep our test assets more compact.


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

Copy link
Member Author

@jcouv jcouv Dec 18, 2018

Choose a reason for hiding this comment

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

Some context: I expected there would be more than two variants here ;-) #Closed

@jcouv
Copy link
Member Author

jcouv commented Dec 19, 2018

Can the comment be removed and the issue resolved?

Indeed. Thanks #Resolved

@@ -334,6 +341,42 @@ private TypeSymbolWithAnnotations(TypeSymbol defaultType, NullableAnnotation nul
_extensions = extensions;
}

public TypeSymbolWithAnnotations AsSpeakable()
{
return Create(this.TypeSymbol, this.GetSpeakableNullableAnnotation(), this.IsNull ? default : this.CustomModifiers);
Copy link
Member

@cston cston Dec 19, 2018

Choose a reason for hiding this comment

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

Create(this.TypeSymbol, this.GetSpeakableNullableAnnotation(), this.IsNull ? default : this.CustomModifiers) [](start = 19, length = 108)

IsNull ? default : Create(...) #Resolved

AssertEqual(expected, getResult, inputs.Length);
}

void AssertEqual(NullableAnnotation[,] expected, Func<int, int, NullableAnnotation> getResult, int size)
Copy link
Member

@cston cston Dec 19, 2018

Choose a reason for hiding this comment

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

void [](start = 8, length = 4)

private static #Resolved

{
var (returnExpr, resultType) = returns[i];
expressions.Add(returnExpr);
resultTypes.Add(returns[i].resultType);
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 28, 2018

Choose a reason for hiding this comment

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

returns[i].resultType [](start = 32, length = 21)

resultType? #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 28, 2018

                // there was a nullability mismatch, and `hadNullabilityMismatch` should be available

Has this been addressed? #Closed


Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:185 in e604396. [](commit_id = e604396, deletion_comment = True)

}

// Set top-level nullability on inferred type
inferredType = TypeSymbolWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(resultTypes));
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 28, 2018

Choose a reason for hiding this comment

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

resultTypes [](start = 113, length = 11)

It looks like we may fail to adjust the type in some items due to the conditional logic in the loop above, but GetNullableAnnotation is supposed to be called only for the same types. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 28, 2018

            // (15,27): warning CS8603: Possible null reference return.

Is this warning expected? Should we infer return type of the lambda as nullable instead? #Closed


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

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Dec 28, 2018

Done with review pass (iteration 20) #Closed

@jcouv
Copy link
Member Author

jcouv commented Dec 28, 2018

                // there was a nullability mismatch, and `hadNullabilityMismatch` should be available

The issue will remain open. I'm not convinced the design described in the comment will be the correct solution. So seems fine to remove.
I'm fine to leave if you prefer.


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


Refers to: src/Compilers/CSharp/Portable/BoundTree/UnboundLambda.cs:185 in e604396. [](commit_id = e604396, deletion_comment = True)

@jcouv
Copy link
Member Author

jcouv commented Dec 28, 2018

            // (15,27): warning CS8603: Possible null reference return.

Good catch. The issue was that BestTypeForLambdReturns was classifying conversion considering nullability, so x would not convert to C<string?> and the top-level nullability of that conversion would not come into play.


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


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

{
return NullableAnnotation.Unknown;
return _getTypeWithAnnotationOpt.Invoke(expr);
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 28, 2018

Choose a reason for hiding this comment

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

_getTypeWithAnnotationOpt.Invoke(expr); [](start = 23, length = 39)

Minor thing: Since conditional access is no longer needed, we can drop the .Invoke #Resolved

{
var annotation = GetNullableAnnotation(argument);
ExactOrBoundsInference(kind, TypeSymbolWithAnnotations.Create(source, annotation), target, ref useSiteDiagnostics);
ExactOrBoundsInference(kind, GetTypeWithAnnotations(argument), target, ref useSiteDiagnostics);
Copy link
Member Author

@jcouv jcouv Dec 28, 2018

Choose a reason for hiding this comment

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

ExactOrBoundsInference [](start = 20, length = 22)

📝 inside ExactOrBoundsInference tuples may get exploded into parts (see LowerBoundTupleInference). So we're applying AsSpeakable in-depth here.
#Resolved

@@ -633,7 +633,23 @@ internal bool NeedsNullableAttribute()

internal abstract bool ApplyNullableTransforms(byte defaultTransformFlag, ImmutableArray<byte> transforms, ref int position, out TypeSymbol result);

internal abstract TypeSymbol SetUnknownNullabilityForReferenceTypes();
internal abstract TypeSymbol SetNullabilityForReferenceTypes(Func<TypeSymbolWithAnnotations, TypeSymbolWithAnnotations> predicate);
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 29, 2018

Choose a reason for hiding this comment

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

predicate [](start = 128, length = 9)

Minor: This isn't really a predicate #Resolved

Copy link
Member Author

@jcouv jcouv Dec 29, 2018

Choose a reason for hiding this comment

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

Perhaps transform then? #Resolved

{
return SetNullabilityForReferenceTypes(setUnknownNullability);

TypeSymbolWithAnnotations setUnknownNullability(TypeSymbolWithAnnotations type)
Copy link
Contributor

@AlekseyTs AlekseyTs Dec 29, 2018

Choose a reason for hiding this comment

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

TypeSymbolWithAnnotations setUnknownNullability(TypeSymbolWithAnnotations type) [](start = 12, length = 79)

Is delegate for the local function going to be cached? If not, consider using a lambda instead. #Resolved

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 21)

@jcouv
Copy link
Member Author

jcouv commented Dec 29, 2018

@jaredpar for approval for preview2. Thanks

@jaredpar
Copy link
Member

approved to merge

@jcouv
Copy link
Member Author

jcouv commented Dec 29, 2018

FYI, I found/fixed a bug with tuples of unspeakable types.
It looks like bootstrap build is running into some assertions. I'm investigating, but having trouble to repro locally.

@jcouv jcouv force-pushed the speakable-types branch 3 times, most recently from 98dfef0 to 6b11302 Compare December 29, 2018 18:14
}

// True if the type is nullable but not an unconstrained type parameter.
private readonly static Func<TypeSymbolWithAnnotations, bool> s_isNullableOnly =
(type) => type.NullableAnnotation.IsAnyNullable() && !type.TypeSymbol.IsTypeParameterDisallowingAnnotation();
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to use a delegate rather than a static method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry about that. I was fixing a couple of places that allocate delegates and got trigger happy on this one which does not have a delegate.
Reverted

@jcouv jcouv merged commit 0a81853 into dotnet:master Dec 31, 2018
@jcouv jcouv deleted the speakable-types branch December 31, 2018 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants