Nullable annotation is no longer part of array rank specifier #32431

gafter · 2019-01-13T22:58:45Z

Fixes #32290 and other related issues

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

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

cston · 2019-01-14T17:15:21Z

        if (typeOpt.IsNull)

Check should be moved to ShouldPrintNullableAnnotation. #Resolved

Refers to: src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs:77 in fb6b088. [](commit_id = fb6b088, deletion_comment = False)

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

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

src/Compilers/CSharp/Test/Symbol/SymbolDisplay/SymbolDisplayTests.cs

Fixes dotnet#32290 and other related issues

gafter · 2019-01-14T21:09:46Z

What is the point of adding PR For Personal Review Only if y'all are going to ignore it? ;)

Anyway, thanks for the feedback. #Closed

jcouv · 2019-01-14T23:33:27Z

What is the point of adding PR For Personal Review Only if y'all are going to ignore it? ;)

In my experience, PRs marked as "personal" get reviewed the fastest. There may be some yet-to-be-named law for that ;-) #Closed

…d into semantic analysis - Move error checking for invalid array dimensions out of the parser and into semantic analysis - Diagnose `e is string?` Fixes dotnet#32025

src/Compilers/CSharp/Test/Syntax/Parsing/ParsingTests.cs

…le-32290

agocke · 2019-01-22T20:19:22Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5037,6 +5037,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_PatternNullableType" xml:space="preserve">
    <value>It is not legal to use nullable type '{0}' in a pattern; use the underlying type '{1}' instead.</value>
  </data>
+  <data name="ERR_IsNullableType" xml:space="preserve">
+    <value>It is not legal to use nullable type '{0}?' in an is-type expression; use the underlying type '{0}' instead.</value>


I would add "because is never matches null" to make it clear why it's illegal. I suspect people trying to do this may not have the correct mental model. #Resolved

That's not the reason. This is only given for nullable reference types. The reason is that it is a runtime check, and nullable reference types do not exist at runtime. I'll clarify.

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

…nce* types are illegal in an is-type or as-type expression.

gafter · 2019-01-22T22:02:48Z

I believe I have responded to all comments. Are there any further comments? #Resolved

…le-32290

jcouv

LGTM Thanks (iteration 8)

cston · 2019-01-25T20:10:04Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

            }

+done:;


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

Minor point: ; is unnecessary. #Resolved

cston · 2019-01-25T20:40:09Z

src/Compilers/CSharp/Test/Syntax/Parsing/StatementParsingTests.cs

@@ -3454,7 +3454,7 @@ static void Test(int arg1, (byte, byte) arg2)
 }
 ";
            var tree = SyntaxFactory.ParseSyntaxTree(source, options: TestOptions.Regular);
-            Assert.Equal(false, tree.GetRoot().ContainsDiagnostics);
+            tree.GetDiagnostics().Verify();
        }


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

Are we testing: _ = new(int, int)?{}; ? #Resolved

Also test error recovery for: _ = new (int, int) ? (x) : y;

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

cston · 2019-01-25T20:50:56Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        result = ScanTypeFlags.NullableType;
+                        break;
+                    case SyntaxKind.AsteriskToken
+                            when lastTokenOfType.Kind != SyntaxKind.CloseBracketToken: // don't allow `Type[]*`


Type[]* [](start = 103, length = 7)

Are we testing this case? #Resolved

cston · 2019-01-25T21:02:01Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        case ParseTypeMode.NewExpression:
+                            // A nullable qualifier is permitted as part of the type in a `new` expression.
+                            // e.g. `new int?()` is allowed.  It creates a null value of type `Nullable<int>`.
+                            // But the object initializer syntax `new int? {}` is not permitted.


new int? {} is not permitted [](start = 65, length = 30)

I'm confused. Should new int? { } be permitted? (It is today.)

The comment says it's not permitted but the return expression allows {. #Resolved

cston · 2019-01-25T21:38:21Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

@@ -16,15 +16,8 @@ private void VisitTypeSymbolWithAnnotations(TypeSymbolWithAnnotations type, Abst
            var visitor = (SymbolDisplayVisitor)(visitorOpt ?? this.NotFirstVisitor);


(SymbolDisplayVisitor) [](start = 26, length = 22)

The cast is no longer necessary. #Resolved

cston · 2019-01-25T21:45:27Z

src/Compilers/CSharp/Portable/SymbolDisplay/SymbolDisplayVisitor.Types.cs

            {
-                if (!this.isFirstSymbolVisited)
+                if (!(arrayType is null) && !this.isFirstSymbolVisited)


(arrayType is null [](start = 21, length = 18)

while condition already checked arrayType != null. #Resolved

cston · 2019-01-25T21:51:21Z

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

@@ -31,7 +31,11 @@ class C
            comp.VerifyDiagnostics(
                // (7,52): error CS1586: Array creation must have array size or array initializer
                //     Expression<Action<dynamic>> e = x => new object[](x);
-                Diagnostic(ErrorCode.ERR_MissingArraySize, "[]"));
+                Diagnostic(ErrorCode.ERR_MissingArraySize, "[]").WithLocation(7, 52),
+                // (7,42): error CS0149: Method name expected


error CS0149: Method name expected [](start = 27, length = 34)

This cascading error seems unfortunate. The first error makes sense but this second error seems incorrect given the first. #Resolved

cston · 2019-01-25T22:07:06Z

src/Compilers/CSharp/Test/Syntax/Parsing/NullableParsingTests.cs

+                }
+            }
+            EOF();
+        }


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

Please test with multiple dimensional array after ?:

new object[1]?[2,3]?[] ``` #Resolved

cston

Several comments and some test suggestions, but not blocking.

Fixes dotnet#32807

* Address minor issues identified in review #32431. Fixes #32807

jcouv · 2019-03-07T21:35:28Z

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

-            TypeSyntax elementTypeSyntax = arrayTypeSyntax.ElementType;
-            var elementType = BindType(elementTypeSyntax, diagnostics);
+            var elementTypeSyntax = arrayTypeSyntax.ElementType;
+            var arrayType = (ArrayTypeSymbol)BindArrayType(arrayTypeSyntax, diagnostics, permitDimensions: true, basesBeingResolved: null).TypeSymbol;


BindArrayType [](start = 45, length = 13)

I think this change is the cause of #33945
We didn't use to check whether the type is IsRestrictedType, but now we do (inside BindArrayType).

…#32431) Fixes dotnet#32290 Fixes dotnet#32169 Fixes dotnet#32141 Fixes dotnet#32025

gafter added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Jan 13, 2019

gafter self-assigned this Jan 13, 2019

gafter requested a review from a team as a code owner January 13, 2019 22:58

gafter added this to Working/In Review in Compiler: Gafter Jan 13, 2019

jcouv added Area-Compilers New Language Feature - Nullable Reference Types Nullable Reference Types labels Jan 14, 2019

jcouv added this to the 16.0.P2 milestone Jan 14, 2019

alrz reviewed Jan 14, 2019

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/PatternMatchingTests4.cs Outdated Show resolved Hide resolved