Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Mar 21, 2017
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fb38c5
Responded to PR comments on #17894
Mar 17, 2017
bfebeaa
Added more tests, and supporting 1.0, 2.0, 3.0, etc... versions
Mar 20, 2017
7e22339
Clean up
Mar 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/compilers/CSharp/Compiler Breaking Changes - post VS2017.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ In C# 7, the compiler accepted a pattern of the form `dynamic identifier`, e.g.

- https://github.com/dotnet/roslyn/issues/17674 In C# 7, the compiler accepted an assignment statement of the form `_ = M();` where M is a `void` method. The compiler now rejects that.

- https://github.com/dotnet/roslyn/issues/17173 Before C# 7.1, `csc` would accept leading zeroes in the `/langversion` option. Now it should reject it. Example: `csc.exe source.cs /langversion:07`.
25 changes: 17 additions & 8 deletions src/Compilers/CSharp/Portable/CSharpResources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 11 additions & 8 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4330,25 +4330,25 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>lambda expression</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion1" xml:space="preserve">
<value>Feature '{0}' is not available in C# 1. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 1. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion2" xml:space="preserve">
<value>Feature '{0}' is not available in C# 2. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 2. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion3" xml:space="preserve">
<value>Feature '{0}' is not available in C# 3. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 3. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion4" xml:space="preserve">
<value>Feature '{0}' is not available in C# 4. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 4. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion5" xml:space="preserve">
<value>Feature '{0}' is not available in C# 5. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 5. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion6" xml:space="preserve">
<value>Feature '{0}' is not available in C# 6. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 6. Please use language version {1} or greater.</value>
</data>
<data name="ERR_FeatureNotAvailableInVersion7" xml:space="preserve">
<value>Feature '{0}' is not available in C# 7. Please use language version {1} or greater.</value>
<value>Feature '{0}' is not available in C# 7. Please use language version {1} or greater.</value>
Copy link
Member

@jaredpar jaredpar Mar 20, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we remove all of the spaces here? Seems unrelated to this PR. #Resolved

Copy link
Member

@jcouv jcouv Mar 20, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a follow-up on the last PR. ERR_FeatureNotAvailableInVersion7_1 was added and it had this extra space. Then Omar pointed out all the previous versions had the same typo. #Resolved

</data>
<data name="ERR_FeatureIsExperimental" xml:space="preserve">
<value>Feature '{0}' is experimental and unsupported; use '/features:{1}' to enable.</value>
Expand Down Expand Up @@ -5033,7 +5033,10 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<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>
<value>Feature '{0}' is not available in C# 7.1. Please use language version {1} or greater.</value>
</data>
<data name="ERR_LanguageVersionCannotHaveLeadingZeroes" xml:space="preserve">
<value>Specified language version '{0}' cannot have leading zeroes</value>
Copy link
Member

@jaredpar jaredpar Mar 20, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we discuss what happens with trailing 0s? Sorry if I missed that part of the email. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per last email from @gafter, we are accepting this as a breaking change, and putting it in the Post VS2017 breaking change markdown (edited in this PR).

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

Copy link
Member

@jaredpar jaredpar Mar 20, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but that covers only a single trailing 0. What happens when I enter "7.000"?

The code gives an error for leading zero but not a trailing zeros. Possibly a non-issue but want to make sure it was explicitly considered. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never accepted them in the first place, so this shouldn't be a breaking change. Am I missing something?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't about a breaking change, it's about symmetry.

/langversion:07
Error: leading zeros not allowed 
/langversion:7.00
Invalid language version

Seems like this could be a reasonable typo in the new point release world.

Not hard feedback. Just an item I want to make sure we've considered.

Copy link
Contributor Author

@OmarTawfik OmarTawfik Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We never accepted trailing zeroes, so that's not a breaking change. Am I missing something? #Resolved

Copy link
Contributor Author

@OmarTawfik OmarTawfik Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. My personal opinion is that everything eventually should be disallowed with the latter error message. This exception was done for the breaking change. Will have to think about it.

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

</data>
<data name="ERR_VoidAssignment" xml:space="preserve">
<value>A value of type 'void' may not be assigned.</value>
Expand Down
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/CommandLine/CSharpCommandLineParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,6 +805,12 @@ internal sealed override CommandLineArguments CommonParse(IEnumerable<string> ar
{
AddDiagnostic(diagnostics, ErrorCode.ERR_SwitchNeedsString, MessageID.IDS_Text.Localize(), "/langversion:");
}
else if (value.StartsWith("0"))
Copy link
Member

@jaredpar jaredpar Mar 20, 2017 •
edited by OmarTawfik
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use an explicit comparer here. The default for StartsWith is to use culture sensitive comparisons and I'm unsure if that is correct here. Believe this should be ordinal. #Resolved

{
// This error was added in 7.1 to stop parsing versions as ints (behaviour in previous Roslyn compilers), and explicitly
// treat them as identifiers (behaviour in native compiler). This error helps users identify that breaking change.
AddDiagnostic(diagnostics, ErrorCode.ERR_LanguageVersionCannotHaveLeadingZeroes, value);
}
else if (!value.TryParse(out languageVersion))
{
AddDiagnostic(diagnostics, ErrorCode.ERR_BadCompatMode, value);
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1470,5 +1470,6 @@ internal enum ErrorCode
ERR_Merge_conflict_marker_encountered = 8300,
ERR_InvalidPreprocessingSymbol = 8301,
ERR_FeatureNotAvailableInVersion7_1 = 8302,
ERR_LanguageVersionCannotHaveLeadingZeroes = 8303,
}
}
48 changes: 34 additions & 14 deletions src/Compilers/CSharp/Portable/LanguageVersion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ public static string ToDisplayString(this LanguageVersion version)
}

/// <summary>
/// Parse a LanguageVersion from a string input, as the command-line compiler does.
/// Try parse a <see cref="LanguageVersion"/> from a string input, returning default if input was null.
/// </summary>
public static bool TryParse(this string version, out LanguageVersion result)
{
Expand All @@ -186,36 +186,56 @@ public static bool TryParse(this string version, out LanguageVersion result)

switch (version.ToLowerInvariant())
{
case "default":
result = LanguageVersion.Default;
return true;

case "latest":
result = LanguageVersion.Latest;
return true;

case "1":
case "1.0":
case "iso-1":
result = LanguageVersion.CSharp1;
return true;

case "2":
case "2.0":
case "iso-2":
result = LanguageVersion.CSharp2;
return true;

case "default":
result = LanguageVersion.Default;
case "3":
case "3.0":
result = LanguageVersion.CSharp3;
return true;

case "latest":
result = LanguageVersion.Latest;
case "4":
case "4.0":
result = LanguageVersion.CSharp4;
return true;

case "5":
case "5.0":
result = LanguageVersion.CSharp5;
return true;

case "6":
case "6.0":
result = LanguageVersion.CSharp6;
return true;

case "7":
case "7.0":
result = LanguageVersion.CSharp7;
return true;

case "7.1":
result = LanguageVersion.CSharp7_1;
return true;

default:
if (int.TryParse(version, NumberStyles.None, CultureInfo.InvariantCulture, out int versionNumber) && versionNumber <= 7)
{
result = (LanguageVersion)versionNumber;
if (result.IsValid())
{
return true;
}
}

result = LanguageVersion.Default;
return false;
}
Expand Down
Loading