Tagging subscribers to this area: @eerhardt
Notify danmosemsft if you want to be subscribed.

we need to avoid any C# feature that is not available in C# 7.3

Do you want to change the LangVersion in the csproj to enforce that?

we need to avoid any C# feature that is not available in C# 7.3

Do you want to change the LangVersion in the csproj to enforce that?

I fiddled with it for a bit, it appears that the build simply ignore whatever that I put in the <LangVersion> msbuild property.

Even this compile (using build.cmd -s clr), note the <LangVersion>Whatever</LangVersion>
However, if I set <PlatformTarget>Whatever</PlatformTarget>, the build would fail.

There must be something special with LangVersion somewhere.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <AssemblyName>ILCompiler.Reflection.ReadyToRun</AssemblyName>
    <AssemblyVersion>1.0.0.0</AssemblyVersion>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <OutputType>Library</OutputType>
    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
    <AssemblyKey>Open</AssemblyKey>
    <IsDotNetFrameworkProductAssembly>true</IsDotNetFrameworkProductAssembly>
    <TargetFramework>netstandard2.0</TargetFramework>
    <CLSCompliant>false</CLSCompliant>
    <NoWarn>8002,NU1701</NoWarn>
    <RuntimeIdentifiers>win-x64;win-x86</RuntimeIdentifiers>
    <OutputPath>$(BinDir)</OutputPath>
    <Platforms>AnyCPU;x64</Platforms>
    <PlatformTarget>AnyCPU</PlatformTarget>
    <LangVersion>Whatever</LangVersion>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="System.Reflection.Metadata" Version="1.8.1" />
    <PackageReference Include="System.Runtime.CompilerServices.Unsafe" Version="4.7.1" />
  </ItemGroup>

  <ItemGroup>
    <Compile Include="..\..\Common\Internal\Runtime\CorConstants.cs" Link="Common\CorConstants.cs" />
    <Compile Include="..\..\Common\Internal\Runtime\ModuleHeaders.cs" Link="Common\ModuleHeaders.cs" />
    <Compile Include="..\..\Common\Internal\Runtime\ReadyToRunConstants.cs" Link="Common\ReadyToRunConstants.cs" />
    <Compile Include="..\..\Common\Internal\Runtime\ReadyToRunInstructionSet.cs" Link="Common\ReadyToRunInstructionSet.cs" />
  </ItemGroup>
</Project>

@cshung to figure out why, you could maybe add "/pp out.txt" to the MSBuild command line and then grep for LangVersion

seems coming from here:

C:\git\runtime\Directory.Build.targets
============================================================================================================================================
-->
  <!--<Import Project="Sdk.targets" Sdk="Microsoft.DotNet.Arcade.Sdk" Condition="'$(SkipImportArcadeSdkFromRoot)' != 'true'" />-->
  <PropertyGroup>
    <!--
      Define this here (not just in Versions.props) because the SDK resets it
      unconditionally in Microsoft.NETCoreSdk.BundledVersions.props.
    -->
    <NETCoreAppMaximumVersion>$(MajorVersion).$(MinorVersion)</NETCoreAppMaximumVersion>
  </PropertyGroup>
  <!-- Language configuration -->
  <PropertyGroup>
    <!-- default to allowing all language features -->
    <LangVersion>preview</LangVersion>

Maybe this should be <LangVersion Condition="'$(LangVersion)' == '''">preview</LangVersion> .. unless there are a bunch of projects that accidentally define it to something else and they want to defeat them.

seems coming from here:

Thanks @danmosemsft, I figured. It appears you're right that Directory.Build.targets is the culprit, but your fix does not work as-is (as LangVersion got some value derived from the target framework before reaching that property, so just checking if it is blank doesn't work), To fix that, we can simply introduce an override as I do in the 2nd commit. I checked, if we build (using build.cmd -s clr)without the fix to remove to recursive matching feature, the build will fail. This should work to prevent future regression.

@safern for the directory.build.targets edit

Would another possible fix be to undo #33868, which would move the defaulting of LangVersion back to the .props file, and then change the condition to be based on $(MSBuildProjectExtension) instead?

Directory.Build.props:

    <!-- default to allowing all language features -->
    <LangVersion>latest</LangVersion>
    <LangVersion Condition="'$(MSBuildProjectExtension)' == '.csproj'">preview</LangVersion>

Then projects in the repo can just set

<LangVersion>X.Y</LangVersion>

as usual.

Would another possible fix be to undo #33868, which would move the defaulting of LangVersion back to the .props file

See the discussion in that issue about it not working in the .props.

and then change the condition to be based on $(MSBuildProjectExtension) instead?

That's not the standard mechanism for this, is it? Seems like another workaround that'll just cause confusion the next time someone tries to do something in this area.

But I don't actually have a strong opinion. I was following @ViktorHofer's advice and will defer to him again :)

See the discussion in that issue about it not working in the .props.

I read the discussion, but didn't see any mention of $(MSBuildProjectExtension) there. I understand why $(Language) doesn't work there - it doesn't get set until the CSharp.CurrentVersion.targets file is imported. However, $(MSBuildProjectExtension) is set by MSBuild itself before the build even starts, so it is guaranteed to be set in the .props. That is the property used to import the CSharp.CurrentVersion.targets file itself:

https://github.com/dotnet/sdk/blob/2eb6c546931b5bcb92cd3128b93932a980553ea1/src/Tasks/Microsoft.NET.Build.Tasks/sdk/Sdk.targets#L25

In order to unblock ILSpy quicker, I reverted the change to fix the LangVersion issue and filed #37498 to track the work to enable the build to fail when feature not available in C# 7.3 is used.

$(MSBuildProjectExtension) is set by MSBuild itself before the build even starts, so it is guaranteed to be set in the .props. That is the property used to import the CSharp.CurrentVersion.targets file itself:

That seems a reasonable approach.