Update implicity declared type expression nullability: by chsienki · Pull Request #36078 · dotnet/roslyn

chsienki · 2019-05-31T01:14:56Z

Small fix for semantic model bug I found working on other correctness bits

When updating the implicit type in a declaration also update the bound type declaration nullability
Add tests

- When updating the implicit type in a declaration also update the bound type declaration nullability - Add tests

chsienki · 2019-05-31T17:04:43Z

@dotnet/roslyn-compiler for review please

jcouv · 2019-05-31T17:15:42Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

        }
+
+        [Fact]
+        public void InferredDeclarationType()


InferredDeclarationType [](start = 20, length = 23)

Could you also test other kinds of implicit declarations?
M(out var x);
var (x, y) = p;
foreach (var (x, y) in pCollection) ...
using (var x = d) ...
using var x = d; (probably already covered by your change) #Resolved

Deconstruction and foreach deconstruction won't work yet (see #35010 which I was working on when I discovered this :))

I'll try the others and see how they fair. #Resolved

jcouv · 2019-05-31T17:27:14Z

        if (local.IsRef)

Should we do something similar for ref locals? They apparently can have an inferred type as well:

public class C {
    public void M(string s) {
        ref var s2 = ref s;
    }
}
``` #Closed

---
Refers to: src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs:1374 in b450f7d. [](commit_id = b450f7da021d5504febbea29f74e0eb823a9212b, deletion_comment = False)

jcouv

Done with review pass (iteration 1)

333fred · 2019-05-31T21:41:27Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+            return;
+        var/*T:object!*/ y3 = x;
+
+        x = null;


Consider also doing this with x2. #Resolved

333fred · 2019-05-31T21:41:53Z

Done review pass (commit 1)

- Always record type for inferred declarations - Add nullable tests - Update nullable public api tests

chsienki · 2019-06-03T20:15:26Z

Expanded tests and fixed ref local case for both API and nullable analysis.

@dotnet/roslyn-compiler for second review please.

cston · 2019-06-03T20:16:48Z

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

+
+        [Fact]
+        [WorkItem(35057, "https://github.com/dotnet/roslyn/issues/35057")]
+        public void RefLocal()


RefLocal [](start = 20, length = 8)

Is this test different from the preceding test?

cston · 2019-06-03T20:21:23Z

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

+
+                if (node.DeclaredTypeOpt != null)
+                {
+                    SetAnalyzedNullability(node.DeclaredTypeOpt, new VisitResult(valueType, type), true);


SetAnalyzedNullability(node.DeclaredTypeOpt, new VisitResult(valueType, type), true) [](start = 20, length = 84)

Is this necessary? (What is the observable effect?) #Closed

In the semantic model, you can query the var of the declaration and get the type. This ensures we correctly report the nullability in that case. #Resolved

jcouv · 2019-06-03T20:31:24Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+        using var /*T:C!*/ y4 = x2;
+
+        using (var /*T:C?*/ y5 = x) { }
+        using (var /*T:C?*/ y6 = x) { }


x [](start = 33, length = 1)

Should be x2? #Closed

jcouv · 2019-06-03T20:32:37Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+
+        x = null;
+        var /*T:C?*/ y12 = x;
+        using var /*T:C?*/ y13 = x;


using var /T:C?/ y13 = x; [](start = 8, length = 27)

Duplicate with y3? #Closed

jcouv · 2019-06-03T20:33:31Z

src/Compilers/CSharp/Test/Symbol/Symbols/Source/NullablePublicAPITests.cs

+                Diagnostic(ErrorCode.WRN_ConvertingNullableToNonNullable, "null").WithLocation(29, 14)
+                );
+            comp.VerifyTypes();
+        }


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

Please also test foreach (var ...) (no deconstruction) #Closed

jcouv

LGTM Thanks (iteration 3) modulo an extra test (foreach)

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

333fred · 2019-06-03T23:11:21Z

LGTM (commit 3), also modulo additional suggestions from Julien.

- Record nullability for inferred foreach variable - Add foreach test

Update implicity declared type expression nullability:

b450f7d

- When updating the implicit type in a declaration also update the bound type declaration nullability - Add tests

chsienki added Area-Compilers Feature - Nullable Semantic Model Nullable Semantic Model Issues labels May 31, 2019

chsienki marked this pull request as ready for review May 31, 2019 02:30

chsienki requested a review from a team as a code owner May 31, 2019 02:30

chsienki requested a review from 333fred May 31, 2019 02:30

jcouv reviewed May 31, 2019

View reviewed changes

jcouv self-assigned this May 31, 2019

333fred reviewed May 31, 2019

View reviewed changes

Record inferred type for ref locals too:

fd7ce13

- Always record type for inferred declarations - Add nullable tests - Update nullable public api tests

cston reviewed Jun 3, 2019

View reviewed changes

Remove duplicate test.

9945e69

cston reviewed Jun 3, 2019

View reviewed changes

jcouv added this to the 16.2.P3 milestone Jun 3, 2019

jcouv reviewed Jun 3, 2019

View reviewed changes

jcouv approved these changes Jun 3, 2019

View reviewed changes

333fred reviewed Jun 3, 2019

View reviewed changes

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs Show resolved Hide resolved

333fred approved these changes Jun 3, 2019

View reviewed changes

cston approved these changes Jun 4, 2019

View reviewed changes

PR Feedback:

82237b5

- Record nullability for inferred foreach variable - Add foreach test

chsienki merged commit efcbc23 into dotnet:master Jun 5, 2019

chsienki deleted the var_decl_type_fix branch June 5, 2019 18:39

Conversation

chsienki commented May 31, 2019

Uh oh!

chsienki commented May 31, 2019

Uh oh!

jcouv May 31, 2019 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chsienki May 31, 2019 • 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 May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

333fred May 31, 2019 • edited by chsienki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred commented May 31, 2019

Uh oh!

chsienki commented Jun 3, 2019

Uh oh!

cston Jun 3, 2019

Choose a reason for hiding this comment

Uh oh!

cston Jun 3, 2019 • 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!

chsienki Jun 3, 2019 • 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 Jun 3, 2019 • 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 Jun 3, 2019 • 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 Jun 3, 2019 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

333fred commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv May 31, 2019 •

edited by chsienki

Loading

chsienki May 31, 2019 •

edited

Loading

jcouv commented May 31, 2019 •

edited

Loading

333fred May 31, 2019 •

edited by chsienki

Loading

cston Jun 3, 2019 •

edited

Loading

chsienki Jun 3, 2019 •

edited by jcouv

Loading

jcouv Jun 3, 2019 •

edited

Loading

jcouv Jun 3, 2019 •

edited

Loading

jcouv Jun 3, 2019 •

edited

Loading