Use speakable annotations in method type re-inference by jcouv · Pull Request #31813 · dotnet/roslyn

jcouv · 2018-12-15T04:42:29Z

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)

gafter · 2018-12-16T20:12:03Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

This is not visible in CodeFlow. #Closed

gafter · 2018-12-16T20:13:06Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

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

gafter · 2018-12-16T20:13:47Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

gafter · 2018-12-16T20:16:43Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

                else
                {
-                    elementType = TypeSymbolWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(bestType, resultBuilder));
+                    elementType = TypeSymbolWithAnnotations.Create(bestType, BestTypeInferrer.GetNullableAnnotation(bestType, speakableResultBuilder));


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

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

gafter · 2018-12-16T20:17:45Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

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

        private static bool IsUnconstrainedTypeParameter(TypeSymbol typeOpt)


Maybe rename this too #Closed

gafter · 2018-12-16T20:20:17Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

        private MethodSymbol InferMethodTypeArguments(BoundCall node, MethodSymbol method, ImmutableArray<BoundExpression> arguments)
        {
            Debug.Assert(method.IsGenericMethod);
+            Debug.Assert(arguments.All(a => a.GetTypeAndNullability().NullableAnnotation.IsSpeakable()));


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

gafter · 2018-12-16T20:21:24Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

                }
                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


You can add local.IsDeclaration. #WontFix

gafter · 2018-12-16T20:22:04Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

                    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);


How could its declared annotation not be speakable? #Resolved

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

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

gafter · 2018-12-16T20:22:23Z

src/Compilers/CSharp/Portable/FlowAnalysis/UnassignedFieldsWalker.cs

                    continue;
                }
-                if (!fieldType.NullableAnnotation.IsAnyNotNullable() && !fieldType.TypeSymbol.IsUnconstrainedTypeParameter())
+                if (!fieldType.NullableAnnotation.IsAnyNotNullable() && !fieldType.TypeSymbol.IsTypeParameterDisallowingAnnotation())


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

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)

gafter

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.

cston · 2018-12-18T18:47:03Z

    //                                    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)

cston · 2018-12-18T19:00:10Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

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

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

cston · 2018-12-18T19:06:06Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

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

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

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

cston · 2018-12-18T19:11:57Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

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

cston · 2018-12-18T19:13:20Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

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

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

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

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

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

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)

cston · 2018-12-18T19:15:26Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

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

cston · 2018-12-18T19:15:36Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

Consider writing two tests. #Closed

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

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

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

jcouv · 2018-12-19T05:29:03Z

Can the comment be removed and the issue resolved?

Indeed. Thanks #Resolved

cston · 2018-12-19T15:48:07Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbolWithAnnotations.cs

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

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

cston · 2018-12-19T16:25:58Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs

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

private static #Resolved

AlekseyTs · 2018-12-28T17:38:31Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

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


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

resultType? #Closed

AlekseyTs · 2018-12-28T17:42:02Z

                // 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)

AlekseyTs · 2018-12-28T17:50:31Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

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


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 · 2018-12-28T18:22:35Z

            // (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 · 2018-12-28T18:43:26Z

Done with review pass (iteration 20) #Closed

jcouv · 2018-12-28T21:24:35Z

                // 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 · 2018-12-28T23:44:45Z

            // (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)

AlekseyTs · 2018-12-28T23:56:10Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

+            if (_conversions.IncludeNullability && _getTypeWithAnnotationOpt != null)
            {
-                return NullableAnnotation.Unknown;
+                return _getTypeWithAnnotationOpt.Invoke(expr);


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

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

jcouv · 2018-12-28T23:58:04Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs

                {
-                    var annotation = GetNullableAnnotation(argument);
-                    ExactOrBoundsInference(kind, TypeSymbolWithAnnotations.Create(source, annotation), target, ref useSiteDiagnostics);
+                    ExactOrBoundsInference(kind, GetTypeWithAnnotations(argument), target, ref useSiteDiagnostics);


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

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

AlekseyTs · 2018-12-29T00:07:22Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

        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);


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

Minor: This isn't really a predicate #Resolved

Perhaps transform then? #Resolved

AlekseyTs · 2018-12-29T00:09:10Z

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

+        {
+            return SetNullabilityForReferenceTypes(setUnknownNullability);
+
+            TypeSymbolWithAnnotations setUnknownNullability(TypeSymbolWithAnnotations type)


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

AlekseyTs

LGTM (iteration 21)

jcouv · 2018-12-29T01:53:42Z

@jaredpar for approval for preview2. Thanks

jaredpar · 2018-12-29T03:12:08Z

approved to merge

jcouv · 2018-12-29T06:25:16Z

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.

cston · 2018-12-30T16:49:55Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MethodTypeInference.cs


+        // 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();


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

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 added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Dec 15, 2018

jcouv added this to the 16.0.P2 milestone Dec 15, 2018

jcouv self-assigned this Dec 15, 2018

jcouv requested a review from a team as a code owner December 15, 2018 04:42

cston mentioned this pull request Dec 15, 2018

[Work items] Nullable reference types #22152

Closed

jcouv force-pushed the speakable-types branch from bf9fb05 to be28d5e Compare December 15, 2018 17:08

gafter reviewed Dec 16, 2018

View reviewed changes

gafter approved these changes Dec 16, 2018

View reviewed changes

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 force-pushed the speakable-types branch from c621e1e to 2370dee Compare December 18, 2018 01:09

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 reviewed Dec 18, 2018

View reviewed changes

cston reviewed Dec 19, 2018

View reviewed changes

AlekseyTs reviewed Dec 28, 2018

View reviewed changes

Address feedback from Aleksey

9dbc302

jcouv force-pushed the speakable-types branch from c04c98e to 9dbc302 Compare December 28, 2018 23:42

AlekseyTs reviewed Dec 28, 2018

View reviewed changes

jcouv commented Dec 28, 2018

View reviewed changes

AlekseyTs reviewed Dec 29, 2018

View reviewed changes

AlekseyTs approved these changes Dec 29, 2018

View reviewed changes

Tweaks

249a73f

jcouv force-pushed the speakable-types branch from 622c6b9 to 249a73f Compare December 29, 2018 01:42

jaredpar added the Approved to merge label Dec 29, 2018

Fix another leak of nested unspeakable types

76110ea

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

Handle another leak of unspeakable types

74f9743

jcouv force-pushed the speakable-types branch from 6b11302 to 74f9743 Compare December 29, 2018 19:39

cston reviewed Dec 30, 2018

View reviewed changes

Revert from lambda to local function

fb48884

jcouv merged commit 0a81853 into dotnet:master Dec 31, 2018

jcouv deleted the speakable-types branch December 31, 2018 18:54

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

		private static bool IsUnconstrainedTypeParameter(TypeSymbol typeOpt)

Conversation

jcouv commented Dec 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

gafter Dec 16, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 19, 2018

Choose a reason for hiding this comment

Uh oh!

gafter left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cston Dec 18, 2018 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cston Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Dec 15, 2018 •

edited

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

jcouv Dec 16, 2018 •

edited

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

jcouv Dec 17, 2018 •

edited

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

gafter Dec 16, 2018 •

edited by jcouv

Loading

cston commented Dec 18, 2018 •

edited

Loading

cston Dec 18, 2018 •

edited by jcouv

Loading

cston Dec 18, 2018 •

edited

Loading

jcouv Dec 19, 2018 •

edited

Loading

cston Dec 18, 2018 •

edited

Loading

cston Dec 18, 2018 •

edited

Loading

jcouv Dec 19, 2018 •

edited

Loading

cston Dec 19, 2018 •

edited

Loading

jcouv Dec 19, 2018 •

edited

Loading

cston Dec 18, 2018 •

edited

Loading

cston Dec 18, 2018 •

edited by jcouv

Loading

jcouv Dec 18, 2018 •

edited

Loading

jcouv commented Dec 19, 2018 •

edited

Loading

cston Dec 19, 2018 •

edited by jcouv

Loading

cston Dec 19, 2018 •

edited by jcouv

Loading

AlekseyTs Dec 28, 2018 •

edited

Loading

AlekseyTs commented Dec 28, 2018 •

edited

Loading

AlekseyTs Dec 28, 2018 •

edited

Loading

AlekseyTs commented Dec 28, 2018 •

edited

Loading

AlekseyTs commented Dec 28, 2018 •

edited

Loading

AlekseyTs Dec 28, 2018 •

edited by jcouv

Loading