Dynamic invocation of a local function succeeds unexpectedly when there is a ref kind mismatch for a 'dynamic' argument.

[AlekseyTs]

class M
{
    static void Main()
    {
        dynamic i = 1;
        
        local(i);
        System.Console.WriteLine(i);
        Member(i); // RuntimeBinderException
        System.Console.WriteLine(i);
        
        void local(ref int x){ x++; }
    }
    
    static void Member(ref int x){ x++; }
}

Observed (https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK4wD4AEBMACAWQFgAoAb1LyrywEYA2GgFkOhgAoBKS6ik6gXgAmATxgQwUAMZ4oeALx5aAbh6C8awQBsA9lIhb2UTqv7qatAHR0AnEZOaBBAKZhgz5PdPmL12neNvc0dqLBZdfUNkZwAzWRgEPAAPTjJkgGp05TwAX008sypNOkYwwld3T2i42ESUtKTM7IKcoA==):

1
Exception
Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: The best overloaded method match for 'M.Member(ref int)' has some invalid arguments
   at CallSite.Target(Closure, CallSite, Type, Object)
   at System.Dynamic.UpdateDelegates.UpdateAndExecuteVoid2[T0,T1](CallSite site, T0 arg0, T1 arg1)
   at M.Main()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Expected:
Compilation error for local(i);.

[AlekseyTs]

There is also an existing test Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGen_DynamicTests.InvokeMember_CallSiteRefOutOmitted which causes an assert in compiler when void f(ref int a, out int b, ref dynamic c, out object d) is converted to a local function in M:

// Interpolated strings used as interpolated string handlers are allowed to match ref parameters without `ref`
Debug.Assert(parameters[i].Type is NamedTypeSymbol { IsInterpolatedStringHandlerType: true, IsValueType: true });

    System.InvalidOperationException : Assertion failed

  Stack Trace: 
ThrowingTraceListener.Fail(String message, String detailMessage) line 26
TraceInternal.Fail(String message)
Debug.Assert(Boolean condition)
LocalRewriter.GetEffectiveArgumentRefKinds(ImmutableArray`1 argumentRefKindsOpt, ImmutableArray`1 parameters) line 1166
LocalRewriter.MakeArguments(ImmutableArray`1 rewrittenArguments, Symbol methodOrIndexer, Boolean expanded, ImmutableArray`1 argsToParamsOpt, ImmutableArray`1& argumentRefKindsOpt, ArrayBuilder`1& temps, Boolean invokedAsExtensionMethod) line 1013
LocalRewriter.<VisitCall>g__visitArgumentsAndFinishRewrite|150_1(BoundCall node, BoundExpression rewrittenReceiver) line 386
LocalRewriter.VisitCall(BoundCall node) line 332
BoundCall.Accept(BoundTreeVisitor visitor) line 6081
BoundTreeVisitor.Visit(BoundNode node) line 151
BoundTreeRewriterWithStackGuard.VisitExpressionWithoutStackGuard(BoundExpression node) line 97
<30 more frames...>
RuntimeEnvironmentUtilities.EmitCompilationCore(Compilation compilation, IEnumerable`1 manifestResources, DiagnosticBag diagnostics, CompilationTestData testData, EmitOptions emitOptions) line 260
RuntimeEnvironmentUtilities.EmitCompilation(Compilation compilation, IEnumerable`1 manifestResources, List`1 dependencies, DiagnosticBag diagnostics, CompilationTestData testData, EmitOptions emitOptions) line 232
DesktopRuntimeEnvironment.Emit(Compilation mainCompilation, IEnumerable`1 manifestResources, EmitOptions emitOptions, Boolean usePdbForDebugging) line 210
CompilationVerifier.Emit(IRuntimeEnvironment testEnvironment, IEnumerable`1 manifestResources, EmitOptions emitOptions) line 481
CompilationVerifier.Emit(String expectedOutput, Boolean trimOutput, Nullable`1 expectedReturnCode, String[] args, IEnumerable`1 manifestResources, EmitOptions emitOptions, Verification peVerify, SignatureDescription[] expectedSignatures) line 269
CommonTestBase.Emit(Compilation compilation, IEnumerable`1 dependencies, IEnumerable`1 manifestResources, SignatureDescription[] expectedSignatures, String expectedOutput, Boolean trimOutput, Nullable`1 expectedReturnCode, String[] args, Action`1 assemblyValidator, Action`1 symbolValidator, EmitOptions emitOptions, Verification verify) line 188
CommonTestBase.CompileAndVerifyCommon(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 assemblyValidator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Boolean trimOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify) line 100
CSharpTestBase.CompileAndVerify(Compilation compilation, IEnumerable`1 manifestResources, IEnumerable`1 dependencies, Action`1 sourceSymbolValidator, Action`1 validator, Action`1 symbolValidator, SignatureDescription[] expectedSignatures, String expectedOutput, Boolean trimOutput, Nullable`1 expectedReturnCode, String[] args, EmitOptions emitOptions, Verification verify) line 999
CodeGen_DynamicTests.CompileAndVerifyIL(String source, String methodName, String expectedOptimizedIL, String expectedUnoptimizedIL, MetadataReference[] references, Boolean allowUnsafe, String callerPath, Int32 callerLine, CSharpParseOptions parseOptions, Verification verify) line 44
CodeGen_DynamicTests.InvokeMember_CallSiteRefOutOmitted() line 8813

[AlekseyTs]

It looks like the behavior is caused by a combination of two parts:

roslyn/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

Lines 3898 to 3905 in 182e829

	// effective RefKind has to match unless argument expression is of the type dynamic.
	// This is a bug in Dev11 which we also implement.
	// The spec is correct, this is not an intended behavior. We don't fix the bug to avoid a breaking change.
	if (!(argRefKind == parRefKind ||
	(argRefKind == RefKind.None && argument.HasDynamicType())))
	{
	return Conversion.NoConversion;
	}

roslyn/src/Compilers/CSharp/Portable/Binder/Binder_Invocation.cs

Lines 755 to 771 in 182e829

	private BoundExpression BindLocalFunctionInvocationWithDynamicArgument(
	SyntaxNode syntax,
	SyntaxNode expression,
	string methodName,
	BoundMethodGroup boundMethodGroup,
	BindingDiagnosticBag diagnostics,
	CSharpSyntaxNode queryClause,
	MethodGroupResolution resolution)
	{
	// Invocations of local functions with dynamic arguments don't need
	// to be dispatched as dynamic invocations since they cannot be
	// overloaded. Instead, we'll just emit a standard call with
	// dynamic implicit conversions for any dynamic arguments. There
	// are two exceptions: "params", and unconstructed generics. While
	// implementing those cases with dynamic invocations is possible,
	// we have decided the implementation complexity is not worth it.
	// Refer to the comments below for the exact semantics.

If the spec violation in OverloadResolution.cs is removed, it looks like only one unit-test is failing in a meaningful way.

roslyn/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDynamicTests.cs

Lines 8787 to 8812 in 182e829

	/// <summary>
	/// ref/out can be omitted at call-site.
	/// </summary>
	[Fact]
	public void InvokeMember_CallSiteRefOutOmitted()
	{
	string source = @"
	public class C
	{
	dynamic d = true;
	public void f(ref int a, out int b, ref dynamic c, out object d)
	{
	b = 1;
	d = null;
	}
	public void M()
	{
	object lo = null;
	dynamic ld;
	f(d, d, ref lo, out ld);
	}
	}";
	CompileAndVerifyIL(source, "C.M", @"

The scenario fails to compile with the following errors:

(17,11): error CS1620: Argument 1 must be passed with the 'ref' keyword
(17,14): error CS1620: Argument 2 must be passed with the 'out' keyword

Do we really want to keep this spec violation in the compiler?

[jaredpar]

Do we really want to keep this spec violation in the compiler?

I'm pretty warry about changing any dynamic behavior that is this old. It seems pretty reasonable that customers would depend on this spec violation given how much ref appears in COM APIs where dynamic is used. I'm recalling all of the office examples from the Dev10 timeframe.

[AlekseyTs]

@jaredpar

Do we really want to keep this spec violation in the compiler?

I'm pretty warry about changing any dynamic behavior that is this old.

While there is a risk of breaking something. In this scenario, we know whether we are dealing with a COM API.
The only test that "guards" the existing behavior (mentioned earlier) fails at runtime:

public class C 
{ 
     dynamic d = true; 
  
     public void f(ref int a, out int b, ref dynamic c, out object d)  
     { 
         b = 1; 
         d = null; 
     } 
      
     public void M() 
     { 
         object lo = null; 
         dynamic ld; 
  
         f(d, d, ref lo, out ld); 
     } 
      
     static void Main()
     {
         var c = new C();
         c.M();
     }
}

Microsoft.CSharp.RuntimeBinder.RuntimeBinderException: The best overloaded method match for 'C.f(ref int, out int, ref object, out object)' has some invalid arguments
   at CallSite.Target(Closure, CallSite, C, Object, Object, Object&, Object&)
   at CallSite.Target(Closure, CallSite, C, Object, Object, Object&, Object&)
   at C.M()
   at C.Main()
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

It is true that changing the behavior is likely to move the failure for legacy scenarios to compile time (vs. runtime). However, is that really that big of a break?

[AlekseyTs]

The relaxation for COM APIs is handled separately here

roslyn/src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

Lines 3313 to 3320 in 182e829

	// Omit ref feature for COM interop: We can pass arguments by value for ref parameters if we are calling a method/property on an instance of a COM imported type.
	// We must ignore the 'ref' on the parameter while determining the applicability of argument for the given method call.
	// During argument rewriting, we will replace the argument value with a temporary local and pass that local by reference.
	if (allowRefOmittedArguments && paramRefKind == RefKind.Ref && argRefKind == RefKind.None && !binder.InAttributeArgument)
	{
	hasAnyRefOmittedArgument = true;
	return RefKind.None;
	}

.

[AlekseyTs]

LDM discussed this issue on January 29th, 2024 and the decision was to go ahead with the fix without dependency on targeted language version. However, the breaking change should be documented.