Add C# 7.1 version to compiler #17894

OmarTawfik · 2017-03-15T21:14:30Z

cc @AlekseyTs @jcouv @dotnet/roslyn-compiler

Contributes to #17173

AlekseyTs

LGTM

cston · 2017-03-15T22:02:37Z

src/Compilers/CSharp/Portable/LanguageVersion.cs

-                    if (int.TryParse(version, NumberStyles.None, CultureInfo.InvariantCulture, out versionNumber) &&
-                        versionNumber <= 6 &&
-                        ((LanguageVersion)versionNumber).IsValid())
+                    if (int.TryParse(version, NumberStyles.None, CultureInfo.InvariantCulture, out int versionNumber) && versionNumber <= 7)


Do we now allow "07"? Consider reporting errors for leading zeros on any version number. #Resolved

@cston this wasn't introduced in this PR. In fact, I think RTM has this behaviour:
c:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\15.0\Bin\Roslyn\csc.exe" a.cs /langversion:05
If you think this should be an error, we can change it in a later PR, but I wanted to merge this to unblock @AlekseyTs. Do I've a sign off? #Resolved

It was a minor point. Previously we allowed 7 but not 07, now we allow 07 and 7.1 but not 07.1. #Resolved

I agree. The weird format like 05 should no longer be allowed, starting with version 7. #Resolved

jcouv · 2017-03-15T23:46:50Z

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

@@ -1355,7 +1357,7 @@ public void LanguageVersion_MapSpecifiedToEffectiveVersion()
            Assert.Equal(LanguageVersion.CSharp6, LanguageVersion.CSharp6.MapSpecifiedToEffectiveVersion());
            Assert.Equal(LanguageVersion.CSharp7, LanguageVersion.CSharp7.MapSpecifiedToEffectiveVersion());
            Assert.Equal(LanguageVersion.CSharp7, LanguageVersion.Default.MapSpecifiedToEffectiveVersion());
-            Assert.Equal(LanguageVersion.CSharp7, LanguageVersion.Latest.MapSpecifiedToEffectiveVersion());


You're missing a test for CSharp7_1 #Resolved

jcouv · 2017-03-15T23:50:05Z

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

@@ -1332,14 +1334,14 @@ public void LanguageVersionAdded_Canary()
            // - update the "UpgradeProject" codefixer


Did you update the UpgradeProject codefixer? We should add a diagnostic for "error CS80XX: Feature is not available in C# 7." and handle it in CSharpUpgradeProjectCodeFixProvider.
This also mean updating the method for checking feature availability. #ByDesign

Also, could you add a test for the LanguageVersionExtensionsInternal.GetErrorCode method. It should loop over Enum.GetValues(typeof(LanguageVersion)).Cast<LanguageVersion>() so that it breaks any time a new enum value is added.
#Resolved

@jcouv If I understand the fixer correctly, this will have to wait until any of the 7.1 features actually make it to master.
I'll log an item for it in the master issue.

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

Yes, I realized that too. But I came up with a solution, by leveraging another feature that's useful for point releases, the #version directive.

I have a PR almost ready: #17957

OmarTawfik · 2017-03-16T01:29:47Z

@jcouv had to merge this as soon as it got sign offs to unblock Aleksey. Will send another PR in the morning to cover the additional comments you left. #Resolved

jcouv · 2017-03-16T02:43:49Z

No problem.
Thanks #Resolved

jcouv · 2017-03-16T17:23:17Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -2690,7 +2690,7 @@ A catch() block after a catch (System.Exception e) block can catch non-CLS excep
    <value>This warning occurs if the assembly attributes AssemblyKeyFileAttribute or AssemblyKeyNameAttribute found in source conflict with the /keyfile or /keycontainer command line option or key file name or key container specified in the Project Properties.</value>
  </data>
  <data name="ERR_BadCompatMode" xml:space="preserve">
-    <value>Invalid option '{0}' for /langversion; must be ISO-1, ISO-2, Default or an integer in range 1 to 7.</value>
+    <value>Invalid option '{0}' for /langversion; must be ISO-1, ISO-2, Default, Latest or a valid version in range 1 to 7.1.</value>


I wonder if we should enumerate the valid versions in the message (1, 2, 3, 4, 5, 6, 7, 7.1) #ByDesign

Wouldn't it be too long? Consider in a couple of point releases:
Invalid option '{0}' for /langversion; must be ISO-1, ISO-2, Default, Latest, 1, 2, 3, 4, 5, 6, 7, 7,1, 7.2, 8.
I'd claim that people calling csc from command line already know what they're doing (and they don't need to know which C# versions were shipped), VS UI or first time users who would normally use VS project dialogs to enumerate available versions.

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

Ok. Let's leave as-is. #Resolved

jcouv

Added a couple suggestions.

jcouv · 2017-03-16T17:23:47Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5032,4 +5032,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_InvalidPreprocessingSymbol" xml:space="preserve">
    <value>Invalid name for a preprocessing symbol; '{0}' is not a valid identifier</value>
  </data>
+  <data name="ERR_FeatureNotAvailableInVersion7_1" xml:space="preserve">
+    <value>Feature '{0}' is not available in C# 7.1.  Please use language version {1} or greater.</value>


Nit: two spaces after 7.1. #Resolved

Paste issue from similar messages :) will fix all of them now.

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

jcouv · 2017-03-16T17:36:34Z

src/Compilers/CSharp/Portable/LanguageVersion.cs

@@ -212,6 +229,7 @@ public static LanguageVersion MapSpecifiedToEffectiveVersion(this LanguageVersio
            switch (version)
            {
                case LanguageVersion.Latest:
+                    return LanguageVersion.CSharp7_1;


I wonder if we could make this code invariant so it doesn't have to be updated with every point release. Maybe we just return the largest enum entry except for LanguageVersion.Latest. #Resolved

I think this file is very easy to edit. After the recent rectoring (by you), all our version work is done here, and adding a new version is simply editing 3~4 lines in that file, plus the required testing.

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

jcouv · 2017-03-16T19:55:14Z

This PR implements parts of #17173 #Resolved

jcouv · 2017-03-17T01:39:44Z

@OmarTawfik As a side note, I noticed that you merge your PRs, rather than squash them. I find that the history is more readable with squashed PRs (it avoids commits like "PR feedback", or "fix" making it into the github history). For your consideration :-) #Resolved

OmarTawfik · 2017-03-17T22:48:14Z

Thanks :)

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

* Responded to PR comments on #17894 * Added more tests, and supporting 1.0, 2.0, 3.0, etc... versions * Clean up

Add C# 7.1 version to compiler

fab31e5

OmarTawfik added the Area-Compilers label Mar 15, 2017

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

AlekseyTs approved these changes Mar 15, 2017

View reviewed changes

cston reviewed Mar 15, 2017

View reviewed changes

OmarTawfik merged commit 4b6bb0c into dotnet:master Mar 15, 2017

OmarTawfik deleted the fix-17173-add-7.1-to-master branch March 15, 2017 23:09

jcouv reviewed Mar 15, 2017

View reviewed changes

jcouv reviewed Mar 16, 2017

View reviewed changes

VSadov mentioned this pull request Mar 16, 2017

[Umbrella] Add LanguageVersion 7.1/15.3 and streamline process #17173

Closed

16 tasks

This was referenced Mar 17, 2017

Update UpgradeProject fixer to handle new LanguageVersion error code #17950

Closed

Add 7.1 support to UpgradeProject, add #version directive #17957

Closed

OmarTawfik mentioned this pull request Mar 20, 2017

Follow up on PR #17894 on 7.1 version parsing #17990

Merged

OmarTawfik added a commit that referenced this pull request Mar 21, 2017

Follow up on PR #17894 on 7.1 version parsing (#17990)

a20a6cb

* Responded to PR comments on #17894 * Added more tests, and supporting 1.0, 2.0, 3.0, etc... versions * Clean up

jcouv mentioned this pull request Mar 22, 2017

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add C# 7.1 version to compiler #17894

Add C# 7.1 version to compiler #17894

OmarTawfik commented Mar 15, 2017

AlekseyTs left a comment

cston Mar 15, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 15, 2017 •

edited

Loading

cston Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 20, 2017

jcouv Mar 20, 2017

OmarTawfik commented Mar 16, 2017 •

edited

Loading

jcouv commented Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 17, 2017

jcouv Mar 17, 2017 •

edited by OmarTawfik

Loading

jcouv left a comment

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 17, 2017

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 17, 2017

jcouv commented Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv commented Mar 17, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Mar 17, 2017

		@@ -1332,14 +1334,14 @@ public void LanguageVersionAdded_Canary()
		// - update the "UpgradeProject" codefixer

Add C# 7.1 version to compiler #17894

Add C# 7.1 version to compiler #17894

Conversation

OmarTawfik commented Mar 15, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

cston Mar 15, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

cston Mar 15, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv Mar 15, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv Mar 15, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv Mar 15, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv Mar 16, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 20, 2017

Choose a reason for hiding this comment

jcouv Mar 20, 2017

Choose a reason for hiding this comment

OmarTawfik commented Mar 16, 2017 • edited Loading

jcouv commented Mar 16, 2017 • edited by OmarTawfik Loading

jcouv Mar 16, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 17, 2017

Choose a reason for hiding this comment

jcouv Mar 17, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Mar 16, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 17, 2017

Choose a reason for hiding this comment

jcouv Mar 16, 2017 • edited by OmarTawfik Loading

Choose a reason for hiding this comment

OmarTawfik Mar 17, 2017

Choose a reason for hiding this comment

jcouv commented Mar 16, 2017 • edited by OmarTawfik Loading

jcouv commented Mar 17, 2017 • edited by OmarTawfik Loading

OmarTawfik commented Mar 17, 2017

cston Mar 15, 2017 •

edited by OmarTawfik

Loading

OmarTawfik Mar 15, 2017 •

edited

Loading

cston Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 15, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

OmarTawfik commented Mar 16, 2017 •

edited

Loading

jcouv commented Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 17, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv commented Mar 16, 2017 •

edited by OmarTawfik

Loading

jcouv commented Mar 17, 2017 •

edited by OmarTawfik

Loading