Add "Nullable" compilation option. by AlekseyTs · Pull Request #30479 · dotnet/roslyn

AlekseyTs · 2018-10-12T18:23:34Z

No description provided.

AlekseyTs · 2018-10-12T21:15:24Z

@333fred, @cston, @jcouv, @dotnet/roslyn-compiler Please review

jcouv · 2018-10-12T22:13:02Z

src/Compilers/CSharp/Portable/CSharpCompilationOptions.cs

+        /// <summary>
+        /// Whether Nullable Reference Types feature is enabled globally.
+        /// </summary>
+        public bool? Nullable { get; private set; }


bool? [](start = 15, length = 5)

Why use a nullable bool that defaults to null, rather than a bool with default false? (like AllowUnsafe above) #Resolved

Why use a nullable bool that defaults to null, rather than a bool with default false? (like AllowUnsafe above)

This just preserves what we have today, the context can be in three different states. If we think that was not intentional or should change, that can be addressed later.

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

Seems a PROTOTYPE comment then.

In reply to: 224930060 [](ancestors = 224930060,224928022)

Seems a PROTOTYPE comment then.

I'll open an issue for that

In reply to: 224934637 [](ancestors = 224934637,224930060,224928022)

jcouv · 2018-10-12T22:14:38Z

src/Compilers/CSharp/Portable/PublicAPI.Shipped.txt

 Microsoft.CodeAnalysis.CSharp.CSharpCompilation.WithScriptCompilationInfo(Microsoft.CodeAnalysis.CSharp.CSharpScriptCompilationInfo info) -> Microsoft.CodeAnalysis.CSharp.CSharpCompilation
 Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions
 Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.AllowUnsafe.get -> bool
-Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.CSharpCompilationOptions(Microsoft.CodeAnalysis.OutputKind outputKind, bool reportSuppressedDiagnostics = false, string moduleName = null, string mainTypeName = null, string scriptClassName = null, System.Collections.Generic.IEnumerable<string> usings = null, Microsoft.CodeAnalysis.OptimizationLevel optimizationLevel = Microsoft.CodeAnalysis.OptimizationLevel.Debug, bool checkOverflow = false, bool allowUnsafe = false, string cryptoKeyContainer = null, string cryptoKeyFile = null, System.Collections.Immutable.ImmutableArray<byte> cryptoPublicKey = default(System.Collections.Immutable.ImmutableArray<byte>), bool? delaySign = null, Microsoft.CodeAnalysis.Platform platform = Microsoft.CodeAnalysis.Platform.AnyCpu, Microsoft.CodeAnalysis.ReportDiagnostic generalDiagnosticOption = Microsoft.CodeAnalysis.ReportDiagnostic.Default, int warningLevel = 4, System.Collections.Generic.IEnumerable<System.Collections.Generic.KeyValuePair<string, Microsoft.CodeAnalysis.ReportDiagnostic>> specificDiagnosticOptions = null, bool concurrentBuild = true, bool deterministic = false, Microsoft.CodeAnalysis.XmlReferenceResolver xmlReferenceResolver = null, Microsoft.CodeAnalysis.SourceReferenceResolver sourceReferenceResolver = null, Microsoft.CodeAnalysis.MetadataReferenceResolver metadataReferenceResolver = null, Microsoft.CodeAnalysis.AssemblyIdentityComparer assemblyIdentityComparer = null, Microsoft.CodeAnalysis.StrongNameProvider strongNameProvider = null, bool publicSign = false, Microsoft.CodeAnalysis.MetadataImportOptions metadataImportOptions = Microsoft.CodeAnalysis.MetadataImportOptions.Public) -> void


CSharpCompilationOptions [](start = 55, length = 24)

FYI @jaredpar @gafter for public API change.

FYI @jaredpar @gafter for public API change.

I believe, this is just a removal of optional values because a new (more complete) overload is added.

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

Yes, I understand. We're supposed to ping them on any PR with public API change (whether shipped or unshipped).

In reply to: 224930250 [](ancestors = 224930250,224928238)

jcouv · 2018-10-12T22:21:18Z

src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs

+            parsedArgs = DefaultParse(new[] { "a.cs", "/langversion:7.3" }, _baseDirectory);
+            parsedArgs.Errors.Verify();
+            Assert.Null(parsedArgs.CompilationOptions.Nullable);
+        }


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

If I run csc.exe /nullable+ /langversion:7.3 a.cs, would I get two diagnostics (one from parsing command-line options and one from compilation diagnostics), or just one? #Resolved

If I run csc.exe /nullable+ /langversion:7.3 a.cs, would I get two diagnostics (one from parsing command-line options and one from compilation diagnostics), or just one?

Just the one reported by the command line parser. Note that both places report errors originating from options.

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

jcouv · 2018-10-12T22:25:24Z

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

@@ -32837,11 +32850,9 @@ static void G6<T, U>() where T : U

            // No warnings with C#7.3.


// No warnings with C#7.3. [](start = 12, length = 26)

Stale comment? #Resolved

Stale comment?

I have no idea what this comment is about, there always were warnings for this scenario. Perhaps it refers to WRN_NullAsNonNullable warnings

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

jcouv · 2018-10-12T22:27:08Z

            //Diagnostic(ErrorCode.ERR_NonNullTypesNotAvailable, "System.Runtime.CompilerServices.NonNullTypes").WithArguments("8.0").WithLocation(10, 2)

I think the #nonnull pragma should also be disallowed with older LangVersion, for same reason we disallow global option. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:284 in 33f5934. [](commit_id = 33f5934, deletion_comment = False)

333fred · 2018-10-12T22:28:39Z

src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs

+
+                        case "nullable-":
+                            if (value != null)
+                                break;


Nit: surround with braces to match file. #Resolved

Nit: surround with braces to match file.

Matches the implementation for "checked" and I believe does not violate the style guidelines.

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

AlekseyTs · 2018-10-12T22:31:40Z

            //Diagnostic(ErrorCode.ERR_NonNullTypesNotAvailable, "System.Runtime.CompilerServices.NonNullTypes").WithArguments("8.0").WithLocation(10, 2)

I think the #nonnull pragma should also be disallowed with older LangVersion, for same reason we disallow global option.

Agree. Hence the PROTOTYPE comment above. This warning is not in scope for this PR because it is not focused on parsing the new directive.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:284 in 33f5934. [](commit_id = 33f5934, deletion_comment = False)

jcouv · 2018-10-12T22:51:50Z

    public void SuppressionOnUnconstrainedTypeParameter()

~~This test should use NonNullTypes=null context. No warning should be produced if the context is true or false.~~

Never mindd, I misread. This test behaves correctly. #Closed

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:330 in 33f5934. [](commit_id = 33f5934, deletion_comment = False)

jcouv · 2018-10-12T22:52:07Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

        {
-            return (options ?? TestOptions.ReleaseDll).WithSpecificDiagnosticOptions(
-                new[] { new System.Collections.Generic.KeyValuePair<string, ReportDiagnostic>("CS" + ((int)ErrorCode.WRN_PragmaNonNullTypes).ToString("0000"), ReportDiagnostic.Suppress) });
+            return (options ?? TestOptions.ReleaseDll).WithNullable(false);


WithNullable(false) [](start = 55, length = 19)

Do we have any tests that exercise the option null? #Closed

~~We should change Nullable to a bool so we don't need to test that case.~~
Nevermind, I guess allowing null is a transitional state.

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

Do we have any tests that exercise the option null?

Yes. Those are all the tests that didn't apply NonNullTypes attribute on a module.

In reply to: 224933817 [](ancestors = 224933817,224933444)

Any test that doesn't call WithNonNullTypesTrue/False will use null. So the case should be covered.
Closing

In reply to: 224933817 [](ancestors = 224933817,224933444)

jcouv

LGTM but we need to follow-up on finalize public API shape for new option. Thanks

Add "Nullable" compilation option.

33f5934

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Oct 12, 2018

AlekseyTs requested review from a team, 333fred, cston and jcouv October 12, 2018 18:23

jcouv reviewed Oct 12, 2018

View reviewed changes

333fred reviewed Oct 12, 2018

View reviewed changes

333fred approved these changes Oct 12, 2018

View reviewed changes

jcouv reviewed Oct 12, 2018

View reviewed changes

jcouv approved these changes Oct 12, 2018

View reviewed changes

AlekseyTs merged commit 93af170 into dotnet:features/NullableReferenceTypes Oct 12, 2018

jcouv mentioned this pull request Oct 18, 2018

Add NullableReferenceTypes msbuild property #30574

Merged

cston mentioned this pull request Nov 5, 2018

[Work items] Nullable reference types #22152

Closed

		@@ -32837,11 +32850,9 @@ static void G6<T, U>() where T : U

		// No warnings with C#7.3.

Conversation

AlekseyTs commented Oct 12, 2018

Uh oh!

AlekseyTs commented Oct 12, 2018

Uh oh!

jcouv Oct 12, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 12, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 12, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv commented Oct 12, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Oct 12, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

jcouv Oct 12, 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 Oct 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcouv Oct 12, 2018 •

edited by AlekseyTs

Loading

jcouv Oct 12, 2018 •

edited by AlekseyTs

Loading

jcouv Oct 12, 2018 •

edited by AlekseyTs

Loading

jcouv commented Oct 12, 2018 •

edited by AlekseyTs

Loading

333fred Oct 12, 2018 •

edited by AlekseyTs

Loading

jcouv commented Oct 12, 2018 •

edited

Loading

jcouv Oct 12, 2018 •

edited

Loading

cston Oct 12, 2018 •

edited

Loading