Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upSupport calling method with ByRef-like type parameters #7721
Conversation
daxian-dbw
requested review from
lzybkr,
BrucePay and
iSazonov
Sep 6, 2018
daxian-dbw
added
the
Documentation Needed
label
Sep 6, 2018
daxian-dbw
self-assigned this
Sep 6, 2018
iSazonov
reviewed
Sep 7, 2018
|
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.
This comment has been minimized.
iSazonov
Sep 7, 2018
Collaborator
Ah, looks like bad UX. Moreover, now C# supports "value arguments by ref".
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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?
lzybkr
reviewed
Sep 8, 2018
| $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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
| $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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
The Travis-CI builds have been successful, but somehow the status was not reflected in the checks below. Here are the CI builds: |
daxian-dbw
merged commit 720e842
into
PowerShell:master
Sep 11, 2018
8 checks passed
daxian-dbw
deleted the
daxian-dbw:byrefparam
branch
Sep 11, 2018
This comment has been minimized.
This comment has been minimized.
|
@daxian-dbw Thanks for the very useful fix! |
daxian-dbw commentedSep 6, 2018
•
edited
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:What is enabled?
[target-type]::newsyntax, but thetarget-typeis not ByRef-like.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests