Determine the maximum supported language version #37018

tmeschter · 2019-07-05T21:19:23Z

.NETCore (version 3.0) supports C# 8.0.

.NETFramework (all versions), .NETStandard (all versions up through 2.1,
the latest at this time), and .NETCore (all versions before 3.0) support
version 7.3 of the C# language. Use of a later version with any of those
frameworks is not supported, though it will not be blocked.

Certain IDE features need to know this maximum supported
lang version in addition to the existing 'LangVersion' property. Here we
capture that as the 'MaxSupportedLangVersion' property. The project
systems will query for this property and pass it along to the language
service.

Note that other frameworks (e.g. Xamarin, Unity, etc.) are expected to
set this property themselves if they wish to use something other than
7.3.

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

.NETCore (version 3.0) supports C# 8.0. .NETFramework (all versions), .NETStandard (all versions up through 2.1, the latest at this time), and .NETCore (all versions before 3.0) support version 7.3 of the C# language. Use of a later version with any of those frameworks is not supported, though it will not be blocked. To support certain features the IDE needs to know this maximum supported lang version in addition to the existing 'LangVersion' property. Here we capture that as the 'MaxSupportedLangVersion' property. The project systems will query for this property and pass it along to the language service. Note that other frameworks (e.g. Xamarin, Unity, etc.) are expected to set this property themselves if they wish to use something other than 7.3.

1. Replace incorrect '.NETCore' with correct '.NETCoreApp' when checking 'TargetFrameworkIdentifier'. 2. Remove unnecessary parentheses.

The 'TargetFrameworkVersion' property takes the form 'v1.0', 'v2.1', etc. To perform an actual version comparison in MSBuild we need to strip off the leading 'v' character. This allows us to specify 8.0 as the max supported C# language version for .NETCoreApp >= 3.0. This avoids a potential future problem where we release .NETCoreApp 5.0 and we inadvertently revert the language version to 7.3.

333fred · 2019-07-08T19:48:38Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

+      <_TargetFrameworkVersionAsVersion>$(TargetFrameworkVersion.Substring(1))</_TargetFrameworkVersionAsVersion>
+      <MaxSupportedLangVersion Condition="'$(MaxSupportedLangVersion)' == '' AND
+                                          '$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND
+                                          '$(_TargetFrameworkVersionAsVersion)' >= '3.0'">8.0</MaxSupportedLangVersion>


8.0 [](start = 90, length = 3)

Requesting @agocke to take a look at this. This is going to quickly become a problem when someone forgets to update this targets file in the future, Andy is there any way we can tie this to the general language version infra we already have? Otherwise, we should add something to build boss to check this and be sure they're in sync.

nguerrera · 2019-07-08T22:04:50Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

+           Note that other frameworks (e.g., Xamarin, Unity, etc.) are expected to override this by setting
+           MaxSupportedLangVersion themselves. -->
+      <!-- Strip the 'v' off the beginning of TargetFrameworkVersion so we can actually compare it as a version. -->
+      <_TargetFrameworkVersionAsVersion>$(TargetFrameworkVersion.Substring(1))</_TargetFrameworkVersionAsVersion>


We have _TargetFrameworkVersionWithoutV defined by SDK, and despite the fact that it is "private" by underscore convention, others use it.

Elsewhere, we are more defensive and do TrimStart(vV) instead of Substring(1).

I think maybe instead of creating yet another private variable people may depend upon, you could consider inlining it like: https://github.com/microsoft/msbuild/blob/0cd0e92a7243088977d31b56626b56d6116de016/src/Tasks/Microsoft.Common.CurrentVersion.targets#L328

Longer term, I would want to allow leading v as a valid version as mentioned here: dotnet/msbuild#3212 (comment)

I guess inlining wouldn't be good for perf when there is more than one version below.

cc @rainersigwald for thoughts here.

I lean toward "use the SDK _TargetFrameworkVersionWithoutV", or we could use this as an excuse to implement $([MSBuild]::CompareVersions()) (but the coordinated insertion would be a pain; do it like this initially either way).

I can't count on _TargetFrameworkVersionWithoutV having been defined, though.

oh, sure, this has to work outside the sdk. Then I'd switch to trimstart.

Consider clearing _TargetFrameworkAsVersion too. I really want to avoid more turds we get scared to clean up after we put in a proper feature in msbuild.

That's fair. Is there a canonical way of doing that? Will <_TargetFrameworkVersionAsVersion></_TargetFrameworkVersionAsVersion> do it? I would assume so, but assumptions are dangerous when it comes to MSBuild. :-)

Yes, or the self-closing tag version <_TargetFrameworkVersionAsVersion />. Both "set it to empty" instead of "deleting the variable", but that's basically indistinguishable.

But in this case can you just inline it? I only see one use . . .

Inlining was my first suggestion, but there will be another version check in here eventually. Maybe better to have the code ready to evolve. When someone goes to add a second one, they may not catch what we've discussed here...

The `TargetFrameworkVersion` property typically starts with a `v` or `V`. To properly compare it as a version we're currently trimming off the `v` via `$(TargetFrameworkVersion.Substring(1))`. However, a safer option is `$(TargetFrameworkVersion.TrimStart(vV))` as this will also properly handle the case where there is no leading `v`.

tmeschter · 2019-07-10T17:07:52Z

I believe @agocke has a different implementation of this that will likely supersede this change.

jasonmalinowski

Looks good from the IDE perspective.

jasonmalinowski · 2019-07-10T17:47:37Z

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets

+      <_TargetFrameworkVersionAsVersion>$(TargetFrameworkVersion.TrimStart(vV))</_TargetFrameworkVersionAsVersion>
+      <MaxSupportedLangVersion Condition="'$(MaxSupportedLangVersion)' == '' AND
+                                          '$(TargetFrameworkIdentifier)' == '.NETCoreApp' AND
+                                          '$(_TargetFrameworkVersionAsVersion)' >= '3.0'">8.0</MaxSupportedLangVersion>


Is there any impact here of this PR implicitly changing the default langversion from "preview" to 8.0?

CyrusNajmabadi · 2022-06-03T17:25:44Z

@tmeschter What do you think our next steps should be with this PR? Thanks!

tmeschter requested review from jasonmalinowski and agocke July 5, 2019 21:19

tmeschter requested a review from a team as a code owner July 5, 2019 21:19

RikkiGibson reviewed Jul 5, 2019

View reviewed changes

src/Compilers/Core/MSBuildTask/Microsoft.CSharp.Core.targets Outdated Show resolved Hide resolved

tmeschter force-pushed the MaxSupportedLangVersion-190705 branch from ac0eac6 to 8507e0e Compare July 5, 2019 21:26

tmeschter force-pushed the MaxSupportedLangVersion-190705 branch from 8507e0e to 2226245 Compare July 5, 2019 21:35

tmeschter added 3 commits July 8, 2019 08:54

Fix a typo; style improvements

e2ff12b

1. Replace incorrect '.NETCore' with correct '.NETCoreApp' when checking 'TargetFrameworkIdentifier'. 2. Remove unnecessary parentheses.

Fix a missing single quote

f12e95e

333fred reviewed Jul 8, 2019

View reviewed changes

nguerrera reviewed Jul 8, 2019

View reviewed changes

jinujoseph added the Area-IDE label Jul 9, 2019

tmeschter force-pushed the MaxSupportedLangVersion-190705 branch from 61171fd to 3d70690 Compare July 9, 2019 17:01

jasonmalinowski approved these changes Jul 10, 2019

View reviewed changes

jasonmalinowski mentioned this pull request Jul 15, 2019

Add MaxLangVersion #37244

Merged

Base automatically changed from master to main March 3, 2021 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Determine the maximum supported language version #37018

Determine the maximum supported language version #37018

tmeschter commented Jul 5, 2019

333fred Jul 8, 2019

nguerrera Jul 8, 2019

nguerrera Jul 8, 2019

nguerrera Jul 8, 2019

rainersigwald Jul 9, 2019

tmeschter Jul 9, 2019

rainersigwald Jul 9, 2019

nguerrera Jul 9, 2019

tmeschter Jul 9, 2019

rainersigwald Jul 9, 2019

nguerrera Jul 9, 2019

tmeschter commented Jul 10, 2019

jasonmalinowski left a comment

jasonmalinowski Jul 10, 2019

CyrusNajmabadi commented Jun 3, 2022

Determine the maximum supported language version #37018

Are you sure you want to change the base?

Determine the maximum supported language version #37018

Conversation

tmeschter commented Jul 5, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeschter commented Jul 10, 2019

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 3, 2022