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

Emit defensive copy for constrained call on in parameter #66189

Merged
merged 4 commits into from
Jan 13, 2023
Merged

Emit defensive copy for constrained call on in parameter #66189

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
86c4d13
Emit defensive copy for constrained call on `in` parameter
jcouv Jan 2, 2023
c711d2b
Address feedback
jcouv Jan 5, 2023
fa16a33
Add test
jcouv Jan 11, 2023
73767e8
Add tracked issue
jcouv Jan 11, 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
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,15 +1626,15 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
}
else
{
// calling a method defined in a base class.
// calling a method defined in a base class or interface.

// When calling a method that is virtual in metadata on a struct receiver,
// we use a constrained virtual call. If possible, it will skip boxing.
if (method.IsMetadataVirtual())
{
// NB: all methods that a struct could inherit from bases are non-mutating
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

all methods that a struct could inherit from bases are non-mutating

This assumption looks suspicious. For example, ToString when overridden in a struct can mutate the struct. #Closed

Copy link
Member Author

@jcouv jcouv Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I have a test for that (InvokeStructToStringOverrideOnInParameter) and we have existing coverage as well (CodeGenReadOnlyStructTests.ReadOnlyMethod_CallBaseMethod for non-override scenario, and other related ones). In that case we fall into the condition above (at line 1600), not here.
I'd be okay always using AddressKind.Writeable here. It's definitely safe, but a bit less optimal for ToString scenarios. Let me know what you think.

// treat receiver as ReadOnly
tempOpt = EmitReceiverRef(receiver, AddressKind.ReadOnly);
tempOpt = EmitReceiverRef(receiver, methodContainingType.IsInterface ? AddressKind.Writeable : AddressKind.ReadOnly);
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

methodContainingType.IsInterface

Why only in this case? See a comment above #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.

Thanks much for suggesting the scenario where an override is added to the struct.
That showed that the assumption was broken. Removed it.

Copy link
Contributor

@AlekseyTs AlekseyTs Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

tempOpt = EmitReceiverRef(receiver, methodContainingType.IsInterface ? AddressKind.Writeable : AddressKind.ReadOnly);

Does this change also affect readonly fields? #Closed

Copy link
Member Author

@jcouv jcouv Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Readonly fields are not affected by this change. Here we're only dealing with calls to a method.
We clarified offline. The question is about using a readonly field as the receiver. If C# is affected, maybe VB is too.

Copy link
Member Author

@jcouv jcouv Jan 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did some investigation. It is indeed possible to hit this with a readonly field as well.

On the VB side, it's a bit confusing because the AddressKind enum values have different meaning than the C# AddressKinds of the same name, but as far as I could tell we didn't have the same problem. Added tests.

It's worth noting that in VB, the For Each generates a conversion (boxing) instead of a constrained call, so I couldn't get exactly the same scenario that was broken in C#.

callKind = CallKind.ConstrainedCallVirt;
}
else
Expand Down
188 changes: 188 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenInParametersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4386,5 +4386,193 @@ .locals init (Test V_0)
}
");
}

[Fact, WorkItem(66135, "https://github.com/dotnet/roslyn/issues/66135")]
public void ConstrainedCallOnInParameter()
{
var source = @"
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

public class C
{
public static void Main()
{
S value = new();
ref readonly S valueRef = ref value;
Console.Write(valueRef);
M(in valueRef);
Console.Write(valueRef);
}
public static void M(in S value)
{
foreach (var x in value) { }
}
}

public struct S : IEnumerable<int>
{
int a;
public readonly override string ToString() => a.ToString();
private IEnumerator<int> GetEnumerator() => Enumerable.Range(0, ++a).GetEnumerator();
IEnumerator<int> IEnumerable<int>.GetEnumerator() => GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}";
var verifier = CompileAndVerify(source, expectedOutput: "00");
// Note: we use a temp instead of directly doing a constrained call on `in` parameter
Copy link
Member

@jjonescz jjonescz Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we use a temp even when the target method is marked as readonly? #Closed

Copy link
Member Author

@jcouv jcouv Jan 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not possible to have exactly this scenario with the only modification being an additional readonly because this scenario uses an interface and readonly is only allowed in struct types (not in interfaces).
Prior to the code change, we'd produce a direct constrained call, so the comment calls out what is important to observe in the IL.

That said, it should be possible to emit a pattern-based GetEnumerator call directly on the struct (ie. foreach without IEnumerable interface). I'll add a test for that. On second thought, that scenario isn't so interesting, as it will not cover the modified test (it'll go down a different branch of the same method, see line 1600).

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, amended my earlier comment

verifier.VerifyIL("C.M", """
{
// Code size 51 (0x33)
.maxstack 1
.locals init (System.Collections.Generic.IEnumerator<int> V_0,
S V_1)
IL_0000: ldarg.0
IL_0001: ldobj "S"
IL_0006: stloc.1
IL_0007: ldloca.s V_1
IL_0009: constrained. "S"
IL_000f: callvirt "System.Collections.Generic.IEnumerator<int> System.Collections.Generic.IEnumerable<int>.GetEnumerator()"
IL_0014: stloc.0
.try
{
IL_0015: br.s IL_001e
IL_0017: ldloc.0
IL_0018: callvirt "int System.Collections.Generic.IEnumerator<int>.Current.get"
IL_001d: pop
IL_001e: ldloc.0
IL_001f: callvirt "bool System.Collections.IEnumerator.MoveNext()"
IL_0024: brtrue.s IL_0017
IL_0026: leave.s IL_0032
}
finally
{
IL_0028: ldloc.0
IL_0029: brfalse.s IL_0031
IL_002b: ldloc.0
IL_002c: callvirt "void System.IDisposable.Dispose()"
IL_0031: endfinally
}
IL_0032: ret
}
""");
}

[Fact, WorkItem(66135, "https://github.com/dotnet/roslyn/issues/66135")]
public void ConstrainedCallOnInParameter_ConstrainedGenericReceiver()
{
var source = @"
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

public class C
{
public static void Main()
{
S value = new();
ref readonly S valueRef = ref value;
Console.Write(valueRef);
M(in valueRef);
Console.Write(valueRef);
}
public static void M<T>(in T value) where T : struct, IEnumerable<int>
{
foreach (var x in value) { }
}
}

public struct S : IEnumerable<int>
{
int a;
public readonly override string ToString() => a.ToString();
private IEnumerator<int> GetEnumerator() => Enumerable.Range(0, ++a).GetEnumerator();
IEnumerator<int> IEnumerable<int>.GetEnumerator() => GetEnumerator();
IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
}";
var verifier = CompileAndVerify(source, expectedOutput: "00");
verifier.VerifyIL("C.M<T>(in T)", """
{
// Code size 51 (0x33)
.maxstack 1
.locals init (System.Collections.Generic.IEnumerator<int> V_0,
T V_1)
IL_0000: ldarg.0
IL_0001: ldobj "T"
IL_0006: stloc.1
IL_0007: ldloca.s V_1
IL_0009: constrained. "T"
IL_000f: callvirt "System.Collections.Generic.IEnumerator<int> System.Collections.Generic.IEnumerable<int>.GetEnumerator()"
IL_0014: stloc.0
.try
{
IL_0015: br.s IL_001e
IL_0017: ldloc.0
IL_0018: callvirt "int System.Collections.Generic.IEnumerator<int>.Current.get"
IL_001d: pop
IL_001e: ldloc.0
IL_001f: callvirt "bool System.Collections.IEnumerator.MoveNext()"
IL_0024: brtrue.s IL_0017
IL_0026: leave.s IL_0032
}
finally
{
IL_0028: ldloc.0
IL_0029: brfalse.s IL_0031
IL_002b: ldloc.0
IL_002c: callvirt "void System.IDisposable.Dispose()"
IL_0031: endfinally
}
IL_0032: ret
}
""");
}

[Fact, WorkItem(66135, "https://github.com/dotnet/roslyn/issues/66135")]
public void InvokeStructToStringOverrideOnInParameter()
{
var text = @"
using System;

class C
{
public static void Main()
{
S1 s = new S1();
Console.Write(M(in s));
Console.Write(M(in s));
}
static string M(in S1 s)
{
return s.ToString();
}
}
struct S1
{
int i;
public override string ToString() => (i++).ToString();
}
";

var comp = CompileAndVerify(text, expectedOutput: "00");

comp.VerifyIL("C.M", """
{
// Code size 21 (0x15)
.maxstack 1
.locals init (S1 V_0)
IL_0000: ldarg.0
IL_0001: ldobj "S1"
IL_0006: stloc.0
IL_0007: ldloca.s V_0
IL_0009: constrained. "S1"
IL_000f: callvirt "string object.ToString()"
IL_0014: ret
}
""");
}

}
}
79 changes: 79 additions & 0 deletions src/Compilers/CSharp/Test/Symbol/Symbols/DefaultInterfaceImplementationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68913,5 +68913,84 @@ event System.Action E2 {add{} remove{}}
Diagnostic(ErrorCode.ERR_ComImportWithImpl, "int").WithArguments("I1.implicit operator int(I1)", "I1").WithLocation(28, 30)
);
}

[Fact, WorkItem(66135, "https://github.com/dotnet/roslyn/issues/66135")]
public void ConstrainedCallOnInParameter_DefaultImplementation()
{
var source = @"
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

public class C
{
public static void Main()
{
S value = new();
ref readonly S valueRef = ref value;
Console.Write(valueRef);
M(in valueRef);
Console.Write(valueRef);
}
public static void M(in S value)
{
foreach (var x in value) { }
}
}

public interface MyEnumerable : IEnumerable<int>
{
IEnumerator<int> GetEnumeratorCore();
IEnumerator<int> IEnumerable<int>.GetEnumerator() => GetEnumeratorCore();
IEnumerator IEnumerable.GetEnumerator() => GetEnumeratorCore();
}

public struct S : MyEnumerable
{
int a;
public readonly override string ToString() => a.ToString();
public IEnumerator<int> GetEnumeratorCore() => Enumerable.Range(0, ++a).GetEnumerator();
}";
var verifier = CompileAndVerify(source, targetFramework: TargetFramework.Net70,
expectedOutput: Execute(isStatic: false) ? "00" : null,
verify: Verify(isStatic: false));

verifier.VerifyIL("C.M", """
{
// Code size 51 (0x33)
.maxstack 1
.locals init (System.Collections.Generic.IEnumerator<int> V_0,
S V_1)
IL_0000: ldarg.0
IL_0001: ldobj "S"
IL_0006: stloc.1
IL_0007: ldloca.s V_1
IL_0009: constrained. "S"
IL_000f: callvirt "System.Collections.Generic.IEnumerator<int> System.Collections.Generic.IEnumerable<int>.GetEnumerator()"
IL_0014: stloc.0
.try
{
IL_0015: br.s IL_001e
IL_0017: ldloc.0
IL_0018: callvirt "int System.Collections.Generic.IEnumerator<int>.Current.get"
IL_001d: pop
IL_001e: ldloc.0
IL_001f: callvirt "bool System.Collections.IEnumerator.MoveNext()"
IL_0024: brtrue.s IL_0017
IL_0026: leave.s IL_0032
}
finally
{
IL_0028: ldloc.0
IL_0029: brfalse.s IL_0031
IL_002b: ldloc.0
IL_002c: callvirt "void System.IDisposable.Dispose()"
IL_0031: endfinally
}
IL_0032: ret
}
""");
}
}
}