-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Handle operations with ByRef-like types gracefully in PowerShell #7533
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we document the change?
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.
Added the document label.
| <value>Creating instances of attribute and delegated Windows RT types is not supported.</value> | ||
| </data> | ||
| <data name="CannotInstantiateBoxedByRefLikeType" xml:space="preserve"> | ||
| <value>Cannot create instances of the ByRef-like type "{0}". ByRef-like types cannot be boxed and should be used only on stack.</value> |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use C# 7.0 syntax.
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 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.
| <value>Cannot convert value to type "{0}". Only core types are supported in this language mode.</value> | ||
| </data> | ||
| <data name="InvalidCastToByRefLikeType" xml:space="preserve"> | ||
| <value>Cannot convert to the ByRef-like type "{0}". ByRef-like types cannot be boxed and should be used only on stack.</value> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also test an indexer.
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.
Oh, I forgot to handle PSGetIndexerBinder, will update.
| private static Dictionary<ConversionTypePair, ConversionData> s_converterCache | ||
| = new Dictionary<ConversionTypePair, ConversionData>(256); | ||
| private static Dictionary<ConversionTypePair, ConversionData> s_converterCache = new Dictionary<ConversionTypePair, ConversionData>(256); | ||
| private static ConversionData<object> s_noConversion = new ConversionData<object>(ConvertNoConversion, ConversionRank.None); |
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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