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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Dec 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64bda64
Do not bail generating base type initializer in the presence of use s…
333fred Dec 10, 2024
750c7e5
Set FailsPEVerify
333fred Dec 10, 2024
f3e0897
Only run test on coreclr
333fred Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7197,7 +7197,7 @@ internal bool TryPerformConstructorOverloadResolution(
return succeededConsideringAccessibility;
}

internal static bool ReportConstructorUseSiteDiagnostics(Location errorLocation, BindingDiagnosticBag diagnostics, bool suppressUnsupportedRequiredMembersError, in CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
internal static void ReportConstructorUseSiteDiagnostics(Location errorLocation, BindingDiagnosticBag diagnostics, bool suppressUnsupportedRequiredMembersError, in CompoundUseSiteInfo<AssemblySymbol> useSiteInfo)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lots of uses of this method, but this was the only one that did something conditional. That seemed like a bad thing to keep around, so I just made the method return void instead to prevent something like this from happening again.

{
if (suppressUnsupportedRequiredMembersError && useSiteInfo.AccumulatesDiagnostics && useSiteInfo.Diagnostics is { Count: not 0 })
{
Expand All @@ -7213,12 +7213,10 @@ internal static bool ReportConstructorUseSiteDiagnostics(Location errorLocation,

diagnostics.ReportUseSiteDiagnostic(diagnostic, errorLocation);
}

return true;
}
else
{
return diagnostics.Add(errorLocation, useSiteInfo);
diagnostics.Add(errorLocation, useSiteInfo);
}
}

Expand Down
5 changes: 1 addition & 4 deletions src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3988,10 +3988,7 @@ internal virtual BoundExpressionStatement BindConstructorInitializer(Constructor

var constructorUseSiteInfo = new CompoundUseSiteInfo<AssemblySymbol>(diagnostics, constructor.ContainingAssembly);
constructorUseSiteInfo.Add(baseConstructor.GetUseSiteInfo());
if (Binder.ReportConstructorUseSiteDiagnostics(diagnosticsLocation, diagnostics, suppressUnsupportedRequiredMembersError: constructor.HasSetsRequiredMembers, constructorUseSiteInfo))
{
return null;
}
Binder.ReportConstructorUseSiteDiagnostics(diagnosticsLocation, diagnostics, suppressUnsupportedRequiredMembersError: constructor.HasSetsRequiredMembers, constructorUseSiteInfo);

diagnostics.Add(diagnosticsLocation, useSiteInfo);

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Test/Emit3/Microsoft.CodeAnalysis.CSharp.Emit3.UnitTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
<PackageReference Include="Microsoft.DiaSymReader" />
<PackageReference Include="Basic.Reference.Assemblies.Net50" />
<PackageReference Include="Basic.Reference.Assemblies.Net60" />
<PackageReference Include="Basic.Reference.Assemblies.Net70" />
<PackageReference Include="Basic.Reference.Assemblies.Net80" />
<PackageReference Include="Basic.Reference.Assemblies.Net90" />
</ItemGroup>
Expand All @@ -35,4 +36,4 @@
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<Import Project="$(RepositoryEngineeringDir)targets\ILAsm.targets" />
</Project>
</Project>
45 changes: 45 additions & 0 deletions src/Compilers/CSharp/Test/Emit3/Semantics/RecordTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14073,6 +14073,51 @@ public static void Main()
CompileAndVerify(comp, verify: Verification.Skipped, expectedOutput: "123").VerifyDiagnostics();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/72357")]
public void CopyCtor_AssemblyWarnings()
{
var comp1 = CreateCompilation("""
public record BaseRecord
{
public required object Object1 { get; init; }
public required object Object2 { get; init; }
}
""", assemblyName: "Base", targetFramework: TargetFramework.Net70);

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

var broken = sut with { Object2 = new object()};
System.Console.Write(broken.Object1 is null);

public record DerivedRecord : BaseRecord;
""", assemblyName: "Derived", references: [comp1.EmitToImageReference()], targetFramework: TargetFramework.Net80);

var verifier = CompileAndVerify(comp2, expectedOutput: ExecutionConditionUtil.IsCoreClr ? "False" : null, verify: Verification.FailsPEVerify);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsCoreClr

I think we usually use IsMonoOrCoreClr in situations like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is specifically .NET 8, I don't think all mono environments would work, so I only did CoreClr.


// Historical note: These warnings were the cause of the bug, so they are not being suppressed
var expectedDiagnostic =
// warning CS1701: Assuming assembly reference 'System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' used by 'Base' matches identity 'System.Runtime, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' of 'System.Runtime', you may need to supply runtime policy
Diagnostic(ErrorCode.WRN_UnifyReferenceMajMin).WithArguments("System.Runtime, Version=7.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", "Base", "System.Runtime, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", "System.Runtime").WithLocation(1, 1);

verifier.VerifyDiagnostics(Enumerable.Repeat(expectedDiagnostic, 21).ToArray());

verifier.VerifyIL("DerivedRecord..ctor(DerivedRecord)", """
{
// Code size 8 (0x8)
.maxstack 2
IL_0000: ldarg.0
IL_0001: ldarg.1
IL_0002: call "BaseRecord..ctor(BaseRecord)"
IL_0007: ret
}
""");
}

[Fact]
public void Deconstruct_Simple()
{
Expand Down