Handle operations with ByRef-like types gracefully in PowerShell#7533
Handle operations with ByRef-like types gracefully in PowerShell#7533daxian-dbw merged 4 commits intoPowerShell:masterfrom
Conversation
iSazonov
left a comment
There was a problem hiding this comment.
@daxian-dbw Do you consider auto cast to/from Span types?
| } | ||
| else | ||
| { | ||
| parseOptions = CSharpParseOptions.Default.WithLanguageVersion(LanguageVersion.Latest); |
There was a problem hiding this comment.
Should we document the change?
There was a problem hiding this comment.
Added the document label.
There was a problem hiding this comment.
I'd remove the mention of "stack" - seems no useful information for script writer.
ByRef-like types cannot be used in PowerShell Core scripts.
There was a problem hiding this comment.
Will update the message to be ByRef-like types are not supported in PowerShell.
|
|
||
| ConstructorInfo constructorInfo = null; | ||
| MethodInfo methodInfo = mi as MethodInfo; | ||
| if (methodInfo != null) |
There was a problem hiding this comment.
We could use C# 7.0 syntax.
There was a problem hiding this comment.
I tried the is syntax first and got a parsing error in the code below where methodInfo is used: Use of unassigned local variable 'methodInfo'. So I changed it back to as syntax.
There was a problem hiding this comment.
should implies other uses are possible. You could keep it simple and just say those types are not supported in PowerShell because it might not be obvious to most that PowerShell boxes everything.
There was a problem hiding this comment.
Will update the message to ... ByRef-like types are not supported in PowerShell.
| { | ||
| public class Test | ||
| { | ||
| public static Span<int> Space |
There was a problem hiding this comment.
Maybe also test an indexer.
There was a problem hiding this comment.
Oh, I forgot to handle PSGetIndexerBinder, will update.
There was a problem hiding this comment.
I'm not sure it's worth caching this instance. It should be rarely needed and multiple instances are fine.
There was a problem hiding this comment.
My bad. This static field is not used anywhere. I will remove it.
|
@iSazonov Sorry for the late response (I'm currently on vacation :)). But when calling methods via Expression with jitting, var arg = @"e:\abc\def";
var method = typeof(Path).GetMethod(nameof(Path.IsPathRooted), new Type[] { typeof(ReadOnlySpan<char>) });
var body = Expression.Call(method, Expression.Convert(Expression.Constant(arg), typeof(ReadOnlySpan<char>)));
var func = Expression.Lambda<Func<bool>>(body, null).Compile();
var rest = func();
Console.WriteLine(rest);
> TruePowerShell evaluates with interpretation by default, which bacially translating Expression tree to pre-defined C# code, so the Anyway, this should be done in a separate PR. |
|
@daxian-dbw Thanks for depth comment! I think support dynamically converting Span based methods is important because main direction in .Net Core is to implement methods with Span parameters and other methods only wrap them. Later we can get APIs without the wrapped methods in CoreFX or third party libraries and PowerShell will not be able tocall them. So my fisrt think was that we could generate the wrap methods in runtime. |
|
Will it resolve: [System.Text.Encoding]::GetEncoding(0)
format-default : Cannot create boxed ByRef-like values.
+ CategoryInfo : NotSpecified: (:) [format-default], InvalidProgramException
+ FullyQualifiedErrorId : System.InvalidProgramException,Microsoft.PowerShell.Commands.FormatDefaultCommandI get this on 6.1.0 RC1. |
|
Here is what it looks with changes from this PR: Instance property access doesn't throw in powershell, even in strict mode. So accessing |
|
BTW, the AppVeyor CI PR failed with the following error The same failure is happening in the daily build, so it's not caused by chnages in this PR. |
|
I guess that remoting enpoints is created when we install PowerShell Core on base CI system but then test a preview version. So possible fix is to remove endpoints with Describe "Validate Enable-PSSession Cmdlet" -Tags @("Feature", 'RequireAdminOnWindows') {
BeforeAll {
if ($IsNotSkipped) {
Get-PSSessionConfiguration | Unregister-PSSessionConfiguration
Enable-PSRemoting
}
} |
PR Summary
Fix #7301
Handle operations with ByRef-like types gracefully in PowerShell.
ByRef-like types are supposed to be used on stack only, so we need to fail gracefully when accessing properties, calling methods, or creating objects related to ByRef-like types.
Found one issue when working on this fix, tracked by #7534
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