-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for restore and safeonly for #nullable directive. #31806
Conversation
|
||
parsedArgs = DefaultParse(new[] { "a.cs", "/langversion:7.3" }, WorkingDirectory); | ||
parsedArgs.Errors.Verify(); | ||
Assert.False(parsedArgs.CompilationOptions.Nullable); | ||
Assert.Equal(NullableContextOptions.Disabled, parsedArgs.CompilationOptions.NullableContextOptions); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 8, length = 1)
Are we testing quotes and backslashes in the nullable:
value? #Resolved
@dotnet/roslyn-ide Please review keyword recommender changes |
@@ -334,21 +334,30 @@ public void RefOnly() | |||
} | |||
|
|||
[Fact] | |||
public void NullableReferenceTypes_True() | |||
public void NullableReferenceTypes_Enabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could make these tests a Theory
, and then have one test with the 3 options. #WontFix
{ | ||
var csc = new Csc(); | ||
csc.Sources = MSBuildUtil.CreateTaskItems("test.cs"); | ||
csc.NullableReferenceTypes = true; | ||
Assert.Equal("/nullable+ /out:test.exe test.cs", csc.GenerateResponseFileContents()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since nullable+
and nullable-
are still valid options, don't we want to keep these tests as well? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@@ -322,20 +322,52 @@ public new CSharpCommandLineArguments Parse(IEnumerable<string> args, string bas | |||
continue; | |||
|
|||
case "nullable": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
} | ||
continue; | ||
|
||
|
||
case "nullable+": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case "nullable+": [](start = 24, length = 17)
Should we remove /nullable+
and /nullable-
now that we have /nullable:enabled
and /nullable:disabled
? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using an obsolete API here? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, you're using Obsolete
to enforce calling these two instead.
In reply to: 242338210 [](ancestors = 242338210)
|
||
namespace Microsoft.CodeAnalysis.CSharp.Syntax | ||
{ | ||
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 3)
#pragma warning restore CS0618 | ||
} | ||
|
||
private void ReportSafetyDiagnostic(ErrorCode errorCode, SyntaxNode syntaxNode, params object[] arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReportSafetyDiagnostic [](start = 21, length = 22)
Nice. Thanks! #Resolved
What is the benefit of this behavior? Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs:1380 in 6e8106c. [](commit_id = 6e8106c, deletion_comment = False) |
parsedArgs.Errors.Verify(); | ||
Assert.Equal(NullableContextOptions.Disabled, parsedArgs.CompilationOptions.NullableContextOptions); | ||
|
||
parsedArgs = DefaultParse(new[] { @"/nullable-", @"/nullable:enabled", "/langversion:8", "a.cs" }, WorkingDirectory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@"/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
TestRoundTripping(node, text); | ||
VerifyErrorCode(node); // no errors | ||
VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.NullableDirectiveTrivia, Status = NodeStatus.IsActive, Text = "safeonly" }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 8, length = 1)
Do you want to update AllInOneCSharpCode too? (the test/resource with every syntax) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be faster to update that test than to file an issue (I don't think we have one) ;-) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -411,12 +411,22 @@ public struct AsyncIteratorMethodBuilder | |||
|
|||
protected static CSharpCompilationOptions WithNonNullTypesTrue(CSharpCompilationOptions options = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True [](start = 66, length = 4)
nit: Consider renaming "True" to "Enabled" and "False" to "Disabled"
First, this is an existing behavior for "enable" and I do not see a reason for "safeonly" to differ. 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM including IDE change. Thanks (iteration 4)
Please update the speclet too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I see you've update the speclet in new iteration. Thanks!
/// <summary> | ||
/// Nullable annotation and warning contexts are enabled. | ||
/// </summary> | ||
Enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
It would have been good to record the Milestone on this PR. I'm trying to find out which preview included this change. |
Found it. I think this was preview2. |
Related to #31130.