-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Support calling method with ByRef-like type parameters #7721
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
Conversation
iSazonov
left a comment
There was a problem hiding this 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" |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ...
BrucePay
left a comment
There was a problem hiding this 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() |
There was a problem hiding this comment.
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?
|
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 Thanks for the very useful fix! |
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