Conversation
|
@dotnet/roslyn-compiler for review please |
There was a problem hiding this comment.
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.
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.
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: Thanks! |
There was a problem hiding this comment.
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.
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.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
We should add a section that makes 9.0 the default when targeting netcoreapp 5.0 #Resolved
There was a problem hiding this comment.
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.
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.
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.
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.
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.
Add a case for
[InlineData("UnknownTFM", "5.0", "7.3")]
#Resolved
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 :) |
|
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.
_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.
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.
We can copy the definition of this property to our targets:
<_TargetFrameworkVersionWithoutV>$(TargetFrameworkVersion.TrimStart('vV'))</_TargetFrameworkVersionWithoutV>
There was a problem hiding this comment.
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.CSharp9set to point atLanguageVersion.Previewand setLangVersionfor 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
LangVersionselection 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