-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
LangVersion 9 #44911
LangVersion 9 #44911
Conversation
@dotnet/roslyn-compiler for review please |
('$(TargetFrameworkIdentifier)' == '.NETStandard' AND '$(_TargetFrameworkVersionWithoutV)' == '2.1')) AND | ||
'$(MaxSupportedLangVersion)' == ''">8.0</MaxSupportedLangVersion> | ||
|
||
<LangVersion Condition="'$(LangVersion)' == '' AND '$(MaxSupportedLangVersion)' != ''">$(MaxSupportedLangVersion)</LangVersion> |
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.
What happens when the target framework is .NET Framework? In that case I believe that $(MaxSupportedLangVersion)
will be empty.
Think the original code here had the right idea. If there is no $(MaxSupportedLangVersion)
calculated, that is if it isn't a target framework that we could positively identify, then fall back to 7.3 as the default. #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.
It'll be 7.3. We check if its either a) netcore < 3, b) netstandard < 2.1 or it's empty (all other frameworks)
Unless a user manually sets it, in which case all bets are off. The logic is confusing but should be the same just inverted (it made more sense to invert the first one as we need to for the second)
Hmm, I think you're right. I think my testing was consuming both the existing targets and the new ones, so we were still setting it to 7.3 for framework. Will fix. #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.
Why did you opt to change the original logic here? For me at least this is much harder to follow. #Resolved
can you update the IDE to remove our CSharp9 constant? it's currently here: Line 13 in 9b22e9c
Thanks! |
@@ -128,13 +128,13 @@ public enum LanguageVersion | |||
/// </summary> | |||
CSharp8 = 800, | |||
|
|||
// When this value is released, update LanguageVersionExtensions in the IDE layer to point to it. | |||
// When this value is available in the released NuGet packge, update LanguageVersionExtensions in the IDE layer to point to it. |
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 didn't understand this. We've made changes to compiler and IDE in single PRs, so I'm surprised that the IDE would be depending on nuget rather than just project references...
To my knowledge, there is one exception only, which is a code style assembly, which does not get to see the latest compiler APIs. #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.
So the CodeStyle package includes the analyzers shared project. That has an analyzer ConvertSwitchStatementToExpressionDiagnosticAnalyzer
that checks for CSharp9. Because CodeStyle is referencing from NuGet we don't have the language version yet.
We can either wait until it's published, or temporarily exclude that analyzer from CodeStyle? #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.
Doesn't have to be done in this PR, but we'll also want to change all references to version preview in tests (including IDE tests) to use version 9 instead. #Closed Refers to: src/Compilers/CSharp/Portable/LanguageVersion.cs:147 in 96eaf82. [](commit_id = 96eaf82, deletion_comment = False) |
@@ -353,6 +358,11 @@ public static bool TryParse(string? version, out LanguageVersion result) | |||
result = LanguageVersion.CSharp8; | |||
return true; | |||
|
|||
case "9": | |||
case "9.0": |
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.
Some tests should be updated for this.
Look at LanguageVersionAdded_Canary
and subsequent tests, which should fail when new version is added.
I believe that canary method has instructions. I'm not sure about the project-system one, which relates to displaying the new language version in the VS dropdown for Core projects.
It's also good to add a test for new version in UpgradeProject fixer tests in IDE. #Closed
I mentioned this in the description. we can't yet but there is a follow up ready to go that does |
I've flipped the logic around for this. For 16.7 we'll have a The IDE etc will still ask you to upgrade to |
<MaxSupportedLangVersion Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' < '5.0') AND | ||
('$(TargetFrameworkIdentifier)' != '.NETStandard' OR '$(_TargetFrameworkVersionWithoutV)' == '2.1') AND | ||
'$(MaxSupportedLangVersion)' == ''">8.0</MaxSupportedLangVersion> | ||
|
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.
We should add a section that makes 9.0 the default when targeting netcoreapp 5.0 #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.
Hmm, I was originally thinking we should just make the default language version be 9.0 given that we're now explicitly setting to 8 or less for anything other than .netcoreapp 5.0.
I guess we can temporarily default netcoreapp 5.0 to csharp 9, and then let it go back to default when we release? #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.
The way I think about it, at the MSBuild level, is that the default is 7.3. Yet there are specific TF well known TF where we will change the default to a different value. So net5
will always have the value 9, it will never have the default. #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.
Hmm, I guess I was thinking there can be multiple netcoreapp5 versions, like 5.1/5.2 etc that should also be C# 9, but given that we're switching to a yearly 5/6/7 cadence, I guess that'll no longer be true? At which point there is a single one-to-one mapping of supported tfm <=> langversion right? #Resolved
Rebase onto latest master. |
ping @dotnet/roslyn-compiler for review |
'$(MaxSupportedLangVersion)' == ''">8.0</MaxSupportedLangVersion> | ||
|
||
<!-- .NETCoreApp == 5.0 --> | ||
<MaxSupportedLangVersion Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND '$(_TargetFrameworkVersionWithoutV)' == '5.0' AND |
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.
Have u verified that these are the values that will be passed for net5.0
? Also what happens when we have the OS specific versions of the TF moniker like net5.0-ios13
? #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.
Yes. net5.0
will still have a target framework of .NETCoreApp
by design. the -ios13
stuff isn't happening in .net 5 timeline now, it got pushed back to net6. Ultimately though, that information will be mapped to different properties
In reply to: 440425405 [](ancestors = 440425405)
[InlineData(".NETStandard", "2.0", "7.3")] | ||
[InlineData(".NETStandard", "2.1", "8.0")] | ||
|
||
[InlineData("UnknownTFM", "0.0", "7.3")] |
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.
Add a case for
[InlineData("UnknownTFM", "5.0", "7.3")]
#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.
Good use of [Theory]
#Resolved
<LangVersion Condition="'$(LangVersion)' == ''">$(MaxSupportedLangVersion)</LangVersion> | ||
<PropertyGroup> | ||
<!-- .NETCoreApp < 3.0, .NETStandard < 2.1, or any other target framework --> | ||
<MaxSupportedLangVersion Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' < '3.0') AND |
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.
Consider making this an _
prefixed property to make it clear that it's a property we don't want users setting themselves. #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.
We don't actually need MaxSupportedLangVersion if we're not letting users set it, but i'm going to keep with an _
prefix to aid potential debugging of msbuild logs.
In reply to: 440427516 [](ancestors = 440427516)
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.
We were setting $(MaxSupportedLangVersion)
for Xamarin projects, to opt into C# 8.0 by default.
Should we use $(_MaxSupportedLangVersion)
now instead? (even though it is considered private)
Context: dotnet/android#5049 (comment)
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 aren't you all using the literal Latest
? It was meant for exactly this purpose: represent the latest RTM version of the language.
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.
It might not be a concern in practice, but in theory language features that require runtime support might not yet be implemented in the Mono runtime.
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.
Generally speaking, those features gracefully degrade and issue errors when used on unsupported runtimes. For example, if you attempt to use a platform-default unmanaged function pointer on a runtime that does not support it (does not include this api), the compiler will issue an error to state that it's not supported.
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.
To be clear, the Xamarin MSBuild targets should include this by default instead?
<LangVersion Condition="'$(LangVersion)' == ''">latest</LangVersion>
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.
It might not be a concern in practice, but in theory language features that require runtime support might not yet be implemented in the Mono runtime.
We don't flip to latest being the new C# version until basically right before we release. The flip to latest being 9 happened just a week a go. So this seems pretty unlikely given htat we coordinate the runtime changes.
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.
This prompted some good discussion that we're working through. The Mono runtime changes are done in master
, on the "netcore" version, originally intended for net5
, but bumped back to net6
. This isn't the version that android and iOS use today. So we will need to look at backporting those changes to the Mono branch we currently use, or figure out what our C#9 support story is.
XmlReader xmlReader = XmlReader.Create(new StringReader($@" | ||
<Project> | ||
<PropertyGroup> | ||
<MaxSupportedLangVersion>55.0</MaxSupportedLangVersion> |
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.
Would remove this test because we don't, or at least shouldn't, support the explicit setting of MaxSUpportedLangVersion
. #Resolved
[InlineData(".NETStandard", "2.1", "8.0")] | ||
|
||
[InlineData("UnknownTFM", "0.0", "7.3")] | ||
public void LanguageVersionGivenTargetFramework(string tfi, string tfv, string expectedVersion) |
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.
LanguageVersionGivenTargetFramework [](start = 20, length = 35)
Since this test needs to be updated when new language versions are added, please call the canary from this test, or adding some other mechanism so that we remember to update this test. #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.
Added a test that'll fail when the default language version changes, which should be a good enough proxy to catch this situation going forward.
In reply to: 443006233 [](ancestors = 443006233)
<MaxSupportedLangVersion Condition="'$(MaxSupportedLangVersion)' == ''">7.3</MaxSupportedLangVersion> | ||
<LangVersion Condition="'$(LangVersion)' == ''">$(MaxSupportedLangVersion)</LangVersion> | ||
<PropertyGroup> | ||
<!-- .NETCoreApp < 3.0, .NETStandard < 2.1, or any other target framework --> |
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.
or any other target framework [](start = 47, length = 30)
I didn't understand this part ("any other target framework") and how it matches the logic below
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 logic is kinda weird to follow, but this will catch everything except .NETCoreApp >= 3 and .NETStandard >= 2.1 , so all other target frameworks get set here. The following checks only kick in when _MaxSupportedLangVersion
isn't set so they won't cause it to change. I think we can probably simplify the logic below though.
In reply to: 443010616 [](ancestors = 443010616)
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.
Got it. Thanks
nit: consider inverting the condition with a top-level "not": not (.NETCoreApp >= 3 || .NETStandard >= 2.1)
In reply to: 445156354 [](ancestors = 445156354,443010616)
Ping @dotnet/roslyn-compiler for a second review please :) |
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 Thanks (iteration 14)
Filed #44962 to handle follow up work from this. |
<LangVersion Condition="'$(LangVersion)' == ''">$(MaxSupportedLangVersion)</LangVersion> | ||
<PropertyGroup> | ||
<!-- .NETCoreApp < 3.0, .NETStandard < 2.1, or any other target framework --> | ||
<_MaxSupportedLangVersion Condition="('$(TargetFrameworkIdentifier)' != '.NETCoreApp' OR '$(_TargetFrameworkVersionWithoutV)' < '3.0') AND |
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.
_TargetFrameworkVersionWithoutV [](start = 96, length = 31)
_TargetFrameworkVersionWithoutV
is a private property defined in .NET SDK. We shouldn't use private properties defined outside of Roslyn targets.
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 check implemented here is the one the SDK team recommended that we use. If there is a better property to use here we'd be happy to do so.
Note: @chsienki didn't add the use of this property here, his change just reorganized the existing checks and built off of the pattern.
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.
We can copy the definition of this property to our targets:
<_TargetFrameworkVersionWithoutV>$(TargetFrameworkVersion.TrimStart('vV'))</_TargetFrameworkVersionWithoutV>
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.
MSBuild is improving this. I've filed #46050 to track
* upstream/master: (1226 commits) Remove unnecessary Clone() (dotnet#45469) Align addition of a synthesized override of object.Equals(object? obj) in records with the latest design. (dotnet#45475) Move SymbolSearch down to EditorFeatures (dotnet#45505) VisitType in MethodToClassRewriter for function pointers. Fix up nondeterminism in serializing naming style preferences Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (dotnet#45482) Fix typo Move to vnext Add constant inerpolated strings to the test plan, update status for records. Don't emit ldftn when the result is unused. PR Feedback: * Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts. Add records to compiler test plan (dotnet#45434) Expand comment in CreateRecoverableText Replace binary serialization of encoding with a custom serializer. (dotnet#45374) LangVersion 9 (dotnet#44911) Avoid loading document text in AbstractObjectBrowserLibraryManager.DocumentChangedAsync Allow TryGetTextVersion to pass through to the initial source Ensure recoverable text is in temporary storage Fix test Updates the option page type GUID to match the one in pkgdef ...
…e_168 * upstream/master: (102 commits) Change contrast ratio to get close to 1.5:1 (#45526) Revert "Move SymbolSearch down to EditorFeatures (#45505)" Delay accessibility checks to avoid cycles (#45441) Prevent trying to convert metadata references into circular project references Remove unnecessary Clone() (#45469) Align addition of a synthesized override of object.Equals(object? obj) in records with the latest design. (#45475) Move SymbolSearch down to EditorFeatures (#45505) VisitType in MethodToClassRewriter for function pointers. Fix up nondeterminism in serializing naming style preferences Update dependencies from https://github.com/dotnet/arcade build 20200626.2 (#45482) Fix typo Move to vnext Add constant inerpolated strings to the test plan, update status for records. Don't emit ldftn when the result is unused. PR Feedback: * Additional tests for nested function contexts. * Override VisitFunctionPointerLoad in MethodToClassRewriter. * Adjust debug asserts. Add records to compiler test plan (#45434) Expand comment in CreateRecoverableText Replace binary serialization of encoding with a custom serializer. (#45374) LangVersion 9 (#44911) Avoid loading document text in AbstractObjectBrowserLibraryManager.DocumentChangedAsync ...
Fixes: dotnet#5049 Context: dotnet/roslyn@12f1b8f#diff-125e2d8c52dd9033f9b248f79f78c3f8 Context: dotnet/roslyn#44911 (comment) The `BuildTest.CSharp8Features` test was failing for me locally where I have Visual Studio 2019 16.7.2 installed: Foo.cs(1,23): error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater. After review, it appears that a change in Roslyn's MSBuild targets removed `$(MaxSupportedLangVersion)` and made the property private: `$(_MaxSupportedLangVersion)`. After some discussion around C# 9.0: * It is unknown if C# 9.0 will be supported by mono/mono/2020-02. Some features like covariant return types and function pointers require runtime changes. * Using a build of .NET 5 rc1, `$(LangVersion)` is already being set to `9.0` by default. We should just change the current behavior of: <MaxSupportedLangVersion Condition=" '$(MaxSupportedLangVersion)' == '' ">8.0</MaxSupportedLangVersion> And change it to: <LangVersion Condition=" '$(LangVersion)' == '' ">8.0</LangVersion> This matches what Roslyn's default behavior is: https://github.com/dotnet/roslyn/blob/1aeda2e94fb9371b96d4bfd94b074860064ec3d2/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L6-L21 .NET 5+ projects should be unaffected by this change, as they do not import `Xamarin.Android.CSharp.targets`. If at some point, mono/mono/2020-02 fully supports C# we can bump this value. Users can opt into `9.0` on their own if desired. `BuildTest.CSharp8Features` now passes for me locally.
Fixes: #5049 Context: dotnet/roslyn@12f1b8f#diff-125e2d8c52dd9033f9b248f79f78c3f8 Context: dotnet/roslyn#44911 (comment) The `BuildTest.CSharp8Features()` test was failing for me locally when I have Visual Studio 2019 16.7.2 installed: Foo.cs(1,23): error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater. After review, it appears that a change in Roslyn's MSBuild targets [removed `$(MaxSupportedLangVersion)`][0] and made the property\ private: `$(_MaxSupportedLangVersion)`. After some discussion around C# 9.0: * It is unknown if C# 9.0 will be supported by mono/mono/2020-02. Some features like covariant return types and function pointers require runtime changes. * Using a build of .NET 5 rc1, `$(LangVersion)` is already being set to `9.0` by default. We should just change the current behavior of: <MaxSupportedLangVersion Condition=" '$(MaxSupportedLangVersion)' == '' ">8.0</MaxSupportedLangVersion> and change it to: <LangVersion Condition=" '$(LangVersion)' == '' ">8.0</LangVersion> This matches [Roslyn's default behavior][1]: .NET 5+ projects should be unaffected by this change, as they do not import `Xamarin.Android.CSharp.targets`. If at some point, if mono/mono/2020-02 fully supports C# 9 we can bump this value. Users can opt into `9.0` on their own if desired. `BuildTest.CSharp8Features()` now passes for me locally. [0]: dotnet/roslyn@12f1b8f#diff-125e2d8c52dd9033f9b248f79f78c3f8 [1]: https://github.com/dotnet/roslyn/blob/1aeda2e94fb9371b96d4bfd94b074860064ec3d2/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L6-L21
Fixes: #5049 Context: dotnet/roslyn@12f1b8f#diff-125e2d8c52dd9033f9b248f79f78c3f8 Context: dotnet/roslyn#44911 (comment) The `BuildTest.CSharp8Features()` test was failing for me locally when I have Visual Studio 2019 16.7.2 installed: Foo.cs(1,23): error CS8370: Feature 'using declarations' is not available in C# 7.3. Please use language version 8.0 or greater. After review, it appears that a change in Roslyn's MSBuild targets [removed `$(MaxSupportedLangVersion)`][0] and made the property\ private: `$(_MaxSupportedLangVersion)`. After some discussion around C# 9.0: * It is unknown if C# 9.0 will be supported by mono/mono/2020-02. Some features like covariant return types and function pointers require runtime changes. * Using a build of .NET 5 rc1, `$(LangVersion)` is already being set to `9.0` by default. We should just change the current behavior of: <MaxSupportedLangVersion Condition=" '$(MaxSupportedLangVersion)' == '' ">8.0</MaxSupportedLangVersion> and change it to: <LangVersion Condition=" '$(LangVersion)' == '' ">8.0</LangVersion> This matches [Roslyn's default behavior][1]: .NET 5+ projects should be unaffected by this change, as they do not import `Xamarin.Android.CSharp.targets`. If at some point, if mono/mono/2020-02 fully supports C# 9 we can bump this value. Users can opt into `9.0` on their own if desired. `BuildTest.CSharp8Features()` now passes for me locally. [0]: dotnet/roslyn@12f1b8f#diff-125e2d8c52dd9033f9b248f79f78c3f8 [1]: https://github.com/dotnet/roslyn/blob/1aeda2e94fb9371b96d4bfd94b074860064ec3d2/src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets#L6-L21
Create
LanguageVersion.CSharp9
set to point atLanguageVersion.Preview
and setLangVersion
for netcoreapp < 5.0 and netstandard2.1 to C# 8.0 by defaultThis doesn't yet remove the psuedo language version (#43348) as CodeStyle references it from the NuGet package so we need to wait for it to be published. That work is ready to go in https://github.com/chsienki/roslyn/tree/remove_pseudo_langver9
I've tested the
LangVersion
selection logic locally, and verified it will work with an imaginary net6.0 and CSharp10 too. (I'll add unit tests when we land the msbuild target tests PR).Fixes #44756