Add support for restore and safeonly for #nullable directive. by AlekseyTs · Pull Request #31806 · dotnet/roslyn

AlekseyTs · 2018-12-14T23:31:32Z

Related to #31130.

AlekseyTs · 2018-12-14T23:32:11Z

cston · 2018-12-14T23:50:54Z

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

            parsedArgs.Errors.Verify();
-            Assert.False(parsedArgs.CompilationOptions.Nullable);
+            Assert.Equal(NullableContextOptions.Disabled, parsedArgs.CompilationOptions.NullableContextOptions);
        }


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

Are we testing quotes and backslashes in the nullable: value? #Resolved

…ommenders.

AlekseyTs · 2018-12-15T00:24:55Z

@dotnet/roslyn-ide Please review keyword recommender changes

AlekseyTs · 2018-12-17T17:47:59Z

@cston, @jcouv, @333fred Please review.

333fred · 2018-12-17T21:51:30Z

src/Compilers/Core/MSBuildTaskTests/CscTests.cs


        [Fact]
-        public void NullableReferenceTypes_True()
+        public void NullableReferenceTypes_Enabled()


Nit: could make these tests a Theory, and then have one test with the 3 options. #WontFix

333fred · 2018-12-17T22:03:27Z

src/Compilers/Core/MSBuildTaskTests/CscTests.cs

            var csc = new Csc();
            csc.Sources = MSBuildUtil.CreateTaskItems("test.cs");
-            csc.NullableReferenceTypes = true;
-            Assert.Equal("/nullable+ /out:test.exe test.cs", csc.GenerateResponseFileContents());


Since nullable+ and nullable- are still valid options, don't we want to keep these tests as well? #Resolved

Since nullable+ and nullable- are still valid options, don't we want to keep these tests as well?

They are valid, but never used by an msbuild task.

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

jcouv · 2018-12-17T22:03:55Z

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

@@ -322,20 +322,52 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
                            continue;

                        case "nullable":


case "nullable": [](start = 24, length = 16)

Please update the speclet with this information about command-line and MSBuild options.

~~Also we should update the help message (filing an issue for that one seems fine if you prefer).~~ Never mind, you did already ;-) #Resolved

jcouv · 2018-12-17T22:05:33Z

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

+                            continue;
+
+
                        case "nullable+":


case "nullable+": [](start = 24, length = 17)

Should we remove /nullable+ and /nullable- now that we have /nullable:enabled and /nullable:disabled? #Closed

Should we remove /nullable+ and /nullable- now that we have /nullable:enabled and /nullable:disabled?

We don't have to. There is similar situation with "debug" switch

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

333fred · 2018-12-17T22:19:50Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+            Debug.Assert(!ErrorFacts.NullableFlowAnalysisSafetyWarnings.Contains(MessageProvider.Instance.GetIdForErrorCode((int)errorCode)));
+            Debug.Assert(ErrorFacts.NullableFlowAnalysisNonSafetyWarnings.Contains(MessageProvider.Instance.GetIdForErrorCode((int)errorCode)));
+#pragma warning disable CS0618
+            ReportDiagnostic(errorCode, syntax);


Why are we using an obsolete API here? #Closed

Oh I see, you're using Obsolete to enforce calling these two instead.

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

333fred · 2018-12-17T22:24:06Z

src/Compilers/CSharp/Portable/Syntax/NullableDirectiveMap.cs

    internal sealed class NullableDirectiveMap
    {
-        private static readonly NullableDirectiveMap Empty = new NullableDirectiveMap(ImmutableArray<(int Position, bool State)>.Empty);
+        private static readonly NullableDirectiveMap Empty = new NullableDirectiveMap(ImmutableArray<(int Position, bool? State)>.Empty);


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

Consider using an enum instead of a bool? so we don't have future confusion as to which state maps to which part of the bool. #WontFix

333fred

LGTM (commit 3)

jcouv · 2018-12-17T22:39:56Z

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

+#pragma warning restore CS0618
+        }
+
+        private void ReportSafetyDiagnostic(ErrorCode errorCode, SyntaxNode syntaxNode, params object[] arguments)


ReportSafetyDiagnostic [](start = 21, length = 22)

Nice. Thanks! #Resolved

jcouv · 2018-12-17T23:14:17Z

        comp.VerifyDiagnostics();

What is the benefit of this behavior?
I'd be tempted to always warn if you turn on nullability with an older language version, regardless of presence of trees.
#Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:1380 in 6e8106c. [](commit_id = 6e8106c, deletion_comment = False)

jcouv · 2018-12-17T23:18:35Z

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

+            parsedArgs.Errors.Verify();
+            Assert.Equal(NullableContextOptions.Disabled, parsedArgs.CompilationOptions.NullableContextOptions);
+
+            parsedArgs = DefaultParse(new[] { @"/nullable-", @"/nullable:enabled", "/langversion:8", "a.cs" }, WorkingDirectory);


@"/nullable-", @"/nullable:enabled" [](start = 45, length = 36)

I found this surprising, but I see this is standard handling for duplicate options (last one wins, no warning). #ByDesign

jcouv · 2018-12-17T23:22:58Z

src/Compilers/CSharp/Test/Syntax/LexicalAndXml/PreprocessorTests.cs

+            TestRoundTripping(node, text);
+            VerifyErrorCode(node); // no errors
+            VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.NullableDirectiveTrivia, Status = NodeStatus.IsActive, Text = "safeonly" });
+        }


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

Do you want to update AllInOneCSharpCode too? (the test/resource with every syntax) #Resolved

Do you want to update AllInOneCSharpCode too? (the test/resource with every syntax)

No I don't. Unless we already have an issue to update that file for Nullable Reference Types feature, I'll create one.

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

I think it'll be faster to update that test than to file an issue (I don't think we have one) ;-) #Resolved

I think it'll be faster to update that test than to file an issue (I don't think we have one) ;-)

Opened #31882. I am pretty sure that was faster.

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

jcouv · 2018-12-17T23:24:20Z

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

@@ -411,12 +411,22 @@ public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter

        protected static CSharpCompilationOptions WithNonNullTypesTrue(CSharpCompilationOptions options = null)


True [](start = 66, length = 4)

nit: Consider renaming "True" to "Enabled" and "False" to "Disabled"

AlekseyTs · 2018-12-17T23:26:14Z

        comp.VerifyDiagnostics();

What is the benefit of this behavior?
I'd be tempted to always warn if you turn on nullability with an older language version, regardless of presence of trees.

First, this is an existing behavior for "enable" and I do not see a reason for "safeonly" to differ.
Second, the language version on the compilation is always set to LanguageVersion.CSharp7 if there are no syntax trees, it doesn't matter what parse options are passed into the CreateCompilation helper. Since, nullable context doesn't matter in case when there are no trees, it really not useful to complain about the language version because there is no way to set the language version.

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

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:1380 in 6e8106c. [](commit_id = 6e8106c, deletion_comment = False)

jcouv

LGTM including IDE change. Thanks (iteration 4)
Please update the speclet too

jcouv

Oops, I see you've update the speclet in new iteration. Thanks!

jcouv · 2018-12-18T05:39:47Z

src/Compilers/CSharp/Portable/NullableContextOptions.cs

+        /// <summary>
+        /// Nullable annotation and warning contexts are enabled.
+        /// </summary>
+        Enabled,


The msbuild property and the command-line and the #nullable directive accept "enable", "disable" and "safeonly". But this enum has "Enabled" and "Disabled".
I think the enum should also use "Enable" and "Disable". #Resolved

jcouv · 2019-02-24T19:56:18Z

It would have been good to record the Milestone on this PR. I'm trying to find out which preview included this change.

jcouv · 2019-02-24T20:02:42Z

Found it. I think this was preview2.

AlekseyTs added 2 commits December 14, 2018 15:19

Add support for restore and safeonly for #nullable directive.

b11b683

Related to dotnet#31130.

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

1ef88b2

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

AlekseyTs requested review from 333fred, cston and jcouv December 14, 2018 23:31

AlekseyTs requested review from a team as code owners December 14, 2018 23:31

cston reviewed Dec 14, 2018

View reviewed changes

Add test scenarios. Add SafeOnlyKeywordRecommender to the list of rec…

6e8106c

…ommenders.

cston mentioned this pull request Dec 17, 2018

[Work items] Nullable reference types #22152

Closed

333fred reviewed Dec 17, 2018

View reviewed changes

jcouv reviewed Dec 17, 2018

View reviewed changes

333fred reviewed Dec 17, 2018

View reviewed changes

333fred approved these changes Dec 17, 2018

View reviewed changes

jcouv reviewed Dec 17, 2018

View reviewed changes

PR feedback

60328bc

jcouv reviewed Dec 17, 2018

View reviewed changes

jcouv approved these changes Dec 17, 2018

View reviewed changes

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

e892209

jcouv self-assigned this Dec 18, 2018

AlekseyTs merged commit 42a8e77 into dotnet:master Dec 18, 2018

jcouv reviewed Dec 18, 2018

View reviewed changes

jcouv mentioned this pull request Dec 20, 2018

Need completion and indentation for #nullable #31130

Closed

3 tasks

jcouv added this to the 16.0.P2 milestone Feb 24, 2019

		@@ -322,20 +322,52 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
		continue;

		case "nullable":

		@@ -411,12 +411,22 @@ public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter

		protected static CSharpCompilationOptions WithNonNullTypesTrue(CSharpCompilationOptions options = null)

Conversation

AlekseyTs commented Dec 14, 2018

Uh oh!

AlekseyTs commented Dec 14, 2018

Uh oh!

cston Dec 14, 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!

AlekseyTs commented Dec 15, 2018

Uh oh!

AlekseyTs commented Dec 17, 2018

Uh oh!

333fred Dec 17, 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!

333fred Dec 17, 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!

AlekseyTs Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 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!

jcouv Dec 17, 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!

AlekseyTs Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

333fred Dec 17, 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 Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

333fred Dec 17, 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!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 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 commented Dec 17, 2018 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Dec 17, 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 Dec 17, 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!

AlekseyTs Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 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!

AlekseyTs Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 17, 2018

Uh oh!

jcouv left a comment

cston Dec 14, 2018 •

edited by AlekseyTs

Loading

333fred Dec 17, 2018 •

edited by AlekseyTs

Loading

333fred Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 17, 2018 •

edited

Loading

333fred Dec 17, 2018 •

edited

Loading

333fred Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 17, 2018 •

edited

Loading

jcouv commented Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 17, 2018 •

edited

Loading

jcouv Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 17, 2018 •

edited by AlekseyTs

Loading

jcouv Dec 18, 2018 •

edited by AlekseyTs

Loading