Ensure that PlatformCompatibilityAnalyzer better handles various TFMs #7542

tannergooding · 2025-01-21T18:26:53Z

This resolves #7525

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs

tannergooding · 2025-01-21T20:13:46Z

Hmmm, tests are failing because build_property.TargetFrameworkIdentifier and build_property.TargetFrameworkVersion cannot be resolved by options.GetMSBuildPropertyValue

I don't quite see where that is normally supposed to be plumbed through/set, is this normally done by Roslyn? Are our tests skipping the normal csproj build setup and so manually setting these config knobs as if they had been set by MSBuild?

tannergooding · 2025-01-21T20:16:03Z

Looks like its supposed to be done by GenerateDocumentationAndConfigFiles in a "normal setup". So the fix won't be fully done until after a proper SDK insertion

For tests, I believe this means I need to explicitly set TargetFramework, TargetFrameworkIdentifier, and TargetFrameworkVersion in all the tests. Is that correct?

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

krwq

Product changes look good and I validated this shows warning in the command line (not in VS though). Test changes look good once they pass and shape didn't change significantly

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

…w expected properties

tannergooding · 2025-01-22T18:06:48Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs

+            if (tfmIdentifier.Equals(NetCoreAppIdentifier, StringComparison.OrdinalIgnoreCase) &&
+                tfmVersion.StartsWith("v", StringComparison.OrdinalIgnoreCase) &&
+                Version.TryParse(tfmVersion[1..], out var version) &&
+                version.Major >= 5)


Updated this to do the simpler check that matches what is required for the format, as per the discussion.

tannergooding · 2025-01-22T18:08:03Z

...ft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.GuardedCallsTests.cs

 [*]
 build_property.TargetFramework = net5
+build_property.TargetFrameworkIdentifier = .NETCoreApp
+build_property.TargetFrameworkVersion = v5.0


Since these tests aren't actually going through msbuild, it means the props/targets that would normally automatically provide these don't run; hence the tests need to pass it along explicitly.

In all cases I've checked what msbuild proper would generate for a given TargetFramework value and ensured the TargetFrameworkIdentifier and TargetFrameworkVersion values we pass along match that behavior

tannergooding · 2025-01-22T18:09:52Z

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

+            yield return new object[] { "build_property.TargetFramework = net5.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", true };
+            yield return new object[] { "build_property.TargetFramework = net5.0-windows\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", true };
+            yield return new object[] { "build_property.TargetFramework = net5.0-ios14.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", true };
+            yield return new object[] { "build_property.TargetFramework = netcoreapp5\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", true };


A case to note is that TargetFramework=netcoreapp5 was previously treated as "unsupported" by the analyzer.

However, the actual MSBuild path done by our props/targets recognizes this TFM and infers TargetFrameworkIdentifier=.NETCoreApp and TargetFrameworkVersion=v5.0 hence is equivalent to TargetFramework=net5.0 and thus is handled by the analyzer under the new code

tannergooding · 2025-01-22T18:10:45Z

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

+            yield return new object[] { "build_property.TargetFramework = netstandard2.1\nbuild_property.TargetFrameworkIdentifier=.NETStandard\nbuild_property.TargetFrameworkVersion=v2.1", false };
+
+            // .NET 5+ TFMs with a single major version digit
+            yield return new object[] { "build_property.TargetFramework = net5\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", true };


Another case to note is that while TargetFrameworkVersion must have two parts, and thus must be v5.0, the actual TFM does not require this.

Simply doing TargetFramework=net5 is recognized by the inferrence and produces TargetFrameworkIdentifier=.NETCoreApp and TargetFrameworkVersion=v5.0 hence is equivalent to TargetFramework=net5.0 and thus is handled by the analyzer under the new code

tannergooding · 2025-01-22T18:11:21Z

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

+            yield return new object[] { "build_property.TargetFramework = net10.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v10.0", true };
+            yield return new object[] { "build_property.TargetFramework = net11.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v11.0", true };


These are the cases that were unexpectedly failing and which are being fixed by this PR.

tannergooding · 2025-01-22T18:12:25Z

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs

+            // Custom TFMs with valid user specified Identifier/Version
+            yield return new object[] { "build_property.TargetFramework = nonesense\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v11.0", true };
+
+            // Custom TFMs with invalid user specified Identifier/Version
+            yield return new object[] { "build_property.TargetFramework = nonesense\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v5", false };
+            yield return new object[] { "build_property.TargetFramework = nonesense\nbuild_property.TargetFrameworkIdentifier=NETCoreApp\nbuild_property.TargetFrameworkVersion=v5.0", false };
+
+            // Custom TFMs with inferred Identifier/Version
+            yield return new object[] { "build_property.TargetFramework = nonesense\nbuild_property.TargetFrameworkIdentifier=Unsupported\nbuild_property.TargetFrameworkVersion=v0.0", false };


These are additional cases that cover custom TFMs to help assert we're handling them as expected

tannergooding · 2025-01-22T18:13:41Z

src/Utilities/Compiler/Options/MSBuildPropertyOptionNames.cs

+        public const string TargetFrameworkIdentifier = nameof(TargetFrameworkIdentifier);
+        public const string TargetFrameworkVersion = nameof(TargetFrameworkVersion);


This is what causes the CompilerVisibleProperty items to be generated in the props/targets that are inserted into the SDK and is what causes the build_property.* metadata to be automatically visible to analyzers by default.

codecov · 2025-01-22T18:28:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.50%. Comparing base (45caa45) to head (52c0b20).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7542   +/-   ##
=======================================
  Coverage   96.50%   96.50%           
=======================================
  Files        1452     1452           
  Lines      347566   347574    +8     
  Branches    11416    11418    +2     
=======================================
+ Hits       335415   335425   +10     
+ Misses       9255     9252    -3     
- Partials     2896     2897    +1

ViktorHofer · 2025-01-22T18:51:35Z

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs

-                int.TryParse(tfmString[3].ToString(), out var major) &&
-                major >= 5)
+            if (tfmIdentifier.Equals(NetCoreAppIdentifier, StringComparison.OrdinalIgnoreCase) &&
+                tfmVersion.StartsWith("v", StringComparison.OrdinalIgnoreCase) &&


I'm not 100% sure about the leading v here. What's the value of the check here? I was imagining that we would just do a TrimStart('vV') before we read from the value. We do that in other places in SDK: https://github.com/dotnet/sdk/blob/0bc6363db69ab830662979d35bfca9f941ab0736/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.BeforeCommon.targets#L191

It depends on if we expect TargetFrameworkVersion to be in the format v#.#[.#[.#]] or [v]#.#[.#[.#]]

The only case we'd not have the v is users specifying custom values and following the standard format feels much safer, particularly given the existence of _TargetFrameworkVersionWithoutV and other assumptions we make.

@ericstj any opinions on this?

We have the https://github.com/dotnet/runtime/blob/1f93ea7763a6391a9063c5e35843253f3d666893/src/libraries/System.Private.CoreLib/src/System/Runtime/Versioning/FrameworkName.cs type that we could use to just parse the value of TargetFrameworkMoniker property

So we could just use that, but barring that I think trimming is better -- since we do that ourselves.
https://github.com/dotnet/runtime/blob/1f93ea7763a6391a9063c5e35843253f3d666893/src/libraries/System.Private.CoreLib/src/System/Runtime/Versioning/FrameworkName.cs#L163-L170

type that we could use to just parse the value of TargetFrameworkMoniker property

This seems like an overall worse thing to depend on as it would result in various throwing cases we'd have to try/catch in the analyzer to ensure robustness.

It also seems like "extra work" as TargetFrameworkMoniker is generated from TargetFrameworkIdentifier + TargetFrameworkVersion which are what feed most other things in MSBuild; not the other way around.

So we could just use that, but barring that I think trimming is better -- since we do that ourselves.

The code is already trimming, the question was rather whether TargetFrameworkVersion should require the v prefix or not. The code currently looks for it to exist and fallsback to the LowerTargetsEnabled path if its not present.

None of the normal MSBuild paths will skip specifying the v, so the only scenario that'd be encounterable is if a user is manually specifying TargetFrameworkIdentifier+TargetFrameworkVersion.

jeffhandley

Well done on this, @tannergooding. Thank you for the very descriptive PR comments that aided in the review.

jeffhandley · 2025-01-22T23:07:50Z

Shoot -- I merged and then the page refreshed and showed recent comments. Sorry, @tannergooding and @ericstj.

tannergooding · 2025-01-22T23:10:40Z

Shoot -- I merged and then the page refreshed and showed recent comments. Sorry, @tannergooding and @ericstj.

Shouldn't be an issue @jeffhandley. The logic here is already significantly better/more robust than what we had and will resolve the issue users were seeing

The question is really just whether we want to allow TargetFrameworkVersion=5.0 or require to be of the form TargetFrameworkVersion=v5.0 (which is what the default inference will always generate). That can easily be done as a follow up and shouldn't block Preview 1 or anything like that, it's just a minor nice to have to best support manually specified properties if that's what we want to do.

ericstj · 2025-01-23T16:45:39Z

It's your call if you want to come back and clean this up. MSBuild properties are the wild west in terms of input validation / extensibility / etc so I usually try to look for existing usage/assumptions when taking a dependency.

Agreed that this discussion wasn't blocking - I just chimed in with my take on the existing discussion.

FWIW this change has made it to the SDK so it should be in Preview1 builds dotnet/sdk@c2f13b4

tannergooding · 2025-01-23T16:48:20Z

I usually try to look for existing usage/assumptions when taking a dependency.

Sounds good. I'll submit a PR allowing no v to be present since it looks like the SDK generally handles that scenario without failing; but it might be something good for us to document and ensure its consistently handled in the future.

tannergooding · 2025-01-23T17:44:14Z

Put up #7544

Ensure that PlatformCompatibilityAnalyzer better handles various TFMs

3fc3836

tannergooding requested a review from a team as a code owner January 21, 2025 18:26

Fix a typo

876f3c0

krwq reviewed Jan 21, 2025

View reviewed changes

...tAnalyzers/Core/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzer.cs Outdated Show resolved Hide resolved

krwq reviewed Jan 22, 2025

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs Outdated Show resolved Hide resolved

krwq reviewed Jan 22, 2025

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs Show resolved Hide resolved

krwq approved these changes Jan 22, 2025

View reviewed changes

krwq reviewed Jan 22, 2025

View reviewed changes

.../UnitTests/Microsoft.NetCore.Analyzers/InteropServices/PlatformCompatibilityAnalyzerTests.cs Outdated Show resolved Hide resolved

Fixing up the PlatformCompatibilityAnalyzer tests to pass down the ne…

52c0b20

…w expected properties

tannergooding commented Jan 22, 2025

View reviewed changes

ViktorHofer reviewed Jan 22, 2025

View reviewed changes

jeffhandley approved these changes Jan 22, 2025

View reviewed changes

jeffhandley merged commit 5c3c07b into dotnet:main Jan 22, 2025
11 checks passed

tannergooding deleted the fix-7525 branch January 22, 2025 23:10

tannergooding mentioned this pull request Jan 23, 2025

Trim leading v from TargetFrameworkVersion #7544

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that PlatformCompatibilityAnalyzer better handles various TFMs #7542

Ensure that PlatformCompatibilityAnalyzer better handles various TFMs #7542

tannergooding commented Jan 21, 2025

tannergooding commented Jan 21, 2025

tannergooding commented Jan 21, 2025 •

edited

Loading

krwq left a comment

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

tannergooding Jan 22, 2025

codecov bot commented Jan 22, 2025 •

edited

Loading

ViktorHofer Jan 22, 2025

tannergooding Jan 22, 2025

ViktorHofer Jan 22, 2025

ericstj Jan 22, 2025 •

edited

Loading

tannergooding Jan 22, 2025 •

edited

Loading

jeffhandley left a comment

jeffhandley commented Jan 22, 2025

tannergooding commented Jan 22, 2025

ericstj commented Jan 23, 2025

tannergooding commented Jan 23, 2025

tannergooding commented Jan 23, 2025

		yield return new object[] { "build_property.TargetFramework = net10.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v10.0", true };
		yield return new object[] { "build_property.TargetFramework = net11.0\nbuild_property.TargetFrameworkIdentifier=.NETCoreApp\nbuild_property.TargetFrameworkVersion=v11.0", true };

		public const string TargetFrameworkIdentifier = nameof(TargetFrameworkIdentifier);
		public const string TargetFrameworkVersion = nameof(TargetFrameworkVersion);

Ensure that PlatformCompatibilityAnalyzer better handles various TFMs #7542

Ensure that PlatformCompatibilityAnalyzer better handles various TFMs #7542

Conversation

tannergooding commented Jan 21, 2025

tannergooding commented Jan 21, 2025

tannergooding commented Jan 21, 2025 • edited Loading

krwq left a 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

codecov bot commented Jan 22, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericstj Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

tannergooding Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

jeffhandley left a comment

Choose a reason for hiding this comment

jeffhandley commented Jan 22, 2025

tannergooding commented Jan 22, 2025

ericstj commented Jan 23, 2025

tannergooding commented Jan 23, 2025

tannergooding commented Jan 23, 2025

tannergooding commented Jan 21, 2025 •

edited

Loading

codecov bot commented Jan 22, 2025 •

edited

Loading

ericstj Jan 22, 2025 •

edited

Loading

tannergooding Jan 22, 2025 •

edited

Loading