Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cloning a .NET 8 record deriving from a .NET 7 record leads to loss of members #72357

Closed
Watno opened this issue Mar 1, 2024 · 2 comments · Fixed by #76347
Closed

Cloning a .NET 8 record deriving from a .NET 7 record leads to loss of members #72357

Watno opened this issue Mar 1, 2024 · 2 comments · Fixed by #76347
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Records Records
Milestone

Comments

@Watno
Copy link

Watno commented Mar 1, 2024

Version Used:
Compiler "4.8.0-7.24067.24 (2635711)"

Steps to Reproduce:

  1. Create a project targeting .NET 7 and define a record class
<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFramework>net7.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
    </PropertyGroup>

</Project>
public record BaseRecord
{
    public required object Object1 { get; init; }
    public required object Object2 { get; init; }
}
  1. Create a project targeting .NET 8 and define a derived record class there
<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <TargetFramework>net8.0</TargetFramework>
        <OutputType>Exe</OutputType>
    </PropertyGroup>

    <ItemGroup>
      <ProjectReference Include="..\DemoBase\DemoBase.csproj" />
    </ItemGroup>

</Project>
public record DerivedRecord : BaseRecord
{
}
  1. Instantiate the derived record and clone it using with
using System.Diagnostics;

var sut = new DerivedRecord
{
    Object1 = new object(),
    Object2 = new object()
};

var broken = sut with { Object2 = new object()}; 

Debug.Assert(broken.Object1 != null);

Demo.zip
https://gist.github.com/Watno/f6d78b49a15530e69a2a1cc1ce8d1e40

Expected Behavior: Members of the base record get cloned to the copy

Actual Behavior: Members of the base record disappear

Additional information: The IL code of the constructor of DerivedRecord seems to be broken:

  .method family hidebysig specialname rtspecialname instance void
    .ctor(
      class DerivedRecord original
    ) cil managed
  {
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor()
      = (01 00 00 00 )
    .custom instance void [System.Runtime]System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute::.ctor()
      = (01 00 00 00 )
    .maxstack 8

    // [1 15 - 1 28]
    IL_0000: nop
    IL_0001: ret

  } // end of method DerivedRecord::.ctor
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 1, 2024
@jaredpar jaredpar added Bug Feature - Records Records and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 8, 2024
@jaredpar jaredpar added this to the 17.10 milestone Mar 8, 2024
@jaredpar
Copy link
Member

jaredpar commented Mar 8, 2024

@jcouv PTAL

@jcouv jcouv modified the milestones: 17.10, 17.11 Mar 26, 2024
@jaredpar jaredpar modified the milestones: 17.11, 17.13 Jul 23, 2024
@arunchndr arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024
@jaredpar jaredpar modified the milestones: 17.13 P2, 17.13 Dec 2, 2024
@333fred
Copy link
Member

333fred commented Dec 9, 2024

Some notes from when I was looking at this earlier today from a ping from a discord user: both required and the lower TFM appear to be necessary to reproduce. Something is causing us to not call the base type's copy constructor.

333fred added a commit to 333fred/roslyn that referenced this issue Dec 10, 2024
…ite warnings

We were returning early from generating a call to the base record copy constructor initializer if use site diagnostics reported any kind of diagnostic, including warnings. This isn't good in general, but is particularly bad for warnings like CS1701, which are typically suppressed by the SDK and results in what seems like a completely clean csc invocation skipping calling the base constructor. Fixes dotnet#72357.
@333fred 333fred assigned 333fred and unassigned jcouv Dec 10, 2024
@jaredpar jaredpar modified the milestones: 17.13, 17.14 Dec 10, 2024
@333fred 333fred added the 4 - In Review A fix for the issue is submitted for review. label Dec 10, 2024
333fred added a commit that referenced this issue Dec 11, 2024
…ite warnings (#76347)

* Do not bail generating base type initializer in the presence of use site warnings

We were returning early from generating a call to the base record copy constructor initializer if use site diagnostics reported any kind of diagnostic, including warnings. This isn't good in general, but is particularly bad for warnings like CS1701, which are typically suppressed by the SDK and results in what seems like a completely clean csc invocation skipping calling the base constructor. Fixes #72357.

* Set FailsPEVerify

* Only run test on coreclr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Compilers Bug Feature - Records Records
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants