Add support for enable/safeonly/nullable within #pragma directive #32208

AlekseyTs · 2019-01-07T03:56:10Z

Customer scenario
The spec the nullable reference types in C# 8.0 beta includes support for #pragma warning enable|safeonly nullable.

Bugs this fixes
Umbrella for nullable feature: #22152

Workarounds, if any
You could control/suppress warnings individually, but the "nullable" group has many warnings.

Risk
Performance impact
Low. This is a shorthand to suppress many warnings at once, which is already possible today (just quite long/verbose compared to this new syntax).

Is this a regression from a previous update?
No

Root cause analysis
Part of new C# 8.0 beta feature

How was the bug found?

Filed https://devdiv.visualstudio.com/DevDiv/_workitems/edit/763407 for shiproom purpose

The relevant portion of the spec https://github.com/dotnet/csharplang/blob/master/proposals/nullable-reference-types-specification.md is implemented.

AlekseyTs · 2019-01-07T17:47:09Z

@cston, @jcouv, @333fred Please review

jcouv · 2019-01-07T22:18:30Z

src/Compilers/CSharp/Portable/Errors/MessageID.cs

@@ -127,7 +127,7 @@ internal enum MessageID
        IDS_FeatureTuples = MessageBase + 12711,
        IDS_FeatureOutVar = MessageBase + 12713,

-        // IDS_FeatureIOperation = MessageBase + 12714,
+        IDS_FeaturePragmaWarningEnableOrSafeOnly = MessageBase + 12714,


IDS_FeaturePragmaWarningEnableOrSafeOnly [](start = 8, length = 40)

What's the benefit of using a discrete feature. Feels like IDS_FeatureNullableReferenceTypes would be good for this, since tightly related to nullability. #Resolved

What's the benefit of using a discrete feature. Feels like IDS_FeatureNullableReferenceTypes would be good for this, since tightly related to nullability.

The following code is not related to nullability, but is going to cause problems with legacy compiler.

#pragma warning enable

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

jcouv · 2019-01-07T22:28:45Z

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

@@ -22253,21 +22253,33 @@ public SyntaxToken DisableOrRestoreKeyword
      get { return new SyntaxToken(this, ((Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.PragmaWarningDirectiveTriviaSyntax)this.Green).disableOrRestoreKeyword, this.GetChildPosition(3), this.GetChildIndex(3)); }
    }

+    public SyntaxToken NullableKeyword 


NullableKeyword [](start = 23, length = 15)

Consider using a more generic name, such as Target or Group. Feels like we introduce more targets/groups in the future.

cston · 2019-01-07T22:40:43Z

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

@@ -461,64 +461,86 @@ private DirectiveTriviaSyntax ParsePragmaDirective(SyntaxToken hash, SyntaxToken
            {
                var warning = this.EatContextualToken(SyntaxKind.WarningKeyword);
                SyntaxToken style;
-                if (this.CurrentToken.Kind == SyntaxKind.DisableKeyword || this.CurrentToken.Kind == SyntaxKind.RestoreKeyword)
+                if (this.CurrentToken.Kind == SyntaxKind.DisableKeyword || this.CurrentToken.Kind == SyntaxKind.RestoreKeyword ||


if [](start = 16, length = 2)

Consider using switch (this.CurrentToken.Kind).

Consider using switch (this.CurrentToken.Kind).

I do not think the use of switch is warranted here. If you are concerned about multiple evaluations of this.CurrentToken.Kind property, I can cache its value in a local and reuse. Let me know.

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

jcouv · 2019-01-07T23:05:16Z

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

+        {
+            var source =
+@"
+#pragma warning disable nullable


Please update the nullable speclet for #pragma change.
It looks like previous #nullable change were also not captured. #WontFix

Please update the nullable speclet for #pragma change.
It looks like previous #nullable change were also not captured.

There is nothing to update in the speclet because it does not contain obsolete/invalid information. I see no good reason to reflect every implementation step in the speclet or duplicate specification in multiple places, implemented specification can be found here https://github.com/dotnet/csharplang/blob/master/proposals/nullable-reference-types-specification.md

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

I forgot that Mads covered that in his spec. That works indeed

In reply to: 245830615 [](ancestors = 245830615,245828755)

jcouv · 2019-01-07T23:09:35Z

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

+        {
+            var source =
+@"
+#pragma warning safeonly nullable


It's a language question, but this seems strange. I expected #pragma warning enable safeonly for this #Resolved

I see that's documented that way in Mads's spec. I'll shoot him an email, but that should not block the PR.

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

It's a language question, but this seems strange. I expected #pragma warning enable safeonly for this

There is a language spec for this here https://github.com/dotnet/csharplang/blob/master/proposals/nullable-reference-types-specification.md and I believe the relevant portion has been reviewed before cristmas.

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

jcouv · 2019-01-07T23:50:00Z

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

+                                directiveState = PragmaWarningState.Disabled;
+                                break;
+                            case SyntaxKind.RestoreKeyword:
+                                directiveState = PragmaWarningState.Default;


Allowing restore for error codes too is not covered in https://github.com/dotnet/csharplang/blob/master/proposals/nullable-reference-types-specification.md
Please confirm with Mads and get it added to the spec if appropriate.
#Resolved

Allowing restore for error codes too is not covered in https://github.com/dotnet/csharplang/blob/master/proposals/nullable-reference-types-specification.md
Please confirm with Mads and get it added to the spec if appropriate.

There is no change around restore, it was always supported. The added actions are enable and safeonly.

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

My bad, I thought we previously supported enable and disable.
Then my question should be on enable. What happens with #pragma warning enable 1234?

In reply to: 245838133 [](ancestors = 245838133,245837409)

Then my question should be on enable. What happens with #pragma warning enable 1234?

It enables the warning, this is also part of the spec

In reply to: 245839503 [](ancestors = 245839503,245838133,245837409)

jcouv

LGTM Thanks

cston · 2019-01-08T00:46:11Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

+#pragma warning safeonly 1695
+";
+            CreateCompilation(text, parseOptions: TestOptions.Regular7_3).VerifyDiagnostics(
+                // (3,17): warning CS1658: Feature 'warning action enable or safeonly' is not available in C# 7.3. Please use language version 8.0 or greater.. See also error CS8370.


.. [](start = 157, length = 2)

Extra . #Resolved

Extra .

Preexisting condition. This happens due to the way CS1658 is created. We have several instances in src\Compilers\CSharp\Test\Syntax\Parsing\ParserErrorMessageTests.cs already:

// (2,17): warning CS1658: Feature 'generics' is not available in C# 1. Please use language version 2 or greater.. See also error CS8022. // (2,16): warning CS1658: Feature 'namespace alias qualifier' is not available in C# 1. Please use language version 2 or greater.. See also error CS8022. // (3,16): warning CS1658: Feature 'namespace alias qualifier' is not available in C# 1. Please use language version 2 or greater.. See also error CS8022.

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

cston · 2019-01-08T00:46:28Z

src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs

+#pragma warning safeonly 1695
+";
+            CreateCompilation(text, parseOptions: TestOptions.Regular7_3).VerifyDiagnostics(
+                // (3,17): warning CS1658: Feature 'warning action enable or safeonly' is not available in C# 7.3. Please use language version 8.0 or greater.. See also error CS8370.


See also error CS8370. [](start = 160, length = 22)

What does this mean? #Resolved

What does this mean?

I assume this means that that error was changed to a warning and documentation for it should be looked at for more information about why the warning is reported. In this case it is a language version mismatch error converted to a warning

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

jaredpar · 2019-01-10T00:04:33Z

approved

Add support for enable/safeonly/nullable within #pragma directive

49fede9

AlekseyTs added Area-Compilers Feature - Nullable Reference Types Nullable Reference Types labels Jan 7, 2019

AlekseyTs requested review from 333fred, cston and jcouv January 7, 2019 03:56

AlekseyTs requested review from a team as code owners January 7, 2019 03:56

AlekseyTs changed the base branch from master to dev16.0-preview2 January 7, 2019 17:46

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

jcouv reviewed Jan 7, 2019

View reviewed changes

cston reviewed Jan 7, 2019

View reviewed changes

jcouv reviewed Jan 7, 2019

View reviewed changes

jcouv approved these changes Jan 7, 2019

View reviewed changes

jcouv self-assigned this Jan 7, 2019

cston reviewed Jan 8, 2019

View reviewed changes

cston approved these changes Jan 8, 2019

View reviewed changes

jcouv added the Needs Shiproom Approval label Jan 9, 2019

jaredpar added Approved to merge and removed Needs Shiproom Approval labels Jan 10, 2019

AlekseyTs merged commit f74016a into dotnet:dev16.0-preview2 Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for enable/safeonly/nullable within #pragma directive #32208

Add support for enable/safeonly/nullable within #pragma directive #32208

AlekseyTs commented Jan 7, 2019 •

edited by jcouv

Loading

AlekseyTs commented Jan 7, 2019

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Jan 7, 2019

jcouv Jan 7, 2019

cston Jan 7, 2019

AlekseyTs Jan 7, 2019

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Jan 7, 2019

jcouv Jan 7, 2019

jcouv Jan 7, 2019 •

edited

Loading

jcouv Jan 7, 2019

AlekseyTs Jan 7, 2019

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Jan 7, 2019

jcouv Jan 8, 2019

AlekseyTs Jan 8, 2019

jcouv left a comment

cston Jan 8, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Jan 8, 2019

cston Jan 8, 2019 •

edited by AlekseyTs

Loading

AlekseyTs Jan 8, 2019

jaredpar commented Jan 10, 2019

Add support for enable/safeonly/nullable within #pragma directive #32208

Add support for enable/safeonly/nullable within #pragma directive #32208

Conversation

AlekseyTs commented Jan 7, 2019 • edited by jcouv Loading

AlekseyTs commented Jan 7, 2019

jcouv Jan 7, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 7, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 7, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

cston Jan 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jan 8, 2019 • edited by AlekseyTs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar commented Jan 10, 2019

AlekseyTs commented Jan 7, 2019 •

edited by jcouv

Loading

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

jcouv Jan 7, 2019 •

edited

Loading

jcouv Jan 7, 2019 •

edited by AlekseyTs

Loading

cston Jan 8, 2019 •

edited by AlekseyTs

Loading

cston Jan 8, 2019 •

edited by AlekseyTs

Loading