Enable nullable reference types for generated code by sharwell · Pull Request #37758 · dotnet/roslyn

sharwell · 2019-08-06T15:34:01Z

Review commit-by-commit is recommended.

…ullable-syntax

sharwell · 2019-08-06T16:08:56Z

@dotnet/roslyn-compiler the build errors are caused by a conflict with db84d66 that occurred in parallel. That change was arguably a breaking API change (inputs that were previously allowed now throw exceptions, even though the impacted source code is not valid C# code). The easiest way to address the code generation issue is to revert the change. Otherwise, I need to figure out how to make a non-nullable Update method override a nullable Update method.

/cc @CyrusNajmabadi

CyrusNajmabadi · 2019-08-06T17:33:27Z

@dotnet/roslyn-compiler the build errors are caused by a conflict with db84d66 that occurred in parallel. That change was arguably a breaking API change (inputs that were previously allowed now throw exceptions

Could you give an example of an input that was previously allowed that we would want to keep allowing?

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

sharwell · 2019-09-23T17:54:05Z

@dotnet/roslyn-compiler this is ready for review

src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/AbstractFileWriter.cs

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

.editorconfig

333fred · 2019-09-24T20:16:00Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

-                            WriteLine("      var {0} = ({1})this.Visit(node.{2});", CamelCase(field.Name), field.Type, field.Name);
+                            if (!IsOptional(field) && field.Type != "SyntaxToken")
+                            {
+                                WriteLine("      var {0} = ({1})this.Visit(node.{2}) ?? throw new ArgumentNullException(\"{0}\");", CamelCase(field.Name), GetFieldType(field, green: false), field.Name);


{1} [](start = 60, length = 3)

This cast ends up coming out as ? often, which is wrong. We should not be trying to cast to a nullable reference type and throwing if it's null.

What form would you suggest instead?

The generated cast should not have a ? in it (for reference types, if casting to an NVT of course it needs one).

So to clarify, you want this?

-(T?)Method() ?? throw ... +(T)(Method() ?? throw ...)

We should not be trying to cast to a nullable reference type and throwing if it's null.

FWIW, this is exactly what we do today, you just can't see it in the code.

That's correct. And more accurately, we cast to a regular reference type and throw if it was null.

➡️ Since this is a cosmetic change only, I'm going to file this as a follow-up issue to clean up the generated code for this case.

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

333fred · 2019-09-24T20:32:03Z

Done review pass (commit 24)

333fred

LGTM (commit 26). @dotnet/roslyn-compiler for a second review.

RikkiGibson · 2019-10-05T05:02:13Z

We need a senior compiler review @dotnet/roslyn-compiler

CyrusNajmabadi · 2019-10-05T20:15:15Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

                        else
                        {
-                            WriteLine("      var {0} = ({1})this.Visit(node.{2});", CamelCase(field.Name), field.Type, field.Name);
+                            if (!IsOptional(field) && field.Type != "SyntaxToken")


field.Type != "SyntaxToken" will always be true since the previous if-check checks for "SyntaxToken" already.

CyrusNajmabadi · 2019-10-05T20:23:00Z

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs

                else
                {
-                    Write("{0} == null ? null : (Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.{1}){0}.Green", CamelCase(field.Name), field.Type);
+                    if (IsAnyList(field.Type) || IsOptional(field))


IsAnyList would already have been covered by the if-checks above, right?

333fred · 2019-10-07T16:24:53Z

@dotnet/roslyn-compiler for a second review please.

jcouv · 2019-10-07T18:16:50Z

I'll look today

jcouv · 2019-10-07T18:17:21Z

Oh, never mind, I see that we have two reviews already.

CyrusNajmabadi · 2019-10-07T21:31:04Z

Awesome!

sharwell added 21 commits August 6, 2019 07:01

Enable nullable reference types for generated syntax trees

fd83561

Update GetFieldType to support nullable types

679bb57

GetSlot, GetNodeSlot, and GetCachedSlot can return null

7ff1c25

Node parents can be null

5af1659

Use GetFieldType instead of field.Type

808e815

Avoid null check for non-optional values

5185745

Use GetFieldType instead of field.Type

09e16fd

GetRed and GetRedAtZero sometimes return non-null

3f8ebc4

Avoid unnecessary null checks

47ad510

Suppress RS0027 in generated code

785b0aa

Lists are lazily initialized

0207ab2

Update GetRedFieldType for nullability

f4dd8ce

Update GetRedPropertyType for nullability

af897a0

Use GetRedPropertyType instead of inline logic

7239dd5

The minimal red factory makes certain required nodes optional

d2c8503

Use null-aware token casts

629bae5

Use GetRedPropertyType instead of field.Type

51a6ffb

Rewriter methods may return null

f0d684a

Use GetFieldType instead of field.Type

53ec5d2

Throw exception early when required value is null

93482af

Merge remote-tracking branch 'dotnet/release/dev16.4-preview1' into n…

ef049b5

…ullable-syntax

CyrusNajmabadi reviewed Aug 6, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 6, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 6, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

jinujoseph added the Area-Compilers label Aug 6, 2019

Use correct type when overriding methods

2715918

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 9, 2019

sharwell marked this pull request as ready for review August 12, 2019 22:10

sharwell removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Sep 23, 2019

sharwell requested a review from a team as a code owner September 23, 2019 17:53

CyrusNajmabadi reviewed Sep 23, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 23, 2019

View reviewed changes

src/Compilers/CSharp/Portable/Generated/Syntax.xml.Internal.Generated.cs Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 23, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/AbstractFileWriter.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 23, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

CyrusNajmabadi approved these changes Sep 23, 2019

View reviewed changes

333fred reviewed Sep 24, 2019

View reviewed changes

.editorconfig Outdated Show resolved Hide resolved

333fred reviewed Sep 24, 2019

View reviewed changes

src/Tools/Source/CompilerGeneratorTools/Source/CSharpSyntaxGenerator/SourceWriter.cs Show resolved Hide resolved

sharwell added 2 commits September 27, 2019 07:39

Remove the RS0027 suppression, since it is no longer necessary

4a23ebf

Rename GetNullableType and make it a local function to avoid confusion

0f5122d

333fred approved these changes Sep 27, 2019

View reviewed changes

sharwell mentioned this pull request Oct 4, 2019

Simplify C# syntax generators. #39077

Merged

CyrusNajmabadi reviewed Oct 5, 2019

View reviewed changes

cston approved these changes Oct 7, 2019

View reviewed changes

Merge remote-tracking branch 'dotnet/master' into nullable-syntax

00adc48

333fred approved these changes Oct 7, 2019

View reviewed changes

jcouv self-assigned this Oct 7, 2019

jcouv added this to the 16.4.P3 milestone Oct 7, 2019

sharwell merged commit 67021b4 into dotnet:master Oct 7, 2019

sharwell deleted the nullable-syntax branch October 7, 2019 21:05

Conversation

sharwell commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharwell commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Aug 6, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sharwell commented Sep 23, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

333fred Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

sharwell Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

333fred Sep 26, 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!

sharwell Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

sharwell Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

333fred Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

sharwell Sep 26, 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!

Uh oh!

333fred commented Sep 24, 2019

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Oct 5, 2019

Uh oh!

CyrusNajmabadi Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Oct 5, 2019

Choose a reason for hiding this comment

Uh oh!

333fred commented Oct 7, 2019

Uh oh!

jcouv commented Oct 7, 2019

Uh oh!

jcouv commented Oct 7, 2019

Uh oh!

CyrusNajmabadi commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sharwell commented Aug 6, 2019 •

edited

Loading

sharwell commented Aug 6, 2019 •

edited

Loading

333fred Sep 26, 2019 •

edited

Loading

sharwell Sep 26, 2019 •

edited

Loading