Skip to content

Fix scoped variance checks involving ref struct interface implementation #78883

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

Merged
merged 4 commits into from
Jun 12, 2025
Merged

Fix scoped variance checks involving ref struct interface implementation #78883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41f61cd
WIP
RikkiGibson Jun 10, 2025
e807f76
handle readonly method case
RikkiGibson Jun 11, 2025
fbc8e31
explicit implementation
RikkiGibson Jun 11, 2025
56d3997
fix tests
RikkiGibson Jun 11, 2025
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2286,7 +2286,7 @@ private static void CheckParameterModifierMismatchMethodConversion(SyntaxNode sy
return;
}

if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(delegateMethod))
if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(delegateMethod, lambdaOrMethod.TryGetThisParameter(out var thisParameter) ? thisParameter : null))
{
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
delegateMethod,
Expand Down
7 changes: 4 additions & 3 deletions ...pilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol_ImplementationChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ static void checkValidMethodOverride(
MethodSymbol overridingMethod,
BindingDiagnosticBag diagnostics)
{
if (RequiresValidScopedOverrideForRefSafety(overriddenMethod))
if (RequiresValidScopedOverrideForRefSafety(overriddenMethod, overridingMethod.TryGetThisParameter(out var thisParam) ? thisParam : null))
{
CheckValidScopedOverride(
overriddenMethod,
Expand Down Expand Up @@ -1378,7 +1378,7 @@ static bool isValidNullableConversion(
/// Returns true if the method signature must match, with respect to scoped for ref safety,
/// in overrides, interface implementations, or delegate conversions.
/// </summary>
internal static bool RequiresValidScopedOverrideForRefSafety(MethodSymbol? method)
internal static bool RequiresValidScopedOverrideForRefSafety(MethodSymbol? method, ParameterSymbol? overrideThisParameter)
{
if (method is null)
{
Expand All @@ -1404,7 +1404,8 @@ internal static bool RequiresValidScopedOverrideForRefSafety(MethodSymbol? metho
// - The method returns a `ref struct` or returns a `ref` or `ref readonly`, or the method has a `ref` or `out` parameter of `ref struct` type, and
// ...
int nRefParametersRequired;
if (method.ReturnType.IsRefLikeOrAllowsRefLikeType() ||
if ((overrideThisParameter is { RefKind: RefKind.Ref or RefKind.Out } && overrideThisParameter.Type.IsRefLikeOrAllowsRefLikeType()) ||
Copy link
Member Author

Choose a reason for hiding this comment

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

I do not believe there is a scenario where the this parameter of an interface implementation is an out parameter. Generally only a this parameter of a constructor is an out parameter. But, it seems correct in principle to try to detect it here.

Copy link
Member

Choose a reason for hiding this comment

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

The IsRefLikeOrAllowsRefLikeType call checks for both a ref struct and a type parameter with the allows ref struct anti-constraint. Is that latter check necessary? I would assume we only need IsRefLikeType check here. Is there a case I'm missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we expect the this-parameter of an override/implementation to be a type parameter ever. We could use the IsRefLikeType check and assert that we don't have a type parameter here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am going to keep the code as-is because if a this-parameter of type-parameter type hypothetically made it into here, it feels reasonable to handle it in the way this implementation is handling it. It doesn't seem like a condition where we have to rethink everything if the language changes to make it possible to occur one day.

Copy link
Member

Choose a reason for hiding this comment

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

Should we also check scoped mismatch of the receiver in CheckValidScopedOverride?

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not think there is a need to do this. To my understanding, the only way to adjust the scoped-ness of a this parameter is to use [UnscopedRef], and an unsafe difference between [UnscopedRef] on interface and implementation is already blocked. For example: https://github.com/dotnet/roslyn/blob/71cc05dad5f064ebe57814a727bed959737306f2/src/Compilers/CSharp/Test/Emit3/RefStructInterfacesTests.cs#L941

I believe similar enforcement already exists for other forms of unsafe [UnscopedRef] differences.

method.ReturnType.IsRefLikeOrAllowsRefLikeType() ||
(method.RefKind is RefKind.Ref or RefKind.RefReadOnly))
{
nRefParametersRequired = 1;
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,7 +1873,7 @@ static void checkMethodOverride(
reportMismatchInParameterType,
(implementingType, isExplicit));

if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(implementedMethod))
if (SourceMemberContainerTypeSymbol.RequiresValidScopedOverrideForRefSafety(implementedMethod, implementingMethod.TryGetThisParameter(out var thisParameter) ? thisParameter : null))
{
SourceMemberContainerTypeSymbol.CheckValidScopedOverride(
implementedMethod,
Expand Down
157 changes: 157 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11300,5 +11300,162 @@ public static void M({{scoped2}} {{modifier}} R r) { }
""";
CreateCompilation(source).VerifyDiagnostics();
}

[Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78711")]
[InlineData("public void UseSpan(Span<int> span)", 17)]
[InlineData("void I.UseSpan(Span<int> span)", 12)]
public void RefStructInterface_ScopedDifference(string implSignature, int column)
{
// This is a case where interface methods need to be treated specially in scoped variance checks.
// Because a ref struct can implement the interface, we need to compare the signatures as if the interface has a receiver parameter which is ref-to-ref-struct.
var source = $$"""
using System;

interface I
{
void UseSpan(scoped Span<int> span);
}

ref struct RS : I
{
public Span<int> Span { get; set; }
public RS(Span<int> span) => Span = span;

{{implSignature}} // 1
{
this.Span = span;
}
}
""";

var comp = CreateCompilation(source, targetFramework: TargetFramework.Net90);
comp.VerifyEmitDiagnostics(
// (13,17): error CS8987: The 'scoped' modifier of parameter 'span' doesn't match overridden or implemented member.
// public void UseSpan(Span<int> span)
Diagnostic(ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation, "UseSpan").WithArguments("span").WithLocation(13, column));
}

[Theory, WorkItem("https://github.com/dotnet/roslyn/issues/78711")]
[InlineData("public readonly void UseSpan(Span<int> span)")]
[InlineData("readonly void I.UseSpan(Span<int> span)")]
public void RefStructInterface_ScopedDifference_ReadonlyImplementation(string implSignature)
{
var source = $$"""
using System;

interface I
{
void UseSpan(scoped Span<int> span);
}

ref struct RS : I
{
public Span<int> Span { get; set; }
public RS(Span<int> span) => Span = span;

{{implSignature}}
{
Span = span; // 1
}
}
""";

var comp = CreateCompilation(source, targetFramework: TargetFramework.Net90);
comp.VerifyEmitDiagnostics(
// (15,9): error CS1604: Cannot assign to 'Span' because it is read-only
// Span = span; // 1
Diagnostic(ErrorCode.ERR_AssgReadonlyLocal, "Span").WithArguments("Span").WithLocation(15, 9));
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/78711")]
public void SimilarScenarioAs78711InvolvingNonReceiverParameter()
{
// Demonstrate a scenario similar to https://github.com/dotnet/roslyn/issues/78711 involving a non-receiver parameter has consistent behavior
// In this case, the interface and implementation parameters cannot differ by type. But it as close as we can get to the receiver parameter case.
var source = """
using System;

interface I
{
void UseSpan1(ref RS rs, scoped Span<int> span);
void UseSpan2(ref readonly RS rs, scoped Span<int> span);
}

class C : I
{
public void UseSpan1(ref RS rs, Span<int> span) // 1
{
rs.Span = span;
}

public void UseSpan2(ref readonly RS rs, Span<int> span)
{
rs.Span = span; // 2
}
}

ref struct RS
{
public Span<int> Span { get; set; }
public RS(Span<int> span) => Span = span;
}
""";

var comp = CreateCompilation(source, targetFramework: TargetFramework.Net90);
comp.VerifyEmitDiagnostics(
// (11,17): error CS8987: The 'scoped' modifier of parameter 'span' doesn't match overridden or implemented member.
// public void UseSpan1(ref RS rs, Span<int> span) // 1
Diagnostic(ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation, "UseSpan1").WithArguments("span").WithLocation(11, 17),
// (18,9): error CS8332: Cannot assign to a member of variable 'rs' or use it as the right hand side of a ref assignment because it is a readonly variable
// rs.Span = span; // 2
Diagnostic(ErrorCode.ERR_AssignReadonlyNotField2, "rs.Span").WithArguments("variable", "rs").WithLocation(18, 9));
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/78711")]
public void Repro_78711()
{
var source = """
using System;

public static class Demo
{
public static void Show()
{
var stru = new Stru();

Unsafe(ref stru);

Console.Out.WriteLine(stru.Span);
}

private static void Unsafe<T>(ref T stru) where T : IStru, allows ref struct
{
Span<char> span = stackalloc char[10];

"bug".CopyTo(span);

stru.UseSpan(span);
}
}

internal interface IStru
{
public void UseSpan(scoped Span<char> span);
}

internal ref struct Stru : IStru
{
public Span<char> Span;

public void UseSpan(Span<char> span) => Span = span; // 1
}
""";

var comp = CreateCompilation(source, targetFramework: TargetFramework.Net90);
comp.VerifyEmitDiagnostics(
// (33,17): error CS8987: The 'scoped' modifier of parameter 'span' doesn't match overridden or implemented member.
// public void UseSpan(Span<char> span) => Span = span;
Diagnostic(ErrorCode.ERR_ScopedMismatchInParameterOfOverrideOrImplementation, "UseSpan").WithArguments("span").WithLocation(33, 17));
}
}
}
Loading