★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/7721Nouvelle recherche | Portail wanayoo
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

Support calling method with ByRef-like type parameters #7721

Merged
merged 2 commits into from Sep 11, 2018

Conversation

Projects
None yet
5 participants
@daxian-dbw
Copy link
Member

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 requested review from lzybkr, BrucePay and iSazonov Sep 6, 2018

@daxian-dbw daxian-dbw self-assigned this Sep 6, 2018

@iSazonov
Copy link
Collaborator

left a comment

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"

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 7, 2018

Collaborator

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

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 7, 2018

Author Member

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].

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 7, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 7, 2018

Author Member

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.

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 8, 2018

Collaborator

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.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 10, 2018

Author Member

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 10, 2018

Collaborator

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"

This comment has been minimized.

Copy link
@iSazonov

iSazonov Sep 7, 2018

Collaborator

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

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 7, 2018

Author Member

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()

This comment has been minimized.

Copy link
@lzybkr

lzybkr Sep 8, 2018

Member

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

This comment has been minimized.

Copy link
@BrucePay

BrucePay Sep 10, 2018

Collaborator

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

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 10, 2018

Author Member

No particular reason, I just went with the file without considering other options ;) I will change to simply the test.

This comment has been minimized.

Copy link
@daxian-dbw

daxian-dbw Sep 10, 2018

Author Member

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

@BrucePay
Copy link
Collaborator

left a comment

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()

This comment has been minimized.

Copy link
@BrucePay

BrucePay Sep 10, 2018

Collaborator

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

@daxian-dbw

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2018

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

8 checks passed

CodeFactor 39 issues fixed. 25 issues found.
Details
PowerShell-CI-linux #PR-7721-20180910.01 succeeded
Details
PowerShell-CI-macos #PR-7721-20180910.01 succeeded
Details
PowerShell-CI-windows #PR-7721-20180910.01 succeeded
Details
WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

@daxian-dbw daxian-dbw deleted the daxian-dbw:byrefparam branch Sep 11, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 13, 2018

@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
You can’t perform that action at this time.