Remove remaining artifacts of symbol-centric NonNullTypes context. #31607

AlekseyTs · 2018-12-07T03:06:04Z

Closes dotnet#30171. Closes dotnet#29838.

AlekseyTs · 2018-12-07T17:39:40Z

jcouv · 2018-12-07T22:34:30Z

src/Compilers/CSharp/Portable/Errors/LazyMissingNonNullTypesContextDiagnosticInfo.cs

@@ -10,32 +11,33 @@ namespace Microsoft.CodeAnalysis.CSharp
    internal sealed class LazyMissingNonNullTypesContextDiagnosticInfo : LazyDiagnosticInfo


LazyMissingNonNullTypesContextDiagnosticInfo [](start = 26, length = 44)

Feels like we don't need this anymore (we can do this check eagerly). #Resolved

Feels like we don't need this anymore (we can do this check eagerly).

No, we cannot do this eagerly because this class is used when we cannot ask if a type is a value type due to the risk of going into a cycle.

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

jcouv · 2018-12-07T22:39:43Z

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

@@ -834,7 +831,7 @@ private NamespaceOrTypeOrAliasSymbolWithAnnotations UnwrapAlias(NamespaceOrTypeO
            if (symbol.IsAlias)
            {
                AliasSymbol discarded;
-                return NamespaceOrTypeOrAliasSymbolWithAnnotations.CreateUnannotated(NonNullTypesContext, (NamespaceOrTypeSymbol)UnwrapAlias(symbol.Symbol, out discarded, diagnostics, syntax, basesBeingResolved));
+                return NamespaceOrTypeOrAliasSymbolWithAnnotations.CreateUnannotated(symbol.IsNullableEnabled, (NamespaceOrTypeSymbol)UnwrapAlias(symbol.Symbol, out discarded, diagnostics, syntax, basesBeingResolved));


symbol.IsNullableEnabled [](start = 85, length = 24)

It would be good to update the speclet to cover aliases.
My recollection is that the nullable context at the definition of the alias should not affect its top-level nullability, because it doesn't have one. The nullability comes from the place where the alias is used.
Is that correct? #WontFix

It would be good to update the speclet to cover aliases.

There is no change around aliases here, the context was taken from the binding position before and is taken from the binding position now. The symbol here is not an alias symbol, it is an NamespaceOrTypeOrAliasSymbolWithAnnotations structure. AliasSymbol stores only NamespaceOrTypeSymbol it never stored nullability or context. UsingDirectiveSyntax was never allowing NullableTypeSyntax as the target name.

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

jcouv · 2018-12-07T22:48:51Z

src/Compilers/CSharp/Portable/Symbols/Source/TypeParameterConstraintClause.cs

+                case TypeParameterConstraintKind.NotNullableReferenceType:
+                    break;
+                default:
+                    ExceptionUtilities.UnexpectedValue(constraints); // This call asserts.


// This call asserts. [](start = 69, length = 21)

nit: seems simpler to do throw ExceptionUtilities.UnexpectedValue(...); as we do everywhere else #WontFix

seems simpler to do throw ExceptionUtilities.UnexpectedValue(...); as we do everywhere else

I do not find this simpler and we do use this method for asserts in several places in the codebase, this is not a unique instance.

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

jcouv · 2018-12-07T23:00:22Z

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

+            AssertNonNullTypesContext_22(source, WithNonNullTypesTrue(), type, "A<System.String!>");
+        }
+
+        private static void AssertNonNullTypesContext_22(string source, CSharpCompilationOptions options, TypeSyntax type, string expected)


AssertNonNullTypesContext_22 [](start = 28, length = 28)

Since this method is re-used, consider giving it a meaningful name. Perhaps VerifySpeculativeTypeInfo or something even more specific (to distinguish from AssertNonNullTypesContext_30). #Resolved

jcouv · 2018-12-07T23:04:10Z

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

+"
+            );
+
+            AssertNonNullTypesContext_30(source, WithNonNullTypesFalse(), type, "A<System.String>");
        }


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

Consider adding a test showing that the last token in a qualified type name is the one that determines the nullable context. #Resolved

Consider adding a test showing that the last token in a qualified type name is the one that determines the nullable context.

Current tests cover all the cases when the context is queried. Implementation doesn't depend on whether IdentifiernameSyntax,or GenericNamesyntax are quilified.

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

jcouv

Done with review pass (iteration 1). Minor questions/feedback on aliases, and removing one more lazy diagnostic.

jcouv

LGTM Thanks (iteration 2)

AlekseyTs · 2018-12-10T06:12:25Z

@cston, @333fred Please review.

cston · 2018-12-10T18:37:22Z

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

                          customModifiers.NullToEmpty());
        }

+        internal static TypeSymbolWithAnnotations Create(TypeSymbol typeSymbol, ImmutableArray<ModifierInfo<TypeSymbol>> customModifiers)


Create [](start = 50, length = 6)

Is this used by SymbolFactory only? If so, please move the method back to that class as a helper method to avoid adding the extra overload here, especially since it uses ModifierInfo<T>. #Resolved

333fred · 2018-12-10T21:54:16Z

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

-            return typeParameters.SelectAsArray((tp) =>
-                TypeSymbolWithAnnotations.Create(NonNullTypesNullContext.Instance, tp, isAnnotated: false, customModifiers: ImmutableArray<CustomModifier>.Empty)
-                );
+            return typeParameters.SelectAsArray((tp) => TypeSymbolWithAnnotations.Create(tp));


There seem to be a few places using this method of creation. We could consider extracting this into a helper and doing the iteration by hand, saving the lambda allocations.

A helper method should continue to use a lambda since the additional memory for a single static delegate instance is probably similar to the extra instructions from a loop.

Remove remaining artifacts of symbol-centric NonNullTypes context.

261872e

Closes dotnet#30171. Closes dotnet#29838.

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Dec 7, 2018

AlekseyTs requested review from 333fred, cston and jcouv December 7, 2018 03:06

AlekseyTs requested a review from a team as a code owner December 7, 2018 03:06

jcouv reviewed Dec 7, 2018

View reviewed changes

jcouv self-assigned this Dec 7, 2018

Add tests

0a5fdd6

jcouv approved these changes Dec 8, 2018

View reviewed changes

cston mentioned this pull request Dec 8, 2018

[Work items] Nullable reference types #22152

Closed

cston reviewed Dec 10, 2018

View reviewed changes

Move helper method back to SymbolFactory

01567e9

cston approved these changes Dec 10, 2018

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into NullableContext_03

e1f5bbc

333fred reviewed Dec 10, 2018

View reviewed changes

AlekseyTs merged commit 0610c79 into dotnet:master Dec 10, 2018

jcouv mentioned this pull request Jan 30, 2019

Use last token in type name for #nullable context #30985

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove remaining artifacts of symbol-centric NonNullTypes context. #31607

Remove remaining artifacts of symbol-centric NonNullTypes context. #31607

AlekseyTs commented Dec 7, 2018

AlekseyTs commented Dec 7, 2018

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Dec 8, 2018

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Dec 8, 2018

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Dec 8, 2018

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

AlekseyTs Dec 8, 2018

jcouv left a comment

jcouv left a comment

AlekseyTs commented Dec 10, 2018

cston Dec 10, 2018 •

edited by AlekseyTs

Loading

333fred Dec 10, 2018

cston Dec 10, 2018 •

edited

Loading

		@@ -10,32 +11,33 @@ namespace Microsoft.CodeAnalysis.CSharp
		internal sealed class LazyMissingNonNullTypesContextDiagnosticInfo : LazyDiagnosticInfo

Remove remaining artifacts of symbol-centric NonNullTypes context. #31607

Remove remaining artifacts of symbol-centric NonNullTypes context. #31607

Conversation

AlekseyTs commented Dec 7, 2018

AlekseyTs commented Dec 7, 2018

jcouv Dec 7, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Dec 8, 2018

Choose a reason for hiding this comment

jcouv Dec 7, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Dec 8, 2018

Choose a reason for hiding this comment

jcouv Dec 7, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Dec 8, 2018

Choose a reason for hiding this comment

jcouv Dec 7, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

jcouv Dec 7, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

AlekseyTs Dec 8, 2018

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Dec 10, 2018

cston Dec 10, 2018 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

333fred Dec 10, 2018

Choose a reason for hiding this comment

cston Dec 10, 2018 • edited Loading

Choose a reason for hiding this comment

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 7, 2018 •

edited by AlekseyTs

Loading

cston Dec 10, 2018 •

edited by AlekseyTs

Loading

cston Dec 10, 2018 •

edited

Loading