`is` operator informs nullability analysis #28686

jcouv · 2018-07-19T04:04:04Z

We know that x is not null following x is string, x is string s, x is K (where K is a non-null constant).
We know that s is not null following x is string s.
We know that y has the same nullability as x following x is var y.
Produce a hidden warning if a null test could never succeed ("string" is null).

Relates to #22152

gafter · 2018-07-19T04:55:52Z

Produce a hidden warning if a null test could never succeed ("string" is null).

Does not sound like a good idea, even as a hidden diagnostic. If a public API method accepts a non-nullable parameter, you still want to encourage the presence of code that guards against null.
#Resolved

jcouv · 2018-07-19T05:36:32Z

@gafter We already produce such a hidden diagnostic on x == null and x != null, and I think that was discussed in LDM.
I understand that null checks are okay. Mads even has a topic queued on generating such checks for public APIs.
I'll capture an open issue. #Resolved

cston · 2018-07-19T14:06:28Z

docs/features/nullable-reference-types.md

@@ -75,9 +75,17 @@ Flow analysis is used to infer the nullability of variables within executable co

 ### Warnings
 _Describe set of warnings. Differentiate W warnings._
+If the analysis determines that a null check always (or never) passes, a hidden warning is produced. For example: `"string" is null`.


hidden diagnostic? #Resolved

cston · 2018-07-19T14:07:15Z

docs/features/nullable-reference-types.md

+- comparisons to `null`: `x == null` and `x != null`
+- `is` operator: `x is null`, `x is K` (where `K` is a constant), `x is string`, `x is string s`
+
+Invocation to methods annotated with the following attributes will also affect flow analysis:


Perhaps Invocation of. #Resolved

cston · 2018-07-19T14:39:55Z

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

        {
+            bool? nullKnowledge = null;


Could this be named isNull? #Resolved

cston · 2018-07-19T14:43:26Z

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

                    break;
            }
+
+            int slot = -1;


Consider adding Debug.Assert(!IsConditionalState);. #Resolved

cston · 2018-07-19T15:17:42Z

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

+
+            int slot = -1;
+            var operand = node.Operand;
+            if (operand.Type.IsReferenceType)


Please test unconstrained generic types.

static void F<T>(T t, object? o) { if (t is string) t.ToString(); if (o is T) o.ToString(); }

#Resolved

Thanks! That's caught two issues:

I've adjusted the check here (we could even remove it, but why track variables with struct type?)

even though we realize that t isn't null after t is string, we still report a possible null de-reference. That's an existing issue with variables of unconstrained generic type (added a PROTOTYPE). #Resolved

cston · 2018-07-19T15:21:45Z

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

+{
+    void Test(object? x)
+    {
+        if (x is C c)


Consider testing x is T t where T is unconstrained or constrained to struct or class. #Resolved

333fred · 2018-07-19T20:56:27Z

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

+{
+    void Test(object? x)
+    {
+        if (x is null)


x is null [](start = 12, length = 9)

Consider tests of inverted logic (ie if (!(x is null))) #Resolved

333fred · 2018-07-19T20:58:16Z

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

+    void Test(object? x)
+    {
+        const string? nullConstant = null;
+        if (x is nullConstant)


nullConstant [](start = 17, length = 12)

Consider a behavior verification test of an is comparison to a nonconstant. #Pending

That's an existing error (constant expected). The is operator works with a type, a declaration pattern or a constant pattern. #Resolved

It might be an existing error, but it's good to ensure we don't crash in this case.

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

333fred · 2018-07-19T20:58:37Z

Done review pass (commit 2) #Resolved

cston · 2018-07-20T15:47:02Z

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

+        /// `switch (x) ... case Point p:` // PROTOTYPE(NullableReferenceTypes): not yet handled
+        ///
+        /// If the expression is trackable, we'll return with different null-states for that expression in the two conditional states.
+        /// If the patterns is a `var` pattern, we'll also have re-inferred the `var` type with nullability and


If the pattern is … (singular)? #Resolved

cston · 2018-07-20T15:58:18Z

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

+        else
+        {
+            x.ToString(); // warn
+        }


Consider adding if (x is var _) { } in the same test. #Resolved

333fred · 2018-07-20T20:26:32Z

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

+            CSharpCompilation c = CreateCompilation(@"
+class C
+{
+    static void F<T>(T t, object? o)


T t [](start = 21, length = 3)

Nit: t is unused, and below. #Resolved

333fred

LGTM (commit 6) with additional test suggestion.

…operators * features/NullableReferenceTypes: `is` operator informs nullability analysis (dotnet#28686) Make module-level NonNullTypes explicit in tests (dotnet#28769) Lazily evaluate NonNullTypes (dotnet#28736) Calculate TypeSymbolWithAnnotations.IsNullable lazily (dotnet#28687)

jcouv added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jul 19, 2018

jcouv added this to the 16.0 milestone Jul 19, 2018

jcouv requested a review from a team as a code owner July 19, 2018 04:04

is operator informs nullability analysis

02f4261

jcouv force-pushed the null-pattern branch from 11cb0d1 to 02f4261 Compare July 19, 2018 05:26

cston reviewed Jul 19, 2018

View reviewed changes

Fixup doc

9f0d2d8

cston reviewed Jul 19, 2018

View reviewed changes

333fred reviewed Jul 19, 2018

View reviewed changes

Fix two regressions

bd9c0d5

jcouv force-pushed the null-pattern branch from 463683f to 6b6b221 Compare July 20, 2018 08:17

Address PR feedback

607c155

jcouv force-pushed the null-pattern branch from 6b6b221 to 607c155 Compare July 20, 2018 08:24

Rename local

ddda977

cston reviewed Jul 20, 2018

View reviewed changes

cston approved these changes Jul 20, 2018

View reviewed changes

Address PR feedback (2)

661e4f1

333fred reviewed Jul 20, 2018

View reviewed changes

Address more PR feedback

0503021

jcouv force-pushed the null-pattern branch from 07d3524 to 0503021 Compare July 23, 2018 18:25

jcouv merged commit 38c858c into dotnet:features/NullableReferenceTypes Jul 23, 2018

jcouv deleted the null-pattern branch July 23, 2018 19:13

jcouv mentioned this pull request Jul 23, 2018

Nullability not tracked for is/cast pattern in switch #26737

Closed

jcouv mentioned this pull request Sep 17, 2018

Support patterns in nullable walker #29909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`is` operator informs nullability analysis #28686

`is` operator informs nullability analysis #28686

jcouv commented Jul 19, 2018

gafter commented Jul 19, 2018 •

edited by jcouv

Loading

jcouv commented Jul 19, 2018 •

edited

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

jcouv Jul 20, 2018 •

edited

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

333fred Jul 19, 2018 •

edited

Loading

333fred Jul 19, 2018 •

edited by jcouv

Loading

jcouv Jul 20, 2018 •

edited

Loading

333fred Jul 20, 2018

333fred commented Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

333fred Jul 20, 2018 •

edited by jcouv

Loading

333fred left a comment

is operator informs nullability analysis #28686

is operator informs nullability analysis #28686

Conversation

jcouv commented Jul 19, 2018

gafter commented Jul 19, 2018 • edited by jcouv Loading

jcouv commented Jul 19, 2018 • edited Loading

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

cston Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Jul 19, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

333fred Jul 20, 2018

Choose a reason for hiding this comment

333fred commented Jul 19, 2018 • edited by jcouv Loading

cston Jul 20, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

cston Jul 20, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred Jul 20, 2018 • edited by jcouv Loading

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

`is` operator informs nullability analysis #28686

`is` operator informs nullability analysis #28686

gafter commented Jul 19, 2018 •

edited by jcouv

Loading

jcouv commented Jul 19, 2018 •

edited

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

jcouv Jul 20, 2018 •

edited

Loading

cston Jul 19, 2018 •

edited by jcouv

Loading

333fred Jul 19, 2018 •

edited

Loading

333fred Jul 19, 2018 •

edited by jcouv

Loading

jcouv Jul 20, 2018 •

edited

Loading

333fred commented Jul 19, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

cston Jul 20, 2018 •

edited by jcouv

Loading

333fred Jul 20, 2018 •

edited by jcouv

Loading