sdk icon indicating copy to clipboard operation
sdk copied to clipboard

CA2252 warnings with .NET 7 RC1 SDK

Open Sergio0694 opened this issue 3 years ago • 9 comments

Version Used: .NET 7 RC1 SDK

Steps to Reproduce:

  1. Clone the .NET Community Toolkit at https://github.com/CommunityToolkit/dotnet/commit/338172af682a5910ca33962211be1751d0dd2008
  2. Run dotnet build on the solution

Expected Behavior:

The solution should build fine like it did up until now.

Actual Behavior:

A whole bunch of CA2252 warnings from the CommunityToolkit.HighPerformance.UnitTests project. This is using a bunch of types from CommunityToolkit.HighPerformance that are marked as [RequiresPreviewFeatures], but the test project using it is using <EnablePreviewFeatures> in the .csproj, so I'm not really sure why with the .NET 7 SDK this is causing issues.

Failing CI run: https://dev.azure.com/dotnet/CommunityToolkit/_build/results?buildId=78431&view=results.

cc. @333fred

Sergio0694 avatar Sep 16 '22 02:09 Sergio0694

@Sergio0694 Does <AssemblyAttribute Include="System.Runtime.Versioning.RequiresPreviewFeaturesAttribute" /> (in a csproj ItemGroup) work? or [assembly: RequiresPreviewFeatures] (in a cs file, e.g, AssemblyInfo.cs) ?

Youssef1313 avatar Sep 16 '22 11:09 Youssef1313

Couple of things:

  1. It seems the the SDK only generates the attribute for you if this condition is met '$(IsNetCoreAppTargetingLatestTFM)' == 'true'
  2. The attribute exists in .NET 6 and later, so you might have to define it yourself for older TFMs.

@buyaa-n could the analyzer check for the MSBuild property directly, instead of checking the attribute? or otherwise, relax conditions of the attribute generation on dotnet/sdk side?

https://github.com/dotnet/sdk/blob/12f697339952e0f15d1fdf90c395b10cd9f9e1c3/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.GenerateAssemblyInfo.targets#L40

Youssef1313 avatar Sep 16 '22 11:09 Youssef1313

@buyaa-n could the analyzer check for the MSBuild property directly, instead of checking the attribute? or otherwise, relax conditions of the attribute generation on dotnet/sdk side?

By design it should not read MSBuild property directly because it is not depend from that one property, the analyzer only need to account RequiresPreviewFeatures attribute

but the test project using it is using <EnablePreviewFeatures> in the .csproj, so I'm not really sure why with the .NET 7 SDK this is causing issues.

In addition to <EnablePreviewFeatures> the RequiresPreviewFeatures attribute will be added if <GenerateRequiresPreviewFeaturesAttribute> property is set to true or as @Youssef1313 mentioned if '$(IsNetCoreAppTargetingLatestTFM)' == 'true'

As the project is not set the <GenerateRequiresPreviewFeaturesAttribute property and the project is targeting net6.0 since upgrading to .NET 7 SDK the IsNetCoreAppTargetingLatestTFM will not be true anymore. So I would suggest to set <GenerateRequiresPreviewFeaturesAttribute>true</GenerateRequiresPreviewFeaturesAttribute> probably with Condition="'$(TargetFramework)' == 'net6.0'" condition

Also I am not sure if '$(IsNetCoreAppTargetingLatestTFM)' == 'true' in MSBuild is correct condition we need, it seems to me it should be a condition that checking .NET 6 or above CC @terrajobst @dsplaisted

buyaa-n avatar Sep 20 '22 18:09 buyaa-n

Transferring the issue into SDK repo as the root cause is not related to the analyzer and to let SDK folks discuss about '$(IsNetCoreAppTargetingLatestTFM)' == 'true' condition

buyaa-n avatar Sep 20 '22 18:09 buyaa-n

@terrajobst It sounds what might be happening here is that a project stops working when moving from the .NET 6 SDK to the .NET 7 SDK, as it targets .NET 6.0 and no longer automatically gets opted in to preview features with the .NET 7 SDK.

What is the expected behavior here?

dsplaisted avatar Sep 20 '22 19:09 dsplaisted

For reference, here's the PR that introduced this logic: #18815

jeffhandley avatar Sep 20 '22 22:09 jeffhandley

I guess https://github.com/dotnet/designs/pull/232 explains it. It looks by-design?

Youssef1313 avatar Sep 20 '22 22:09 Youssef1313

Ah, you're right @Youssef1313!

The key section is here: https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md#meaning-of-property-in-multi-targeted-projects

This design detail was intended to handle scenarios around preview features that ship as part of the .NET SDK. The reported issue here does not involve .NET SDK preview feature, but the logic gets applied nonetheless.

The workarounds are to either target net7.0 or use the .NET 6 SDK. We should consider updating that logic to avoid this pitfall, but I think that would be out of scope for .NET 7 at this point.

jeffhandley avatar Sep 20 '22 22:09 jeffhandley

@jeffhandley Can you handle tracking any changes for .NET 8 you might want to do for this?

dsplaisted avatar Sep 26 '22 17:09 dsplaisted

Looking back at this as we wind down .NET 8, and I think we should close this as by-design without making any changes. As mentioned above, this behavior was intentional as the behavior of Preview Features was that it requires having the runtime and SDK aligned on their versions as either side could have incompatible changes to the feature in the next version; enforcing the alignment keeps you on the understood pairing of versions for that preview feature.

jeffhandley avatar Jul 26 '23 18:07 jeffhandley