Nullable analysis of yield return #31072

jcouv · 2018-11-09T19:42:02Z

Implements NullableWalker.VisitYieldReturnStatement and use TSWA for MethodSymbol.IteratorElementType (and machinery to produce it).

Fixes #23701

jcouv · 2018-11-09T19:51:55Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

+    }
+}
+
+#nullable disable


📝 This code was moved from the async-streams tests, but I added a #nullable disable here to avoid nullable warnings in types that we only privately depend on. #Resolved

agocke · 2018-11-09T20:15:50Z

@jcouv I have a dumb question. In LDM we've basically decided that we're going to treat nullable as though it were an analyzer or layer on top of the type system. Seeing every little piece of code flowing nullable in Binding seems error prone and difficult to follow.

Is it possible to move this out of Binding entirely and just rely on the nullable walker pass to create an augmented tree?

Edit: never mind this is a bad idea. We really need to keep track of nullability at the symbol level, but there's no tree to rewrite (except for creating an entirely new symbol table).

This is unfortunate. #Resolved

agocke

LGTM except for missing test

agocke · 2018-11-09T20:58:24Z

src/Compilers/CSharp/Portable/Lowering/SynthesizedMethodBaseSymbol.cs

                {
-                    _iteratorElementType = TypeMap.SubstituteType(BaseMethod.IteratorElementType).TypeSymbol;
+                    if (BaseMethod.IteratorElementType.IsNull)


Does TypeMap.SubstituteType not take a TypeSymbolWithAnnotations overload? #Resolved

agocke · 2018-11-09T21:00:44Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs

@@ -347,7 +353,7 @@ await foreach (var s in M())
 ref struct S
 {
 }";
-            var comp = CreateCompilationWithTasksExtensions(new[] { source, s_common }, options: TestOptions.DebugExe);
+            var comp = CreateCompilationWithTasksExtensions(new[] { source, AsyncStreamsTypes }, options: TestOptions.DebugExe);


All of these could use the helper I created CreateCompilationWithAsyncIterator. Up to you, though. #Resolved

agocke · 2018-11-09T21:11:02Z

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

+                Diagnostic(ErrorCode.WRN_NullReferenceReturn, "null").WithLocation(25, 26)
+                );
+        }
+
        [Fact]


I'd expect a yield break positive test just to verify it generates no warnings. #Resolved

jcouv · 2018-11-09T22:23:55Z

@agocke Thanks
I rebased (to solve the CI build issue) and addressed your feedback. #Resolved

jcouv · 2018-11-09T22:54:48Z

@agocke From discussion with Chuck, I think your original point was valid. Maybe I don't need to make IteratorElementType a TSWA.
Initially, I thought it was exposed by a public API, so I went down that road. I'll undo and explore your proposal. #Resolved

cston · 2018-11-09T23:24:24Z

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

@@ -946,7 +946,7 @@ protected override BoundNode VisitReturnStatementNoAdjust(BoundReturnStatement n
            Debug.Assert(!IsConditionalState);

            BoundExpression expr = node.ExpressionOpt;
-            if (expr == null)
+            if (expr == null || expr.HasErrors)


expr.HasErrors [](start = 32, length = 14)

expr.HasErrors [](start = 32, length = 14)

We should report nullable issues within expr, even if the outer BoundExpression has errors. #Resolved

cston · 2018-11-09T23:25:46Z

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

            return method.IsGenericTaskReturningAsync(compilation) ?
-                ((NamedTypeSymbol)returnType.TypeSymbol).TypeArgumentsNoUseSiteDiagnostics.Single() :
-                returnType;
+               ((NamedTypeSymbol)returnType.TypeSymbol).TypeArgumentsNoUseSiteDiagnostics.Single() :


[](start = 0, length = 15)

[](start = 0, length = 15)

Indenting looks incorrect. #Resolved

cston · 2018-11-09T23:26:51Z

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

+        public override BoundNode VisitYieldReturnStatement(BoundYieldReturnStatement node)
+        {
+            BoundExpression expr = node.Expression;
+            if (expr == null || expr.HasErrors)


expr.HasErrors [](start = 32, length = 14)

expr.HasErrors [](start = 32, length = 14)

Report warnings within expr even if expr.HasErrors. #Resolved

jcouv · 2018-11-10T01:15:24Z

Solution without changes to binding works well. Thanks!
Rebased again to deal with latest conflicts. #Resolved

jcouv · 2018-11-10T01:21:19Z

@jaredpar Please approve for preview2. Thanks

cston · 2018-11-12T15:03:16Z

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

+            if (expr == null)
+            {
+                return null;
+            }


} [](start = 12, length = 1)

Please add a test where expr.HasErrors and also results in nullable warnings. #Resolved

Sorry, it looks like I had forgotten to push the commit updating tests. There are already tests for this.
Please take another look.

I much prefer the behavior that avoids cascading diagnostics. We should discuss more. I filed a follow-up issue: #31121 #Resolved

333fred · 2018-11-12T18:58:59Z

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

+                        return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0];
+                }
+
+                if (returnType.OriginalDefinition == compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_IAsyncEnumerable_T))


System_Collections_Generic_IAsyncEnumerable_T [](start = 96, length = 45)

More general question: do we want to make this iterator type a SpecialType, like the other iterators? And does async enumerable have an equivalent enumerator type we need to worry about here? #Resolved

From my understanding, "special type" is like a "well-know type", except that it must live in the same assembly as object (ie. in the core library).
We haven't added a "special type" in a long time (the entire GitHub history).

Yes, there is a corresponding IAsyncEnumerator<T> type. Async-iterator methods can't yet return such enumerators yet. That will be in a PR later this week (#31114). I added a note there to update this code too. Good catch

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

333fred · 2018-11-12T18:59:20Z

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

+                    return ((NamedTypeSymbol)returnType).TypeArgumentsNoUseSiteDiagnostics[0];
+                }
+
+                Debug.Assert(false);


Consider throwing ExceptionUtilities.Unreachable? #Resolved

Had to adjust this further for regressions caught by CI. Now return default;. Sorry for the churn.
The scenario is that you put a yield return in a regular method.

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

agocke · 2018-11-12T22:21:22Z

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

+            VisitOptionalImplicitConversion(expr, getIteratorElementType(method.ReturnType.TypeSymbol), useLegacyWarnings: false, AssignmentKind.Return);
+            return null;
+
+            TypeSymbolWithAnnotations getIteratorElementType(TypeSymbol returnType)


Looks like we have a helper GetIteratorElementTypeFromReturnType that already exists and is almost identical to this function. Can we share?

I considered and tried it, but the code ended up worse in my opinion.
To make a static method, we need to pass two extra parameters (binder and compilation). Trying to only use the compilation and not the binder is possible but was trouble.
Then the two methods also differ in their handling of errors. The original method uses a GetSpecialType that requires a diagnostic bag (I could pass a dummy one I suppose).

Let me know what you think.

FWIW, here's what that looks like:
jcouv@e81acf5

Obsolete review

agocke

LGTM

agocke · 2018-11-12T23:24:05Z

src/Compilers/CSharp/Portable/Binder/InMethodBinder.cs

        {
            if (refKind == RefKind.None && returnType.Kind == SymbolKind.NamedType)
            {
                switch (returnType.OriginalDefinition.SpecialType)
                {
                    case SpecialType.System_Collections_IEnumerable:
                    case SpecialType.System_Collections_IEnumerator:
-                        return GetSpecialType(SpecialType.System_Object, diagnostics, errorLocationNode);
+                        var objectType = diagnostics is null ? compilation.GetSpecialType(SpecialType.System_Object) : binder.GetSpecialType(SpecialType.System_Object, diagnostics, errorLocationNode);


Maybe split the ternary over three lines?

jcouv · 2018-11-13T03:52:47Z

@jaredpar Please approve for preview2. Thanks

jaredpar · 2018-11-13T17:00:58Z

Approved

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Nov 9, 2018

jcouv added this to the 16.0.P2 milestone Nov 9, 2018

jcouv self-assigned this Nov 9, 2018

jcouv requested a review from a team as a code owner November 9, 2018 19:42

jcouv commented Nov 9, 2018

View reviewed changes

agocke previously approved these changes Nov 9, 2018

View reviewed changes

jcouv force-pushed the yield-return branch from e166cbe to 32cbd80 Compare November 9, 2018 22:23

cston reviewed Nov 9, 2018

View reviewed changes

jcouv and others added 5 commits November 9, 2018 17:05

Nullable analysis of yield return

53d4e8a

Address feedback from Andy

5019025

Bulk undo all modified binding files

ea0ac2a

Take a NullableWalker-only approach

875554c

Address feedback from Chuck

c06f8af

jcouv force-pushed the yield-return branch from 02569c2 to c06f8af Compare November 10, 2018 01:14

cston reviewed Nov 12, 2018

View reviewed changes

cston approved these changes Nov 12, 2018

View reviewed changes

333fred reviewed Nov 12, 2018

View reviewed changes

jcouv mentioned this pull request Nov 12, 2018

Async-iterator method can return IAsyncEnumerator<T> #31114

Merged

Update tests and address feedback

d138c17

333fred approved these changes Nov 12, 2018

View reviewed changes

Fix regression caught by CI

cc016e4

agocke reviewed Nov 12, 2018

View reviewed changes

Factor code more

410272e

jcouv force-pushed the yield-return branch from 74f6e01 to 410272e Compare November 12, 2018 23:14

agocke approved these changes Nov 12, 2018

View reviewed changes

jcouv force-pushed the yield-return branch from da01511 to 2156033 Compare November 13, 2018 01:09

Inlined binder.GetSpecialType to remove binder parameter

3965ca0

jcouv force-pushed the yield-return branch from 2156033 to 3965ca0 Compare November 13, 2018 02:08

jcouv added the Approved to merge label Nov 13, 2018

jcouv merged commit ff8e1c7 into dotnet:dev16.0-preview2 Nov 13, 2018

jcouv deleted the yield-return branch November 13, 2018 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nullable analysis of yield return #31072

Nullable analysis of yield return #31072

jcouv commented Nov 9, 2018 •

edited

Loading

jcouv Nov 9, 2018 •

edited

Loading

agocke commented Nov 9, 2018 •

edited by jcouv

Loading

agocke left a comment

agocke Nov 9, 2018 •

edited by jcouv

Loading

agocke Nov 9, 2018 •

edited by jcouv

Loading

agocke Nov 9, 2018 •

edited by jcouv

Loading

jcouv commented Nov 9, 2018 •

edited

Loading

jcouv commented Nov 9, 2018 •

edited

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

jcouv commented Nov 10, 2018 •

edited

Loading

jcouv commented Nov 10, 2018

cston Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018 •

edited

Loading

333fred Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018 •

edited

Loading

333fred Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018

agocke Nov 12, 2018

jcouv Nov 12, 2018 •

edited

Loading

jcouv Nov 12, 2018

agocke left a comment

agocke Nov 12, 2018

jcouv commented Nov 13, 2018

jaredpar commented Nov 13, 2018

Nullable analysis of yield return #31072

Nullable analysis of yield return #31072

Conversation

jcouv commented Nov 9, 2018 • edited Loading

jcouv Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

agocke commented Nov 9, 2018 • edited by jcouv Loading

agocke left a comment

Choose a reason for hiding this comment

agocke Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

agocke Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

agocke Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Nov 9, 2018 • edited Loading

jcouv commented Nov 9, 2018 • edited Loading

cston Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Nov 9, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv commented Nov 10, 2018 • edited Loading

jcouv commented Nov 10, 2018

cston Nov 12, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Nov 12, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Nov 12, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Nov 12, 2018

Choose a reason for hiding this comment

agocke Nov 12, 2018

Choose a reason for hiding this comment

jcouv Nov 12, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Nov 12, 2018

Choose a reason for hiding this comment

agocke left a comment

Choose a reason for hiding this comment

agocke Nov 12, 2018

Choose a reason for hiding this comment

jcouv commented Nov 13, 2018

jaredpar commented Nov 13, 2018

jcouv commented Nov 9, 2018 •

edited

Loading

jcouv Nov 9, 2018 •

edited

Loading

agocke commented Nov 9, 2018 •

edited by jcouv

Loading

agocke Nov 9, 2018 •

edited by jcouv

Loading

agocke Nov 9, 2018 •

edited by jcouv

Loading

agocke Nov 9, 2018 •

edited by jcouv

Loading

jcouv commented Nov 9, 2018 •

edited

Loading

jcouv commented Nov 9, 2018 •

edited

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

cston Nov 9, 2018 •

edited by jcouv

Loading

jcouv commented Nov 10, 2018 •

edited

Loading

cston Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018 •

edited

Loading

333fred Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018 •

edited

Loading

333fred Nov 12, 2018 •

edited by jcouv

Loading

jcouv Nov 12, 2018 •

edited

Loading