Add and use TargetFramework.Net70 #64490

jcouv · 2022-10-04T23:05:22Z

Closes #61463 (adds Net70)
Closes #61235 (removes RuntimeSupportsNumericIntPtr )

Relates to test plan #60578 (Numeric IntPtr)
Relates to test plan #59194 (ref fields)

Documentation of process:

create a package with ref assemblies (see doc and PR for .NET 7 ref assemblies)
ask First Responders to ingest resulting package from nuget into dotnet-public Azure feed
add new TargetFramework, consuming that package (see first commit in this PR)

Note to future self: when adding a new TFM, it is good to update NetCoreApp to point to it. This requires a bit of a process:

Move TargetFrameworkUtil.NetCoreApp forward and start a draft PR
Fix up all of the tests that just have simple errors
Hard code the rest to Net50
Merge

Ideally, we can remove the packages for old reference assemblies, to reduce the amount of copy to test folders in CI.

AlekseyTs · 2022-10-05T16:48:33Z

src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs

        {
            get
            {
                // CorLibrary should never be null, but that invariant is broken in some cases for MissingAssemblySymbol.
                // Tracked by https://github.com/dotnet/roslyn/issues/61262
-                return CorLibrary?.RuntimeSupportsNumericIntPtr == true;
+                return CorLibrary is not null &&
+                    RuntimeSupportsFeature(SpecialMember.System_Runtime_CompilerServices_RuntimeFeature__NumericIntPtr);


Consider separating product changes and infrastructure changes into separate PRs

AlekseyTs · 2022-10-05T16:53:24Z

src/Compilers/Core/Portable/MetadataReference/AssemblyMetadata.cs

@@ -260,11 +260,6 @@ internal new AssemblyMetadata Copy()
            return new AssemblyMetadata(this, shareCachedSymbols: true);
        }

-        internal AssemblyMetadata CopyWithoutSharingCachedSymbols()


internal AssemblyMetadata CopyWithoutSharingCachedSymbols()

Given the volume of changes in this PR, consider splitting it into several more targeted PRs. We can discuss offline in more details.

AlekseyTs · 2022-10-05T17:05:37Z

The title of the PR feels somewhat misleading. For example, in my mind adding TargetFramework.Net70 for tests is not equivalent to changing target framework for project files, etc. In general, it feels like this PR bundles to many unrelated work items together. I understand that there is likely an order dependency between them (one must be done before the other), but there are probably work items that that do not depend on others.

jcouv · 2022-10-05T19:21:27Z

#if NET7_0_OR_GREATER

@AlekseyTs I've shrunk this PR to exclude the upgrade to new SDK.

I just want to point out this means that tests in NumerIntPtrTests no longer execute (see this line, but we never build the test projects with .NET 7). I'm okay with this temporary regression in coverage given that I have the follow-up change working already.

Note that RefFieldTests has the same problem, but there is no regression there since they didn't execute prior to this PR.

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:32 in 86432cd. [](commit_id = 86432cd, deletion_comment = False)

cston · 2022-10-06T15:54:38Z

src/Compilers/CSharp/Portable/Symbols/AssemblySymbol.cs

        {
            get
            {
                // CorLibrary should never be null, but that invariant is broken in some cases for MissingAssemblySymbol.
                // Tracked by https://github.com/dotnet/roslyn/issues/61262
-                return CorLibrary?.RuntimeSupportsNumericIntPtr == true;
+                return CorLibrary is not null &&


CorLibrary is not null &&

Is the CorLibrary check needed here? It's not included in similar properties below. #Resolved

It is. One test reaches this with a null CorLibrary (CompilationReferences_More, crash in EmitDifference). This is tracked by above issue.
I'm not sure why numeric IntPtr is uniquely susceptible, but it might be because it's involved in metadata decoding for a type that already exists in the BCL ref assemblies.

cston · 2022-10-06T16:13:45Z

        CompileAndVerify(comp, verify: Verification.FailsPEVerify, expectedOutput: IncludeExpectedOutput(

Just curious: Why do these tests fail PEVerify?

In reply to: 1270347021

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:1406 in ab4209b. [](commit_id = ab4209b, deletion_comment = False)

AlekseyTs · 2022-10-06T16:28:10Z

IL_0014: call ""readonly bool nint?.HasValue.get""

What is the source of baseline changes like this?

In reply to: 1270370025

In reply to: 1270370025 #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:1635 in ab4209b. [](commit_id = ab4209b, deletion_comment = False)

AlekseyTs · 2022-10-06T16:35:28Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

            comp.VerifyDiagnostics(
+                // (4,18): error CS9064: Target runtime doesn't support ref fields.
+                //     public ref T F;
+                Diagnostic(ErrorCode.ERR_RuntimeDoesNotSupportRefFields, "F").WithLocation(4, 18),


Diagnostic(ErrorCode.ERR_RuntimeDoesNotSupportRefFields, "F").WithLocation(4, 18),

If the goal is to verify presence of this diagnostics, then we probably shouldn't be targeting default framework. Consider targeting Net50, for example. #Closed

On second look, the original test didn't seem to be about the attribute definition being provided in source. I've removed the extra validation I'd added.

jcouv · 2022-10-06T16:36:16Z

IL_0014: call ""readonly bool nint?.HasValue.get""

This was a change in the BCL: dotnet/runtime#1727

In reply to: 1270370025

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:1635 in ab4209b. [](commit_id = ab4209b, deletion_comment = False)

AlekseyTs · 2022-10-06T16:39:41Z

Done with review pass (commit 4)

jcouv · 2022-10-06T16:49:46Z

        CompileAndVerify(comp, verify: Verification.FailsPEVerify, expectedOutput: IncludeExpectedOutput(

I've not investigated in great details.
PEVerify gives rather cryptic errors. All the skipped tests give 3x "Type load failed." regardless of content of test. Even if this test is removed (empty Main() method) those errors occur.
I would not expect PE Verify to work in Net70.

In reply to: 1270347021

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:1406 in ab4209b. [](commit_id = ab4209b, deletion_comment = False)

AlekseyTs · 2022-10-06T16:57:25Z

        CompileAndVerify(comp, verify: Verification.FailsPEVerify, expectedOutput: IncludeExpectedOutput(

I've not investigated in great details.
PEVerify gives rather cryptic errors. All the skipped tests give 3x "Type load failed." regardless of content of test.

Could this be a temporary problem caused by the fact that we are trying to run tests in .NET 6 runtime, but using ref assemblies of the next version?

I would not expect PE Verify to work in Net70.

It is not obvious to me why that is. Could you elaborate please? At least I think we need to open a follow-up issue, or perhaps make verification conditional on whether we are building against NET7

In reply to: 1270401588

Refers to: src/Compilers/CSharp/Test/Emit2/Emit/NumericIntPtrTests.cs:1406 in ab4209b. [](commit_id = ab4209b, deletion_comment = False)

jcouv · 2022-10-06T17:50:52Z

CodeFlow froze for the second time when I tried to post this response. Using GitHub for third attempt...

Could this be a temporary problem caused by the fact that we are trying to run tests in .NET 6 runtime, but using ref assemblies of the next version?

PEVerify only runs in Desktop runtime. Which version of the Core runtime we use doesn't seem relevant.
Either way, the same problem occurs in my next PR which adopts .NET 7 runtime to execute tests.

I would not expect PE Verify to work in Net70.

PEVerify is a tool/API for validation of PE on Desktop runtime. As far as I can tell, it is only given the bits for the module that we've emitted (see CLRHelpers.PeVerify), not references, so I don't know how it could resolve references to .NET Core BCL.

This is not a new problem, by the way. All tests that use TargetFramework.Net60 or Net50 skip PEVerify (PEVerify only runs when IsWindowsDesktop is true).

At least I think we need to open a follow-up issue, or perhaps make verification conditional on whether we are building against NET7

Let's discuss with Jared. I don't think we're investing in validation using PEVerify anymore.

AlekseyTs

LGTM (commit 5)

jcouv · 2022-10-06T18:31:12Z

I've resolved conflict with #64404 and that caused me to identify a bug in Verification.PassesOrFailFast logic. Conflict resolution and fix are in separate/last commit (e0f2dd2).

* upstream/main: (252 commits) Use the source-built version of ref packs and don't use app host when building in source-build (dotnet#64055) Enable rich LSIF hover information. (dotnet#64580) Add ChecksumAlgorithm to project snapshot (dotnet#62840) Utility for uploading artifact on test failure (dotnet#64578) Enable diagnostics Revert "Remove unused TS brace completion code" Publish additional packages to vssdk feed (dotnet#64571) spelling Move check Simplify SymbolKey implementation lint Update publish data to test PR validation fix (dotnet#64559) Simplify Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/SymbolKey/SymbolKey.cs Lint Proper equality checks Update src/Workspaces/Remote/Core/RemoteCallback.cs Revert "Not wait for solution crawler because it can be very busy" Add and use TargetFramework.Net70 (dotnet#64490) Not wait for solution crawler because it can be very busy ...

jcouv and others added 2 commits October 4, 2022 12:06

Add TargetFramework.Net70

edcf3d0

Remove RuntimeSupportsByRefFields setter

ad69fb6

jcouv self-assigned this Oct 4, 2022

dotnet-issue-labeler bot added the Area-Compilers label Oct 4, 2022

jcouv force-pushed the net-70 branch 4 times, most recently from d2a47f8 to 06e4f21 Compare October 5, 2022 05:28

Remove RuntimeSupportsNumericIntPtr setter

34ac0ac

jcouv force-pushed the net-70 branch from 06e4f21 to b8f2fbc Compare October 5, 2022 06:14

AlekseyTs reviewed Oct 5, 2022

View reviewed changes

jcouv force-pushed the net-70 branch 2 times, most recently from 9d3d153 to 86432cd Compare October 5, 2022 19:13

jcouv changed the title ~~Add TargetFramework.Net70~~ Add and use TargetFramework.Net70 Oct 5, 2022

jcouv force-pushed the net-70 branch from 86432cd to 34ac0ac Compare October 5, 2022 20:58

Remove unused method

ab4209b

jcouv marked this pull request as ready for review October 6, 2022 05:50

jcouv requested review from a team as code owners October 6, 2022 05:50

cston reviewed Oct 6, 2022

View reviewed changes

AlekseyTs reviewed Oct 6, 2022

View reviewed changes

Remove added validation

080e20c

AlekseyTs approved these changes Oct 6, 2022

View reviewed changes

cston approved these changes Oct 6, 2022

View reviewed changes

jcouv added 2 commits October 6, 2022 11:27

Merge remote-tracking branch 'dotnet/main' into net-70

40be908

Merge conflict and fix Verification.PassesOrFailFast

e0f2dd2

cston approved these changes Oct 6, 2022

View reviewed changes

Another conflict

fd1c7a7

jcouv merged commit 09fd334 into dotnet:main Oct 6, 2022

jcouv deleted the net-70 branch October 6, 2022 23:27

ghost added this to the Next milestone Oct 6, 2022

allisonchou mentioned this pull request Oct 11, 2022

[Automated] PRs inserted in VS build main-33011.173 #64652

Closed

jcouv mentioned this pull request Oct 18, 2022

Make TargetFramework.NetCoreApp use Net70 #64791

Merged

ryzngard mentioned this pull request Oct 19, 2022

Fix Color Contrast For STE Close Button #64834

Merged

RikkiGibson modified the milestones: Next, 17.5 P1 Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and use TargetFramework.Net70 #64490

Add and use TargetFramework.Net70 #64490

jcouv commented Oct 4, 2022 •

edited

Loading

AlekseyTs Oct 5, 2022

AlekseyTs Oct 5, 2022

AlekseyTs commented Oct 5, 2022

jcouv commented Oct 5, 2022

cston Oct 6, 2022 •

edited by jcouv

Loading

jcouv Oct 6, 2022

cston commented Oct 6, 2022 •

edited by jcouv

Loading

AlekseyTs commented Oct 6, 2022 •

edited

Loading

AlekseyTs Oct 6, 2022 •

edited

Loading

jcouv Oct 6, 2022

jcouv commented Oct 6, 2022

AlekseyTs commented Oct 6, 2022

jcouv commented Oct 6, 2022

AlekseyTs commented Oct 6, 2022

jcouv commented Oct 6, 2022

AlekseyTs left a comment

jcouv commented Oct 6, 2022

Add and use TargetFramework.Net70 #64490

Add and use TargetFramework.Net70 #64490

Conversation

jcouv commented Oct 4, 2022 • edited Loading

AlekseyTs Oct 5, 2022

Choose a reason for hiding this comment

AlekseyTs Oct 5, 2022

Choose a reason for hiding this comment

AlekseyTs commented Oct 5, 2022

jcouv commented Oct 5, 2022

cston Oct 6, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Oct 6, 2022

Choose a reason for hiding this comment

cston commented Oct 6, 2022 • edited by jcouv Loading

AlekseyTs commented Oct 6, 2022 • edited Loading

AlekseyTs Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Oct 6, 2022

Choose a reason for hiding this comment

jcouv commented Oct 6, 2022

AlekseyTs commented Oct 6, 2022

jcouv commented Oct 6, 2022

AlekseyTs commented Oct 6, 2022

jcouv commented Oct 6, 2022

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented Oct 6, 2022

jcouv commented Oct 4, 2022 •

edited

Loading

cston Oct 6, 2022 •

edited by jcouv

Loading

cston commented Oct 6, 2022 •

edited by jcouv

Loading

AlekseyTs commented Oct 6, 2022 •

edited

Loading

AlekseyTs Oct 6, 2022 •

edited

Loading