Skip to content

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Sep 6, 2018

PR Summary

Fix #7596

Support calling methods with ByRef-like type parameters in PowerShell, as long as the argument specified for the parameter can be implicitly/explicitly converted to the ByRef-like type.

We cannot create an instance of a ByRef-like type in PowerShell, but there are types that can be implicitly or explicitly converted to ByRef-like types, such as T[] -> Span<T>, T[] -> ReadOnlySpan<T>, String -> ReadOnlySpan<T>. ArraySegment<T> -> Span<T>, ArraySegment<T> -> ReadOnlySpan<T>. With changes in this PR, we can call methods with ByRef-like parameter types by providing the arguments of the types that can be casted to the target ByRef-like type. For example:

PS:5> $charArray = "F:\".ToCharArray()
PS:6> [System.IO.Path]::IsPathRooted($charArray)
True

What is enabled?

  1. Invoking methods that have ByRef-like parameters, but the return type is not ByRef-like.
  2. Invoking constructors with ByRef-like parameters via [target-type]::new syntax, but the target-type is not ByRef-like.
  3. Accessing indexers that have ByRef-like parameters, but the return type is not ByRef-like.

PR Checklist

@daxian-dbw daxian-dbw added the Documentation Needed in this repo Documentation is needed in this repo label Sep 6, 2018
@daxian-dbw daxian-dbw self-assigned this Sep 6, 2018
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with one comment.

Also it might make sense to show in the tests that if we pass an argument by reference, then we can return its changed value.

It "Support method calls with ByRef-like parameter as long as the argument can be casted to the ByRef-like type" {
$testObj.PrintMySpan("abc", "def") | Should -BeExactly "abc3"
$testObj.PrintMySpan("abc", "Hello".ToCharArray()) | Should -BeExactly "abc5"
{ $testObj.PrintMySpan("abc", 12) } | Should -Throw -ErrorId "MethodArgumentConversionInvalidCastArgument"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, looks like bad UX. Moreover, now C# supports "value arguments by ref".

Copy link
Member Author

Choose a reason for hiding this comment

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

now C# supports "value arguments by ref".

Can you please elaborate that? Do you mean MethodName(ref int id)? If so, then powershell already supports it via [ref].

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems I did not understand my idea myself:-)

PowerShell like to convert to/from string - 1 + "2" returns 3. So if we accept a string in the test I'd expect that we convert the int to string and ReadOnlySpan<char>.
Perhaps there is more complex scenario. If a parameter type is Span I'd expect that we can able to convert the int to Span too and accept array of int-s.

If this is possible, you can do this in the next PR.

Copy link
Member Author

@daxian-dbw daxian-dbw Sep 7, 2018

Choose a reason for hiding this comment

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

I don't think that's a good idea. Passing arguments to a ByRef-like parameter should be explicit, and I don't think we should do a second layer conversion from the argument to the types that might be able to implicitly/explicitly cast to the target ByRef-like parameter type.

An extreme example: string can implicitly cast to ReadOnlySpan<char>, and any value can be converted to a string in powershell.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the implicit cast in C# what is a difference string from ReadOnlySpan<char>? Currently CoreFX has overloads with string parameter as wrapper on methods with ReadOnlySpan<char>. We can free use any of the overloads - no difference in result.
Now let's look PowerShell behavior.
function Test1([string] $var) and function Test1([int] $var) both accepts 123 and "123".
If they will be methods we see the same.
Then if it is a binary method with string parameter we see the same. But if it is ReadOnlySpan<char> we get an error.
Seems as minimum we should consider a string as special case.
As maximum perhaps we can discover that there is more common scenarios.

Copy link
Member Author

Choose a reason for hiding this comment

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

The purpose of this PR is to make PowerShell able to interact with libraries that only exposes APIs with ByRef-like parameters. When there is an equivalent wrapper API, there is no point to call the ByRef-like parameter based API in PowerShell.

As for the conversion, string and ReadOnlySpan<char> are two different types with different semantics, and having implicit casing support between them doesn't change that. PowerShell doesn't support two-layer indirect conversion, for example, PowerShell supports [FileInfo] -> [string], and [string] -> [type], but [FileInfo] -> [type] will fail, even if the FileInfo object can be converted to a string value that can be converted to a type:

$a = [System.IO.FileInfo]::new("string")
$b = [string] $a
$b
> string

$c = [type] $b ## success
$d = [type] $a ## fail
Cannot convert the "string" value of type "System.IO.FileInfo" to type "System.Type".
At line:1 char:1
+ $d = [type] $a
+ ~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidArgument: (:) [], RuntimeException
+ FullyQualifiedErrorId : ConvertToFinalInvalidCastException

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, then it's best to explicitly indicate in the documentation why this does not work otherwise there will be a lot of questions.

$result = [DotNetInterop.Test2]::new("abc")
$result.Name | Should -BeExactly "Number of chars: 3"

{ [DotNetInterop.Test2]::new(12) } | Should -Throw -ErrorId "MethodCountCouldNotFindBest"
Copy link
Collaborator

@iSazonov iSazonov Sep 7, 2018

Choose a reason for hiding this comment

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

The same. new("abc") vs new(12)

Copy link
Member Author

Choose a reason for hiding this comment

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

new(12) is to use an integer to test the "not castable" scenario. What is your suggestion to change?

$typeXmlFile = Join-Path -Path $TestDrive -ChildPath "TestType.ps1xml"
Set-Content -Path $typeXmlFile -Value $typeXml -Encoding Ascii
$ps = [powershell]::Create()
$ps.AddCommand("Update-TypeData").AddParameter("AppendPath", $typeXmlFile).Invoke()
Copy link
Contributor

Choose a reason for hiding this comment

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

Update-TypeData can be used without creating a file if you'd prefer.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed - much easier. Or is there some reason to use the file?

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 particular reason, I just went with the file without considering other options ;) I will change to simply the test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to use Update-TypeData -TypeName -MemberType ...

Copy link
Collaborator

@BrucePay BrucePay left a comment

Choose a reason for hiding this comment

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

LGTM

$typeXmlFile = Join-Path -Path $TestDrive -ChildPath "TestType.ps1xml"
Set-Content -Path $typeXmlFile -Value $typeXml -Encoding Ascii
$ps = [powershell]::Create()
$ps.AddCommand("Update-TypeData").AddParameter("AppendPath", $typeXmlFile).Invoke()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed - much easier. Or is there some reason to use the file?

@daxian-dbw
Copy link
Member Author

The Travis-CI builds have been successful, but somehow the status was not reflected in the checks below. Here are the CI builds:
https://travis-ci.org/PowerShell/PowerShell/builds/426886335

@daxian-dbw daxian-dbw merged commit 720e842 into PowerShell:master Sep 11, 2018
@daxian-dbw daxian-dbw deleted the byrefparam branch September 11, 2018 23:57
@iSazonov
Copy link
Collaborator

@daxian-dbw Thanks for the very useful fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When calling method, we should be theoretically able to pass an argument to a ByRef-like parameter via implicit casting

5 participants