Erase nullability from base and interfaces for types from unannotated assemblies by cston · Pull Request #28028 · dotnet/roslyn

cston · 2018-06-20T19:56:58Z

Erase nullability from base and interfaces for types from unannotated assemblies.

cston · 2018-06-20T19:59:25Z

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

                            if (a.QuestionToken.IsKind(SyntaxKind.QuestionToken))
                            {
-                                type = TypeSymbolWithAnnotations.CreateNullableReferenceType(array);
+                                type = TypeSymbolWithAnnotations.Create(array, isNullableIfReferenceType: true);


Inlined the one instance of CreateNullableReferenceType().

cston · 2018-06-20T20:00:15Z

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

            return new NonLazyType(typeSymbol, isNullable, customModifiers);
        }

-        public TypeSymbolWithAnnotations AsNullableReferenceOrValueType(CSharpCompilation compilation, SyntaxReference nullableTypeSyntax)


nullableTypeSyntax was not needed.

cston · 2018-06-21T18:07:56Z

@dotnet/roslyn-compiler please review.

jcouv · 2018-06-21T18:09:53Z

Please tag with Area-Compiler so it shows up in review queue. Thanks #Resolved

cston · 2018-06-26T15:29:27Z

@dotnet/roslyn-compiler please review.

jcouv · 2018-06-26T17:55:06Z

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

+{
+    static void F(object? x, B b)
    {
+        var y = b.F;


; [](start = 19, length = 1)

Maybe add a VerifyTypes comment on b.F? Expecting object~ #Closed

jcouv · 2018-06-26T17:56:07Z

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

        (new[] { x!, x! })[0].ToString();
        (new[] { y, y! })[0].ToString();
        (new[] { y!, y })[0].ToString();
    }


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

If it's not tested elsewhere, I'd also include permutations mixing x and y. Also, mixing z and w below. #Closed

jcouv · 2018-06-26T17:59:07Z

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

+        D<A> d;
+        d = Create(x).F;
+        x = y;
+        d = Create(x).F; // warning


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

Consider Create(y).F directly (rather than assigning x = y first). #Closed

jcouv · 2018-06-26T18:00:03Z

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

+        F(y, x)/*T:A?*/.ToString(); // 4
+        F(y, y)/*T:A!*/.ToString();
+        F(y, z)/*T:A!*/.ToString();
+        F(z, x)/*T:A*/.ToString(); // 5


// 5 [](start = 35, length = 4)

~~There is no warning for 5. I think this comment can be removed.~~ Never mind. There should be a warning (one the ordering issue is fixed) #Closed

jcouv · 2018-06-26T19:19:51Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

        private ImmutableArray<NamedTypeSymbol> _lazyInterfaces = default(ImmutableArray<NamedTypeSymbol>);
-        private NamedTypeSymbol _lazyDeclaredBaseType = ErrorTypeSymbol.UnknownResultType;
+        private NamedTypeSymbol _lazyDeclaredBaseType1 = ErrorTypeSymbol.UnknownResultType;
+        private NamedTypeSymbol _lazyDeclaredBaseType2 = ErrorTypeSymbol.UnknownResultType;


_lazyDeclaredBaseType2 [](start = 32, length = 22)

A comment would be useful (why do we need two?). Or maybe rename by adding "WithNullability" #Closed

jcouv · 2018-06-26T19:21:12Z

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

        }

+        // PROTOTYPE(NullableReferenceTypes): Combine with other GetDeclaredBaseType overload and make abstract.
+        internal virtual NamedTypeSymbol GetDeclaredBaseType(ConsList<Symbol> basesBeingResolved, bool ignoreNullability)


GetDeclaredBaseType [](start = 41, length = 19)

Where is this method called from? It seems that only the overload in PENamedTypeSymbol is called from PENamedTypeSymbol itself. #Closed

Removed method from base type, thanks.

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

jcouv · 2018-06-26T19:23:24Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+            if (ignoreNullability)
            {
-                Interlocked.CompareExchange(ref _lazyDeclaredBaseType, MakeDeclaredBaseType(), ErrorTypeSymbol.UnknownResultType);
+                return _lazyDeclaredBaseType1;


Let's use a descriptive name, rather than "1" and "2". #Resolved

jcouv

Done with review pass (iteration 5).

…s' into 27967

333fred · 2018-06-27T21:14:27Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PENamedTypeSymbol.cs

+            return GetDeclaredBaseType(basesBeingResolved, ignoreNullability: false);
+        }
+
+        private NamedTypeSymbol GetDeclaredBaseType(ConsList<Symbol> basesBeingResolved, bool ignoreNullability)


basesBeingResolved [](start = 69, length = 18)

Why do we even have this parameter if it's never used?

Good catch. Removed.

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

333fred · 2018-06-27T22:04:49Z

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

+        i1.F(x);
+        i1.F(y);
+        i2.F(x);
+        i2.F(y); // warn


This one is interesting. Perhaps more of a nullable design question, but is there going to be a way for a user to specify that I2.F takes a T?? Seems like a user would want to be able to override this behavior.

An implicit implementation of I.F could be defined as F(T?) since the interface method is null-oblivious, but interface I2 could not redefine I.F.

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

333fred

LGTM (commit 7) with a more general question on implementation strategy.

cston · 2018-06-28T05:13:38Z

@jcouv, @333fred, all feedback addressed.

…s' into 27967

jcouv · 2018-06-29T22:05:13Z

I know that rebasing causes other problems, but merging also makes the review difficult. Now a number of changes showing up in CodeFlow are not new changes... :-(

jcouv

LGTM

Add ignoreNullability parameter to GetDeclaredBaseType()

fef4382

cston requested a review from a team as a code owner June 20, 2018 19:56

cston changed the title ~~Add ignoreNullability parameter to GetDeclaredBaseType()~~ Erase nullability from base and interfaces for types from unannotated assemblies Jun 20, 2018

cston commented Jun 20, 2018

View reviewed changes

cston added 3 commits June 20, 2018 13:06

Add test

8a3bfe9

Fix tests

5e88643

Unskip test

408c3b0

cston added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jun 21, 2018

jcouv reviewed Jun 26, 2018

View reviewed changes

PR feedback

6ed14cc

jcouv reviewed Jun 26, 2018

View reviewed changes

jcouv self-assigned this Jun 26, 2018

PR feedback

584d1c8

cston mentioned this pull request Jun 26, 2018

Use [NonNullTypes] to interpret unannotated types in source #27807

Merged

4 tasks

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

afae76e

…s' into 27967

333fred reviewed Jun 27, 2018

View reviewed changes

333fred approved these changes Jun 27, 2018

View reviewed changes

Remove unused parameter

bcf2465

cston added 3 commits June 27, 2018 23:40

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

bee170e

…s' into 27967

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

3ab710f

…s' into 27967

Merge remote-tracking branch 'upstream/features/NullableReferenceType…

63108b5

…s' into 27967

jcouv approved these changes Jun 29, 2018

View reviewed changes

cston merged commit fffa9be into dotnet:features/NullableReferenceTypes Jun 30, 2018

cston deleted the 27967 branch June 30, 2018 06:51

cston mentioned this pull request Jul 25, 2018

Revert two step initialization of constraints and base type #28834

Closed

Conversation

cston commented Jun 20, 2018

Uh oh!

cston Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

cston Jun 20, 2018

Choose a reason for hiding this comment

Uh oh!

cston commented Jun 21, 2018

Uh oh!

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

Uh oh!

Uh oh!

cston commented Jun 26, 2018

Uh oh!

jcouv Jun 26, 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 Jun 26, 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 Jun 26, 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 Jun 26, 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 Jun 26, 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 Jun 26, 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 Jun 26, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Jun 26, 2018 • edited by cston 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!

333fred Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

cston Jun 28, 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!

333fred Jun 27, 2018

Choose a reason for hiding this comment

Uh oh!

cston Jun 28, 2018

Choose a reason for hiding this comment

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Jun 28, 2018

Uh oh!

jcouv commented Jun 29, 2018

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

jcouv commented Jun 21, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited

Loading

jcouv Jun 26, 2018 •

edited by cston

Loading

cston Jun 28, 2018 •

edited

Loading