Add "#error version" to C# and udpate UpgradeProject to support 7.1 #18045

jcouv · 2017-03-22T02:26:16Z

This is a re-worked version of PR #17957. In particular, it introduces no new syntax nodes.

This change solves two problems:

print out the compiler version as a diagnostic for troubleshooting purpose (with #error version)
generate a "please use version 7.1" diagnostic even when the compiler supports no 7.1 features yet (with #error version:7.1)

This is only supported in C# for now. I can file a separate issue for VB if needed.

Adds ability to print the compiler version as diagnostic (#17859)
Updates the UpgradeProject fixer as follow-up on #17894

@gafter @cston @tmat @CyrusNajmabadi for review. Thanks

jcouv · 2017-03-22T02:27:04Z

.../Source/CompilerGeneratorTools/Source/VisualBasicSyntaxGenerator/XML/ParseTreeDescription.vb

@@ -138,7 +138,7 @@ Public Class ParseTree
            Return Enumerations(enumString)
        End If

-        ReportError(referencingElement, "{0} is not a valid field type", enumString)
+        ReportError(referencingElement, "{0} is not a valid field type. You should add a node-kind entry in the syntax.xml.", enumString)


This fixes an annoyance I ran into when working on the original PR.

jcouv · 2017-03-22T02:27:10Z

src/EditorFeatures/TestUtilities/Diagnostics/AbstractUserDiagnosticTest.cs

@@ -69,7 +70,9 @@ protected bool TryGetDocumentAndSelectSpan(TestWorkspace workspace, out Document

        protected Document GetDocumentAndAnnotatedSpan(TestWorkspace workspace, out string annotation, out TextSpan span)
        {
-            var hostDocument = workspace.Documents.Single(d => d.AnnotatedSpans.Any());
+            var annotatedDocuments = workspace.Documents.Where(d => d.AnnotatedSpans.Any());
+            Debug.Assert(!annotatedDocuments.IsEmpty(), "No annotated span found");


This fixes an annoyance I ran into when working on the original PR.

jcouv · 2017-03-22T15:26:31Z

@dotnet-bot test windows_debug_unit64_prtest please

cston · 2017-03-22T18:52:03Z

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

+                        eod = this.AddError(eod, triviaOffset, triviaWidth, ErrorCode.ERR_CompilerAndLanguageVersion, version,
+                            this.Options.SpecifiedLanguageVersion.ToDisplayString());
+                    }
+                    else if (LanguageVersionFacts.TryParse(errorText, out var languageVersion))


Perhaps #error version:<version> rather than simply #error <version>.

cston · 2017-03-22T18:52:37Z

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

+        private static string GetExpectedVersion()
+        {
+            string fileVersion = typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute<AssemblyFileVersionAttribute>().Version;
+            string hash = CommonCompiler.ExtractShortCommitHash(typeof(CSharpCompiler).GetTypeInfo().Assembly.GetCustomAttribute<CommitHashAttribute>().Hash);


Consider extracting local for typeof(CSharpCompiler).GetTypeInfo().Assembly.

cston · 2017-03-22T18:55:53Z

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

+                syntaxNodePredicate,
+                argumentOrderDoesNotMatter,
+                code.GetType());
+        }


Consider deriving class from TestBase rather than duplicating the method.

gafter · 2017-03-22T19:07:50Z

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

+                {
+                    if (errorText.Equals("version", StringComparison.Ordinal))
+                    {
+                        Assembly assembly = typeof(DirectiveParser).GetTypeInfo().Assembly;


I think this should be typeof(CSharpCompiler) to match the type used to get the assembly in the compiler. It might happen to be the same assembly today, but better not to make the code depend on that fact.

gafter · 2017-03-22T19:11:52Z

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

+                // (1,8): error CS1029: #error: 'version'
+                // #error version
+                Diagnostic(ErrorCode.ERR_ErrorDirective, "version").WithArguments("version").WithLocation(1, 8),
+                // (1,8): error CS8304: Compiler version: '42.42.42.42424 (<developer build>)'. Language version: '4'.


Language version '4' doesn't look right.

gafter · 2017-03-22T19:14:47Z

src/Features/CSharp/Portable/UpgradeProject/CSharpUpgradeProjectCodeFixProvider.cs

@@ -22,9 +22,10 @@ internal class CSharpUpgradeProjectCodeFixProvider : AbstractUpgradeProjectCodeF
        private const string CS8025 = nameof(CS8025); // error CS8025: Feature is not available in C# 4. Please use language version X or greater.
        private const string CS8026 = nameof(CS8026); // error CS8026: Feature is not available in C# 5. Please use language version X or greater.
        private const string CS8059 = nameof(CS8059); // error CS8059: Feature is not available in C# 6. Please use language version X or greater.
+        private const string CS8302 = nameof(CS8302); // error CS8302: Feature is not available in C# 7.0. Please use language version X or greater.


This will break when we compact the set of newly assigned error codes? Or are we committing to this particular number for the error now?

Yes this will need to be updated when we compact error codes.
When we change the error ID, the code fixer will not trigger an the UpgradeProjectFromCSharp7ToLatest and UpgradeProjectFromCSharp7ToCSharp7_1 will fail. Then we'll update the code fixer.

It looks like CS8302 is "... not available in C# 7.1." rather than 7.0.

gafter · 2017-03-22T19:59:42Z

(Done reviewing this revision)

cston · 2017-03-22T21:16:14Z

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

+            {
+                Kind = SyntaxKind.ErrorDirectiveTrivia,
+                Status = NodeStatus.IsActive,
+                Text = "7.1"


Text = "version:7.1"?

There must be a bug in the test method (the test passed). I'll investigate.

Indeed, the verification method was silently dropping some expectations :-S

jcouv · 2017-03-22T22:20:21Z

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

@@ -2560,7 +2591,7 @@ public void TestNegUndefWithBadTokensAfterName()
            var node = Parse(text);
            TestRoundTripping(node, text, false);
            VerifyErrorCode(node, (int)ErrorCode.ERR_EndOfPPLineExpected);
-            VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.UndefDirectiveTrivia, Status = NodeStatus.IsActive, Text = "FOO(" });
+            VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.UndefDirectiveTrivia, Status = NodeStatus.IsActive, Text = "FOO" });


Note: the paren is parsed as part of the EndOfDirective, not the Name.

jcouv · 2017-03-22T22:21:30Z

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

@@ -2584,7 +2615,7 @@ public void TestNegUndefWithBadNumericalName()
            var node = Parse(text);
            TestRoundTripping(node, text, false);
            VerifyErrorCode(node, (int)ErrorCode.ERR_IdentifierExpected);
-            VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.UndefDirectiveTrivia, Status = NodeStatus.IsActive, Text = "1234" });
+            VerifyDirectivesSpecial(node, new DirectiveInfo { Kind = SyntaxKind.UndefDirectiveTrivia, Status = NodeStatus.IsActive, Text = "" });


Note: the 1234 /// ... is parsed as part of EndOfDirective, not the Name.
This one does seem strange. Let me know if I should file a bug.

jcouv · 2017-03-22T22:55:06Z

@dotnet-bot test ubuntu_16_debug_prtest please

cston · 2017-03-22T23:08:47Z

LGTM

gafter · 2017-03-23T01:09:18Z

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

+                Diagnostic(ErrorCode.ERR_ErrorDirective, "version").WithArguments("version").WithLocation(1, 8),
+                // (1,8): error CS8304: Compiler version: '42.42.42.42424 (<developer build>)'. Language version: 4.
+                // #error version
+                Diagnostic(ErrorCode.ERR_CompilerAndLanguageVersion, "version").WithArguments(GetExpectedVersion(), "4").WithLocation(1, 8)


Why is this showing language version 4?

I thought you were talking about the formatting ('4' vs. 4) in your previous comment :-S

The reason is because Parse uses language version 4:

private CSharpParseOptions GetOptions(SourceCodeKind kind, string[] defines) { return new CSharpParseOptions(languageVersion: LanguageVersion.CSharp4, kind: kind, preprocessorSymbols: defines); }

CyrusNajmabadi · 2017-03-23T04:40:35Z

src/EditorFeatures/CSharpTest/Diagnostics/UpgradeProject/UpgradeProjectTests.cs

+@"
+class Program
+{
+#error [|version:7.1|]


sorry, i'm not getting what the user scenario is for this. Can you clarify? I did understand what the use of "#error version" was though.

Some of the bug reports we're getting are the result of using older versions of the compiler. When the compiler is hosted by a framework (asp.net, azure functions, CLI, etc) it will be convenient for troubleshooting to be able to check the compiler version with a source change. It's similar to "phpversion".

I get the reason for "#error version". I'm curious what the purposes of "#error version:number" is though...

Sorry, I had misread your comment.
When we do a point release, adding the new version is the first thing we do (as features depend on its existence). I have documented a checklist of what needs to be done to complete that first step. That includes fixing the UpgradeProject fixer. But there is a chicken and egg problem, because no feature will trigger the diagnostic requesting this new version.
That is the purpose of #error version:<version>.
This is not the only way the problem can be solved, but it is easy enough and convenient.

gafter

LGTM

gafter · 2017-03-23T20:47:58Z

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

+                // (1,8): error CS1029: #error: 'version:7.1'
+                // #error version:7.1
+                Diagnostic(ErrorCode.ERR_ErrorDirective, "version:7.1").WithArguments("version:7.1").WithLocation(1, 8),
+                // (1,8): error CS8302: Feature 'version' is not available in C# 7.1. Please use language version 7.1 or greater.


Should this instead say that feature 'version' is not available in C# 4? Because Parse uses version 4?

That makes sense and still fulfills the need. Fixed.
Thanks for the suggestion

jcouv · 2017-03-24T19:15:13Z

@dotnet-bot test windows_eta_open_prtest please

Add "#error version" to C# and udpate UpgradeProject to support 7.1

3c4893f

dnfclas added the cla-already-signed label Mar 22, 2017

jcouv commented Mar 22, 2017

View reviewed changes

jcouv assigned jcouv and gafter Mar 22, 2017

jcouv added this to the 15.3 milestone Mar 22, 2017

cston reviewed Mar 22, 2017

View reviewed changes

gafter reviewed Mar 22, 2017

View reviewed changes

PR feedback

5d81ef0

cston reviewed Mar 22, 2017

View reviewed changes

Addressing Chuck's feedback and fixing test harness bug

9da0117

jcouv commented Mar 22, 2017

View reviewed changes

gafter reviewed Mar 23, 2017

View reviewed changes

Add comment

0c04bef

CyrusNajmabadi reviewed Mar 23, 2017

View reviewed changes

gafter approved these changes Mar 23, 2017

View reviewed changes

Include the current language version in the diagnostic

da46683

gafter removed their assignment Mar 23, 2017

jcouv merged commit 31073d0 into dotnet:master Mar 24, 2017

jcouv deleted the upgrade-project branch March 24, 2017 20:01

jcouv mentioned this pull request Mar 24, 2017

Print compiler version in diagnostics when certain code is compiled #17859

Closed

jcouv mentioned this pull request Mar 27, 2017

Update UpgradeProject fixer to handle new LanguageVersion error code #17950

Closed

jcouv mentioned this pull request Apr 7, 2017

Support public, internal, private, static, virtual, sealed, abstract, extern and async modifiers with interface methods. #18519

Merged

jcouv mentioned this pull request Aug 2, 2017

Publish documentation for C# compiler features not covered in the C# language spec #411

Open

jcouv mentioned this pull request Aug 30, 2017

Unable to compile C# 7.1 code on VSTS build agents #21783

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "#error version" to C# and udpate UpgradeProject to support 7.1 #18045

Add "#error version" to C# and udpate UpgradeProject to support 7.1 #18045

jcouv commented Mar 22, 2017 •

edited

Loading

jcouv Mar 22, 2017

jcouv Mar 22, 2017

jcouv commented Mar 22, 2017

cston Mar 22, 2017

cston Mar 22, 2017

cston Mar 22, 2017

gafter Mar 22, 2017

gafter Mar 22, 2017

gafter Mar 22, 2017

jcouv Mar 22, 2017

cston Mar 22, 2017

gafter commented Mar 22, 2017

cston Mar 22, 2017 •

edited

Loading

jcouv Mar 22, 2017

jcouv Mar 22, 2017

jcouv Mar 22, 2017

jcouv Mar 22, 2017

jcouv commented Mar 22, 2017

cston commented Mar 22, 2017

gafter Mar 23, 2017

jcouv Mar 23, 2017

CyrusNajmabadi Mar 23, 2017

jcouv Mar 23, 2017

CyrusNajmabadi Mar 23, 2017

jcouv Mar 23, 2017

gafter left a comment

gafter Mar 23, 2017

jcouv Mar 23, 2017

jcouv commented Mar 24, 2017

Add "#error version" to C# and udpate UpgradeProject to support 7.1 #18045

Add "#error version" to C# and udpate UpgradeProject to support 7.1 #18045

Conversation

jcouv commented Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Mar 22, 2017

cston Mar 22, 2017 • edited 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 commented Mar 22, 2017

cston commented Mar 22, 2017

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

Choose a reason for hiding this comment

gafter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Mar 24, 2017

jcouv commented Mar 22, 2017 •

edited

Loading

cston Mar 22, 2017 •

edited

Loading