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.

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

Fix up .NET method invocation with Optional arg #21387

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -7167,6 +7167,13 @@ internal static Expression InvokeMethod(MethodBase mi, DynamicMetaObject target,
{
argExprs[i] = Expression.Default(parameterType);
}
else if (!parameters[i].HasDefaultValue && parameterType != typeof(object) && argValue == Type.Missing)
{
// If the method contains just [Optional] without a default value set then we cannot use
// Type.Missing as a placeholder. Instead we use the default value for that type. Only
// exception to this rule is when the parameter type is object.
argExprs[i] = Expression.Default(parameterType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please reword the comment.
It seems your comment in the PR description is more clear about behavior with object

When it comes to the compiled assembly the ParameterInfo.DefaultValue for an argument with only [Optional] is System.Reflection.Missing but this value cannot be passed as an argument unless the type is object because it cannot be casted to the argument type. Instead using null or the default value for that type should be used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the comment, please let me know if there's any further improvements or clarifications it should have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

Please remove unrelated style and formatting changes. Our docs ask to pull separate PRs such changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah sorry, that last host I made this change on was set to format on save, will undo those.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

}
else
{
// We don't specify the parameter type in the constant expression. Normally the default
Expand Down
151 changes: 151 additions & 0 deletions test/powershell/engine/Basic/CLRBinding.Tests.ps1
@@ -0,0 +1,151 @@
# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

Describe ".NET Method Binding Tests" -tags CI {
BeforeAll {
Add-Type -TypeDefinition @'
using System;
using System.Runtime.InteropServices;

namespace CLRBindingTests;

public class TestClass
{
public int Prop { get; }

public TestClass(int value = 1)
{
Prop = value;
}

public static string StaticWithDefaultExpected() => StaticWithDefault();
public static string StaticWithDefault(string value = "foo") => value;

public static string StaticWithOptionalAndValueExpected() => StaticWithOptionalAndValue();
public static string StaticWithOptionalAndValue([Optional, DefaultParameterValue("bar")] string value) => value;

public static string StaticWithOptionalExpected() => StaticWithOptional();
public static string StaticWithOptional([Optional] string value) => value;

public object InstanceWithDefaultExpected() => InstanceWithDefault();
public object InstanceWithDefault(object value = null) => value;

public object InstanceWithOptionalAndValueExpected() => InstanceWithOptionalAndValue();
public object InstanceWithOptionalAndValue([Optional, DefaultParameterValue("foo")] object value) => value;

public object InstanceWithOptionalExpected() => InstanceWithOptional();
public object InstanceWithOptional([Optional] object value) => value;

public string MultipleArgsWithDefaultExpected(string prefix) => MultipleArgsWithDefault(prefix);
public string MultipleArgsWithDefault(string prefix, string extra = "abc") => $"{prefix}{extra}";

public string MultipleArgsWithOptionalAndValueExpected(string prefix) => MultipleArgsWithOptionalAndValue(prefix);
public string MultipleArgsWithOptionalAndValue(string prefix, [Optional, DefaultParameterValue("def")] string extra) => $"{prefix}{extra}";

public string MultipleArgsWithOptionalExpected(string prefix) => MultipleArgsWithOptional(prefix);
public string MultipleArgsWithOptional(string prefix, [Optional] string extra) => $"{prefix}{extra}";
}

public class TestClassCstorWithOptionalAndValue
{
public int Prop { get; }

public TestClassCstorWithOptionalAndValue([Optional, DefaultParameterValue(2)] int value)
{
Prop = value;
}
}

public class TestClassCstorWithOptional
{
public int Prop { get; }

public TestClassCstorWithOptional([Optional] int value)
{
Prop = value;
}
}
'@
}

It "Binds to constructor with default argument" {
$c = [CLRBindingTests.TestClass]::new()
$c.Prop | Should -Be 1
}

It "Binds to constructor with Optional with DefaultValue argument" {
$c = [CLRBindingTests.TestClassCstorWithOptionalAndValue]::new()
$c.Prop | Should -Be 2
}

It "Binds to constructor with Optional argument" {
$c = [CLRBindingTests.TestClassCstorWithOptional]::new()
$c.Prop | Should -Be 0
}

It "Binds to static method with default argument" {
$expected = [CLRBindingTests.TestClass]::StaticWithDefaultExpected()
$actual = [CLRBindingTests.TestClass]::StaticWithDefault()
$actual | Should -Be $expected
}

It "Binds to static method with Optional with DefaultValue argument" {
$expected = [CLRBindingTests.TestClass]::StaticWithOptionalAndValueExpected()
$actual = [CLRBindingTests.TestClass]::StaticWithOptionalAndValue()
$actual | Should -Be $expected
}

It "Binds to static method with Optional argument" {
$expected = [CLRBindingTests.TestClass]::StaticWithOptionalExpected()
$actual = [CLRBindingTests.TestClass]::StaticWithOptional()
$actual | Should -Be $expected
}

It "Binds to instance method with default argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.InstanceWithDefaultExpected()
$actual = $c.InstanceWithDefault()
$actual | Should -Be $expected
}

It "Binds to instance method with Optional with DefaultValue argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.InstanceWithOptionalAndValueExpected()
$actual = $c.InstanceWithOptionalAndValue()
$actual | Should -Be $expected
}

It "Binds to instance method with Optional argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.InstanceWithOptionalExpected()
$actual = $c.InstanceWithOptional()
$actual | Should -Be $expected
}

It "Binds to instance method with normal arg and default argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.MultipleArgsWithDefaultExpected("prefix")
$actual = $c.MultipleArgsWithDefault("prefix")
$actual | Should -Be $expected
}

It "Binds to instance method with Optional with normal arg and DefaultValue argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.MultipleArgsWithOptionalAndValueExpected("prefix")
$actual = $c.MultipleArgsWithOptionalAndValue("prefix")
$actual | Should -Be $expected
}

It "Binds to instance method with normal arg and Optional argument" {
$c = [CLRBindingTests.TestClass]::new()

$expected = $c.MultipleArgsWithOptionalExpected("prefix")
$actual = $c.MultipleArgsWithOptional("prefix")
$actual | Should -Be $expected
}
}