Use covariant return for synthesized record clone #53404

Youssef1313 · 2021-05-13T21:34:56Z

cston · 2021-05-13T21:42:29Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordClone.cs

-            return (ReturnType: VirtualCloneInBase() is { } baseClone ?
-                                     baseClone.ReturnTypeWithAnnotations : // Use covariant returns when available
+            return (ReturnType: VirtualCloneInBase() is { } baseClone && !ContainingAssembly.RuntimeSupportsCovariantReturnsOfClasses ?
+                                     baseClone.ReturnTypeWithAnnotations :
                                     TypeWithAnnotations.Create(isNullableEnabled: true, ContainingType),


Please include a test that demonstrates the change. #Closed

I was expecting to see CI failures since this code path is already extensively tested. Will look and see why no tests are failing.

The tests for records don't vary the RuntimeSupportsCovariantReturnsOfClasses value from corlib, and I don't know what value is there in the corlib that we reference.

One way to control this is to define your own tiny corlib in a test using CreateEmptyCompilation. See RecordTests.ObjectGetHashCode_15 for example.
Another way is to use an old vs. new target framework (targetFramework in CreateCompilation) assuming that the available options are sufficient to achieve the desired effect.

@jcouv I made the assumption that it will work based on another test doing the same. But it looks like the other test has dead code path. But double checking from CI run (#53413)

One way to control this is to define your own tiny corlib in a test using CreateEmptyCompilation

I prefer that we don't do this, we want to test against real frameworks and verify runtime behavior as well. Achieving that with a private tiny corlib is going to be non trivial. I think that testing both modes should be rather simple. It looks like record tests use default TargetFramework. That means that, when tests are run on desktop they are run against desktop (doesn't support covariant returns) and, when run on CoreCLR they are run against CoreCLR (supports covariant returns). We should be able to observe difference between these two modes. However, it looks like default target framework for tests on CoreClr is NetStandard20 (doesn't say that the feature will be supported at runtime), we should be able to explicitly request NetCoreApp framework. I think TargetFramework.StandardLatest would give us what we want for desktop and for CoreCLR. The expectations in the tests will have to be conditional based on the platform

RikkiGibson · 2021-05-13T23:13:50Z

Is it expected that code gen for with-expressions is also able to remove a cast when the covariant record clone is used?

cston · 2021-05-14T03:22:37Z

If the compiler generates a covariant return when compiled against a particular runtime, is the resulting assembly usable with a runtime that does not support covariant returns? If not, is there a way to opt-out of the change to the synthesized clone method?

jcouv · 2021-05-14T04:13:45Z

@cston Here's my understanding: Records shipped with .NET 5 which supports covariant returns. Using new language features on older frameworks is unsupported.
So I think the two scenarios where records exist but covariant returns don't are (1) compiler tests and (2) unsupported usages.

jaredpar · 2021-05-14T04:57:05Z

If the compiler generates a covariant return when compiled against a particular runtime, is the resulting assembly usable with a runtime that does not support covariant returns?

No but this shouldn't present a problem. We only allow covariant returns when the runtime flags in corelib indicate that covariant returns are supported. That flag is only present on net5.0 and above. It is not supported for pretty much any scenario to compile against net5.0 and then run against an older target framework. That's true irrespective of whether or not you take advantage of covariant returns.

What we need to be careful about in this change is:

Ensuring that we only emit the covariant return when on a target framework that supports it.
Working through the tricky cases
1. Mixing covariant override with base types that used non-covariant override
2. Doing a covariant override where the new type is less accessible than the base type (not sure if that is or isn't legal).
3. etc ...

Youssef1313 · 2021-05-14T06:55:05Z

Ensuring that we only emit the covariant return when on a target framework that supports it.

@jaredpar Isn't that guaranteed by checking ContainingAssembly.RuntimeSupportsCovariantReturnsOfClasses? This is now covered by tests that run on both .NET Standard and .NET 5

Mixing covariant override with base types that used non-covariant override

What should be expected here? Using covariant return even if the base type didn't?

AlekseyTs · 2021-05-14T13:39:05Z

So I think the two scenarios where records exist but covariant returns don't are (1) compiler tests and (2) unsupported usages.

I am not sure what are you suggesting. This has no effect on what compiler should do and what should be tested.

In reply to: 840994639

AlekseyTs · 2021-05-14T13:51:26Z

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

-        [InlineData(true)]
-        public void CopyCtor(bool useCompilationReference)
+        [InlineData(false, TargetFramework.Standard)]
+        [InlineData(false, TargetFramework.NetCoreApp)]


TargetFramework.NetCoreApp

Doing this is probably going to create a problem when a test is running on desktop. We will take advantage of covariant returns at build, but the runtime won't support that when we try to execute the test environment. I suggest always using TargetFramework.StandardLatest target and simply adjust test expectations accordingly based on the platform that executes the unit-test. There is a RuntimeUtilities.IsCoreClrRuntime API to detect that, for example.

@AlekseyTs From other tests that used .NET Standard only, they never had runtime support for new features. See #53416 where the tests I'm modifying there had an unreachable code path. (See #53413 where the test succeeded when I was throwing an exception in the unreachable code path).

From other tests that used .NET Standard only, they never had runtime support for new features.

There is a difference between TargetFramework.Standard and TargetFramework.StandardLatest, I believe.

Thanks @AlekseyTs!

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

AlekseyTs · 2021-05-14T14:47:10Z

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

            comp.VerifyDiagnostics(
                // (9,17): error CS0082: Type 'C' already reserves a member called 'get_P' with the same parameter types
                // record C(object P)
                Diagnostic(ErrorCode.ERR_MemberReserved, "P").WithArguments("get_P", "C").WithLocation(9, 17));

+            var expectedClone = comp.Assembly.RuntimeSupportsCovariantReturnsOfClasses


comp.Assembly.RuntimeSupportsCovariantReturnsOfClasses

I think we would want to add an assert that this value is true when RuntimeUtilities.IsCoreClrRuntime is true. Same for the other PR that fixes dead code in tests #Closed

I am suggesting doing this in every test that is supposed to observe the difference based on Assembly.RuntimeSupportsCovariantReturnsOfClasses

AlekseyTs · 2021-05-14T14:51:25Z

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

-        [Fact]
-        public void Equality_14()
+        [Theory]
+        [InlineData(TargetFramework.NetCoreApp)]


NetCoreApp

Sme comment as for the other test #Closed

AlekseyTs · 2021-05-14T14:57:07Z

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

+
+public record C(int I) : B(I);";
+
+            var compA = CreateCompilation(sourceA);


CreateCompilation

Since we are not trying to run the code, consider using target framework NetStandard20 here, rather than standard #Closed

Youssef1313 · 2021-05-14T15:32:06Z

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

+
+        [Theory, WorkItem(44902, "https://github.com/dotnet/roslyn/issues/44902")]
+        [CombinatorialData]
+        public void CrossAssemblySupportingAndNotSupportingCovariantReturns(bool useCompilationReference)


@AlekseyTs Would you mind looking at this test? Couldn't get rid of the extra diagnostics (WRN_UnifyReferenceMajMin). #Resolved

Would you mind looking at this test? Couldn't get rid of the extra diagnostics (WRN_UnifyReferenceMajMin).

It is probably unavoidable given the current set of reference assemblies. I suggest filtering them out
or suppressig in compilation options to reduce the noise.

jcouv · 2021-05-18T22:06:35Z

I am not sure what are you suggesting. This has no effect on what compiler should do and what should be tested.

@AlekseyTs I didn't make any suggestion, I was merely addressing Chuck's concern. There is no real-world & supported scenario where this change would result in an "unusable" assembly as he described it. The only times we're going to hit such situation are either compiler tests or unsupported.
Jared gave the same answer in more details.

jcouv · 2021-05-31T23:27:19Z

@Youssef1313 There's a remaining test failure in CI (CrossAssemblySupportingAndNotSupportingCovariantReturns).

jcouv · 2021-06-10T16:53:49Z

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

+    public static void Main()
+    {
+        var c = new C() { X = 1 };
+        var c2 = c with { X = 2 };


Could we extract a method for with on C and one on D, and verify IL just on that? That would make the difference more compact and clearer.

@jcouv Sorry for missing this comment. I updated the test.

Thanks. We probably no longer need to verify IL for Main (CHelper and DHelper are the important ones)

jcouv

LGTM Thanks (iteration 14) with test nit

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

AlekseyTs · 2021-06-25T03:39:44Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordClone.cs

@@ -97,8 +97,8 @@ static bool modifiersAreValid(DeclarationModifiers modifiers)

        protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplementation) MakeParametersAndBindReturnType(BindingDiagnosticBag diagnostics)
        {
-            return (ReturnType: VirtualCloneInBase() is { } baseClone ?
-                                     baseClone.ReturnTypeWithAnnotations : // Use covariant returns when available
+            return (ReturnType: VirtualCloneInBase() is { } baseClone && !ContainingAssembly.RuntimeSupportsCovariantReturnsOfClasses ?


!ContainingAssembly.RuntimeSupportsCovariantReturnsOfClasses

Consider reordering conditions. #Closed

AlekseyTs

LGTM (commit 15)

Youssef1313 · 2021-07-11T05:29:38Z

@jcouv Is this ready to merge?

jcouv · 2021-07-11T06:18:25Z

Thanks for the ping. Yes this has two sign-offs. Let's merge.
Thanks @Youssef1313

Use covariant return for synthesized record clone

23f3b94

Youssef1313 requested a review from a team as a code owner May 13, 2021 21:34

dotnet-issue-labeler bot added the Area-Compilers label May 13, 2021

cston reviewed May 13, 2021

View reviewed changes

Youssef1313 marked this pull request as draft May 13, 2021 22:20

jcouv added this to the C# 10 milestone May 14, 2021

jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label May 14, 2021

Update tests

c21e7cf

Youssef1313 added 3 commits May 14, 2021 09:26

Fix tests

54ae436

Fix test

c46f7fb

Add test

173760e

AlekseyTs reviewed May 14, 2021

View reviewed changes

Address feedback

e9c6c35

AlekseyTs reviewed May 14, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs Outdated Show resolved Hide resolved

Restore empty line

48ccab1

AlekseyTs reviewed May 14, 2021

View reviewed changes

More test fixes

37cf2a4

Youssef1313 commented May 14, 2021

View reviewed changes

Youssef1313 added 4 commits May 14, 2021 17:35

VerifyIL of clone

c08d38d

Fix test

2b0d088

Fix few tests

3723cba

Fix test

55db7da

Youssef1313 marked this pull request as ready for review May 18, 2021 18:20

Fix test

e3c369a

jcouv reviewed Jun 10, 2021

View reviewed changes

Simplify test

d224dd2

jcouv approved these changes Jun 23, 2021

View reviewed changes

Youssef1313 commented Jun 23, 2021

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs Outdated Show resolved Hide resolved

Address feedback

8941ff4

jcouv requested a review from AlekseyTs June 23, 2021 18:26

AlekseyTs reviewed Jun 25, 2021

View reviewed changes

AlekseyTs approved these changes Jun 25, 2021

View reviewed changes

Address feedback

85c48b7

Youssef1313 mentioned this pull request Jun 28, 2021

Records (C# reference)|Inheritance|with expressions in derived records – Clone does not use *covariant return type* dotnet/docs#24881

Closed

jcouv merged commit 8eb425e into dotnet:main Jul 11, 2021

ghost modified the milestones: C# 10, Next Jul 11, 2021

Youssef1313 deleted the record-clone-covariant-ret branch July 11, 2021 06:19

allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use covariant return for synthesized record clone #53404

Use covariant return for synthesized record clone #53404

Youssef1313 commented May 13, 2021

cston May 13, 2021 •

edited by jcouv

Loading

Youssef1313 May 13, 2021

jcouv May 14, 2021

Youssef1313 May 14, 2021

AlekseyTs May 14, 2021

RikkiGibson commented May 13, 2021

cston commented May 14, 2021

jcouv commented May 14, 2021

jaredpar commented May 14, 2021

Youssef1313 commented May 14, 2021 •

edited

Loading

AlekseyTs commented May 14, 2021

AlekseyTs May 14, 2021

Youssef1313 May 14, 2021

AlekseyTs May 14, 2021

Youssef1313 May 14, 2021

AlekseyTs May 14, 2021 •

edited

Loading

AlekseyTs May 14, 2021

AlekseyTs May 14, 2021 •

edited

Loading

AlekseyTs May 14, 2021 •

edited

Loading

Youssef1313 May 14, 2021 •

edited by jcouv

Loading

AlekseyTs May 14, 2021

jcouv commented May 18, 2021

jcouv commented May 31, 2021

jcouv Jun 10, 2021

Youssef1313 Jun 23, 2021

jcouv Jun 23, 2021

jcouv left a comment

AlekseyTs Jun 25, 2021 •

edited

Loading

AlekseyTs left a comment

Youssef1313 commented Jul 11, 2021

jcouv commented Jul 11, 2021


		public record C(int I) : B(I);";

		var compA = CreateCompilation(sourceA);

Use covariant return for synthesized record clone #53404

Use covariant return for synthesized record clone #53404

Conversation

Youssef1313 commented May 13, 2021

cston May 13, 2021 • edited by jcouv Loading

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

RikkiGibson commented May 13, 2021

cston commented May 14, 2021

jcouv commented May 14, 2021

jaredpar commented May 14, 2021

Youssef1313 commented May 14, 2021 • edited Loading

AlekseyTs commented May 14, 2021

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

AlekseyTs May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 14, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 14, 2021 • edited Loading

Choose a reason for hiding this comment

Youssef1313 May 14, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented May 18, 2021

jcouv commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

Youssef1313 commented Jul 11, 2021

jcouv commented Jul 11, 2021

cston May 13, 2021 •

edited by jcouv

Loading

Youssef1313 commented May 14, 2021 •

edited

Loading

AlekseyTs May 14, 2021 •

edited

Loading

AlekseyTs May 14, 2021 •

edited

Loading

AlekseyTs May 14, 2021 •

edited

Loading

Youssef1313 May 14, 2021 •

edited by jcouv

Loading

AlekseyTs Jun 25, 2021 •

edited

Loading