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

Jump to bottom

Add more ref readonly tests and address comments #69183

Merged
merged 8 commits into from
Jul 28, 2023
Merged

Add more ref readonly tests and address comments #69183

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4dac215
Test more signature matching scenarios
jjonescz Jul 24, 2023
72bd42c
Assert that mismatch flag is not set without considering differences
jjonescz Jul 24, 2023
ee9a3e4
Extract signature matching logic
jjonescz Jul 24, 2023
21f8e0b
Mention issue about missing `modreq(In)` on delegate `Invoke`
jjonescz Jul 24, 2023
6735e5e
Test operators, expression trees, IOperation
jjonescz Jul 24, 2023
5c91a21
Generalize ref matching mode assert
jjonescz Jul 25, 2023
098afe9
Verify invocation in expression trees
jjonescz Jul 25, 2023
8806f45
Test unary operator
jjonescz Jul 25, 2023
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
23 changes: 19 additions & 4 deletions src/Compilers/CSharp/Portable/Symbols/MemberSignatureComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,9 @@ private MemberSignatureComparer(
_typeComparison = typeComparison;
Debug.Assert((_typeComparison & TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly) == 0,
$"Rely on the {nameof(refKindCompareMode)} flag to set this to ensure all cases are handled.");
Debug.Assert((_refKindCompareMode & RefKindCompareMode.AllowRefReadonlyVsInMismatch) == 0 ||
(_refKindCompareMode & RefKindCompareMode.ConsiderDifferences) != 0,
$"Cannot set {nameof(RefKindCompareMode.AllowRefReadonlyVsInMismatch)} without {nameof(RefKindCompareMode.ConsiderDifferences)}.");
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Consider making the assert more general. I.e. either _refKindCompareMode == DoNotConsiderDifferences, or the ConsiderDifferences flag is set. #Closed

if ((refKindCompareMode & RefKindCompareMode.ConsiderDifferences) == 0)
{
_typeComparison |= TypeCompareKind.FunctionPointerRefMatchesOutInRefReadonly;
Expand Down Expand Up @@ -765,10 +768,7 @@ private static bool HaveSameParameterTypes(ImmutableArray<ParameterSymbol> param
// Metadata signatures don't distinguish ref/out, but C# does - even when comparing metadata method signatures.
if ((refKindCompareMode & RefKindCompareMode.ConsiderDifferences) != 0)
{
if (refKind1 != refKind2 &&
!((refKindCompareMode & RefKindCompareMode.AllowRefReadonlyVsInMismatch) != 0 &&
((refKind1 == RefKind.RefReadOnlyParameter && refKind2 == RefKind.In) ||
(refKind1 == RefKind.In && refKind2 == RefKind.RefReadOnlyParameter))))
if (!areRefKindsCompatible(refKindCompareMode, refKind1, refKind2))
{
return false;
}
Expand All @@ -783,6 +783,21 @@ private static bool HaveSameParameterTypes(ImmutableArray<ParameterSymbol> param
}

return true;

static bool areRefKindsCompatible(RefKindCompareMode refKindCompareMode, RefKind refKind1, RefKind refKind2)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks. This was much easier to follow :-)

{
if (refKind1 == refKind2)
{
return true;
}

if ((refKindCompareMode & RefKindCompareMode.AllowRefReadonlyVsInMismatch) != 0)
{
return (refKind1, refKind2) is (RefKind.RefReadOnlyParameter, RefKind.In) or (RefKind.In, RefKind.RefReadOnlyParameter);
}

return false;
}
}

private static TypeWithAnnotations SubstituteType(TypeMap typeMap, TypeWithAnnotations typeSymbol)
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceDelegateMethodSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ internal override LexicalSortKey GetLexicalSortKey()
protected override bool HasSetsRequiredMembersImpl => false;
}

// Note that `in`/`ref readonly` parameters currently don't have `modreq(In)`
// even though the `Invoke` method is virtual. https://github.com/dotnet/roslyn/issues/69079
private sealed class InvokeMethod : SourceDelegateMethodSymbol
{
private readonly ImmutableArray<CustomModifier> _refCustomModifiers;
Expand Down
152 changes: 152 additions & 0 deletions src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,73 @@ static void verify(ModuleSymbol m)
}
}

[Fact]
public void Operators()
{
var source = """
class C
{
public static C operator+(ref readonly C x, C y) => x;
public static implicit operator C(ref readonly int x) => null;
public static explicit operator C(ref readonly int x) => null;
}
""";
CreateCompilation(source).VerifyDiagnostics(
// (3,29): error CS0631: ref and out are not valid in this context
// public static C operator+(ref readonly C x, C y) => x;
Diagnostic(ErrorCode.ERR_IllegalRefParam, "+").WithLocation(3, 29),
// (4,37): error CS0631: ref and out are not valid in this context
// public static implicit operator C(ref readonly int x) => null;
Diagnostic(ErrorCode.ERR_IllegalRefParam, "C").WithLocation(4, 37),
// (5,37): error CS0631: ref and out are not valid in this context
// public static explicit operator C(ref readonly int x) => null;
Diagnostic(ErrorCode.ERR_IllegalRefParam, "C").WithLocation(5, 37),
// (5,37): error CS0557: Duplicate user-defined conversion in type 'C'
// public static explicit operator C(ref readonly int x) => null;
Diagnostic(ErrorCode.ERR_DuplicateConversionInClass, "C").WithArguments("C").WithLocation(5, 37));
}

[Fact]
public void ExpressionTrees()
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ExpressionTrees

I think we also should test calling methods with ref readonly parameters inside an expression tree, With all possible call site modifiers, including none. Also, passing an rValue case. #Closed

{
var source = """
using System;
using System.Linq.Expressions;

Expression<D> e1 = (ref readonly int p) => C.M(in p);
Expression<D> e2 = (ref readonly int p) => C.M(ref p);
Expression<D> e3 = (ref readonly int p) => C.M(p);
Expression<D> e4 = (int p) => C.M(in p);
Expression<Action<int>> e5 = (int p) => C.M(in p);

delegate void D(ref readonly int p);

static class C
{
public static void M(ref readonly int x) { }
}
""";
CreateCompilation(source).VerifyDiagnostics(
// (4,38): error CS1951: An expression tree lambda may not contain a ref, in or out parameter
// Expression<D> e1 = (ref readonly int p) => C.M(in p);
Diagnostic(ErrorCode.ERR_ByRefParameterInExpressionTree, "p").WithLocation(4, 38),
// (5,52): error CS8329: Cannot use variable 'p' as a ref or out value because it is a readonly variable
// Expression<D> e2 = (ref readonly int p) => C.M(ref p);
Diagnostic(ErrorCode.ERR_RefReadonlyNotField, "p").WithArguments("variable", "p").WithLocation(5, 52),
// (6,38): error CS1951: An expression tree lambda may not contain a ref, in or out parameter
// Expression<D> e3 = (ref readonly int p) => C.M(p);
Diagnostic(ErrorCode.ERR_ByRefParameterInExpressionTree, "p").WithLocation(6, 38),
// (6,48): warning CS9506: Argument 1 should be passed with the 'in' keyword
// Expression<D> e3 = (ref readonly int p) => C.M(p);
Diagnostic(ErrorCode.WRN_ArgExpectedIn, "p").WithArguments("1").WithLocation(6, 48),
// (7,20): error CS1661: Cannot convert lambda expression to type 'Expression<D>' because the parameter types do not match the delegate parameter types
// Expression<D> e4 = (int p) => C.M(in p);
Diagnostic(ErrorCode.ERR_CantConvAnonMethParams, "(int p) => C.M(in p)").WithArguments("lambda expression", "System.Linq.Expressions.Expression<D>").WithLocation(7, 20),
// (7,25): error CS1676: Parameter 1 must be declared with the 'ref readonly' keyword
// Expression<D> e4 = (int p) => C.M(in p);
Diagnostic(ErrorCode.ERR_BadParamRef, "p").WithArguments("1", "ref readonly").WithLocation(7, 25));
}

[Fact]
public void Delegate()
{
Expand Down Expand Up @@ -3239,6 +3306,67 @@ static void Main()
Diagnostic(ErrorCode.ERR_NoCorrespondingArgument, "M").WithArguments("d", "C.M(ref readonly System.DateTime)").WithLocation(8, 9));
}

[Theory, CombinatorialData]
public void OperationTree([CombinatorialValues("ref ", "in ", "")] string modifier)
{
var source = $$"""
class C
{
void M(ref readonly int p) { }
void M2(int x)
/*<bind>*/{
M({{modifier}}x);
}/*</bind>*/
}
""";
var comp = CreateCompilation(source);

VerifyOperationTreeAndDiagnosticsForTest<BlockSyntax>(comp, $$"""
IBlockOperation (1 statements) (OperationKind.Block, Type: null) (Syntax: '{ ... }')
IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'M({{modifier}}x);')
Expression:
IInvocationOperation ( void C.M(ref readonly System.Int32 p)) (OperationKind.Invocation, Type: System.Void) (Syntax: 'M({{modifier}}x)')
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: ContainingTypeInstance) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'M')
Arguments(1):
IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: p) (OperationKind.Argument, Type: null) (Syntax: '{{modifier}}x')
IParameterReferenceOperation: x (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'x')
InConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
OutConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
""",
modifier == ""
? new[]
{
// (6,11): warning CS9503: Argument 1 should be passed with 'ref' or 'in' keyword
// M(x);
Diagnostic(ErrorCode.WRN_ArgExpectedRefOrIn, "x").WithArguments("1").WithLocation(6, 11)
}
: DiagnosticDescription.None);

VerifyFlowGraphForTest<BlockSyntax>(comp, $$"""
Block[B0] - Entry
Statements (0)
Next (Regular) Block[B1]
Block[B1] - Block
Predecessors: [B0]
Statements (1)
IExpressionStatementOperation (OperationKind.ExpressionStatement, Type: null) (Syntax: 'M({{modifier}}x);')
Expression:
IInvocationOperation ( void C.M(ref readonly System.Int32 p)) (OperationKind.Invocation, Type: System.Void) (Syntax: 'M({{modifier}}x)')
Instance Receiver:
IInstanceReferenceOperation (ReferenceKind: ContainingTypeInstance) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'M')
Arguments(1):
IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: p) (OperationKind.Argument, Type: null) (Syntax: '{{modifier}}x')
IParameterReferenceOperation: x (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'x')
InConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
OutConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
Next (Regular) Block[B2]
Block[B2] - Exit
Predecessors: [B1]
Statements (0)
""");
}

[Fact]
public void Invocation_VirtualMethod()
{
Expand Down Expand Up @@ -4606,6 +4734,30 @@ static void Main()
Diagnostic(ErrorCode.WRN_OverridingDifferentRefness, "M2").WithArguments("in int x", "ref readonly int x").WithLocation(9, 12));
}

[Fact]
public void Implementation_RefReadonly_In_Close()
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Implementation_RefReadonly_In_Close

What is the goal of the test? Should we test success scenario instead by making the methods in C public? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the point is to test MemberSignatureComparer.CSharpCloseImplicitImplementationComparer

{
var source = """
interface I
{
void M1(in int x);
void M2(ref readonly int x);
}
class C : I
{
void M1(ref readonly int x) { }
void M2(in int x) { }
}
""";
CreateCompilation(source).VerifyDiagnostics(
// (6,11): error CS0737: 'C' does not implement interface member 'I.M1(in int)'. 'C.M1(ref readonly int)' cannot implement an interface member because it is not public.
// class C : I
Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberNotPublic, "I").WithArguments("C", "I.M1(in int)", "C.M1(ref readonly int)").WithLocation(6, 11),
// (6,11): error CS0737: 'C' does not implement interface member 'I.M2(ref readonly int)'. 'C.M2(in int)' cannot implement an interface member because it is not public.
// class C : I
Diagnostic(ErrorCode.ERR_CloseUnimplementedInterfaceMemberNotPublic, "I").WithArguments("C", "I.M2(ref readonly int)", "C.M2(in int)").WithLocation(6, 11));
}

[Fact]
public void Implementation_RefReadonly_In_Indexer()
{
Expand Down
31 changes: 31 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/InterceptorsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3060,6 +3060,37 @@ static class D
Diagnostic(ErrorCode.WRN_InterceptorSignatureMismatch, @"InterceptsLocation(""Program.cs"", 19, 11)").WithArguments("C.Method2(System.IntPtr)", "D.Interceptor2(C, nint)").WithLocation(30, 6));
}

[Fact]
public void SignatureMismatch_09()
{
var source = """
using System.Runtime.CompilerServices;
using System;

static class Program
{
public static void InterceptableMethod(ref readonly int x) => Console.Write("interceptable " + x);

public static void Main()
{
int x = 5;
InterceptableMethod(in x);
}
}

static class D
{
[InterceptsLocation("Program.cs", 11, 9)]
public static void Interceptor(in int x) => Console.Write("interceptor " + x);
}
""";
var comp = CreateCompilation(new[] { (source, "Program.cs"), s_attributesSource }, parseOptions: TestOptions.RegularPreview.WithFeature("InterceptorsPreview"));
comp.VerifyEmitDiagnostics(
// Program.cs(17,6): error CS9144: Cannot intercept method 'Program.InterceptableMethod(ref readonly int)' with interceptor 'D.Interceptor(in int)' because the signatures do not match.
// [InterceptsLocation("Program.cs", 11, 9)]
Diagnostic(ErrorCode.ERR_InterceptorSignatureMismatch, @"InterceptsLocation(""Program.cs"", 11, 9)").WithArguments("Program.InterceptableMethod(ref readonly int)", "D.Interceptor(in int)").WithLocation(17, 6));
}

[Fact]
public void ScopedMismatch_01()
{
Expand Down