Skip to content

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Aug 15, 2018

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

Copy link
Collaborator

@iSazonov iSazonov left a 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);
Copy link
Collaborator

@iSazonov iSazonov Aug 16, 2018

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?

Copy link
Member Author

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>
Copy link
Collaborator

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.

Copy link
Member Author

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)
Copy link
Collaborator

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.

Copy link
Member Author

@daxian-dbw daxian-dbw Aug 21, 2018

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>
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

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.

Copy link
Member Author

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);
Copy link
Contributor

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.

Copy link
Member Author

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.

@daxian-dbw daxian-dbw added the Area-Maintainers-Documentation specific to documentation in this repo label Aug 21, 2018
@daxian-dbw
Copy link
Member Author

daxian-dbw commented Aug 21, 2018

@iSazonov Sorry for the late response (I'm currently on vacation :)).
Yes, I thought about implicit cast to/from Span/ReadOnlySpan. The implicit cast doesn't help regular conversion operation in PowerShell for the same root reason -- the target ByRef-like type cannot be boxed.

But when calling methods via Expression with jitting, Expression.Convert can be used to implicit cast an argument to the Span types before passing to the call (see an example below).

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

> True

PowerShell evaluates with interpretation by default, which bacially translating Expression tree to pre-defined C# code, so the Expression.Convert might not work like when Expression tree is jitted (haven't looked into it, will investigate that). If it works with interpreter too (or we can update the interpreter to make it work), we will need to update the method resolution. We currently use the same "figuring-out-conversion" method for regular conversion in powershell as well as when resolving the best matching method. The implicit cast for ByRef-like target types should continue to be "no-conversion" for regular conversion operation, but acceptable for method resolution.

Anyway, this should be done in a separate PR. I will open an issue to track it. The issue #7596 was opened to track it.

@iSazonov
Copy link
Collaborator

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

@daxian-dbw
Copy link
Member Author

@lzybkr and @iSazonov Your comments have been addressed. Please take another look when you have time. Thanks!

@iSazonov
Copy link
Collaborator

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

I get this on 6.1.0 RC1.

@daxian-dbw
Copy link
Member Author

Here is what it looks with changes from this PR:

PS:1> [System.Text.Encoding]::GetEncoding(0)


Preamble          :
BodyName          :
EncodingName      : Western European (Windows)
HeaderName        :
WebName           : windows-1252
WindowsCodePage   :
IsBrowserDisplay  :
IsBrowserSave     :
IsMailNewsDisplay :
IsMailNewsSave    :
IsSingleByte      : True
EncoderFallback   : System.Text.InternalEncoderBestFitFallback
DecoderFallback   : System.Text.InternalDecoderBestFitFallback
IsReadOnly        : True
CodePage          : 1252

Instance property access doesn't throw in powershell, even in strict mode. So accessing Preamble in the formatting code won't break it like it did previously.

@daxian-dbw
Copy link
Member Author

BTW, the AppVeyor CI PR failed with the following error

TEST FAILURES
Description: Enable-PSSession Cmdlet creates a default PSSession configuration untied to a specific PowerShell version.
Name:        Validate Enable-PSSession Cmdlet.Enable-PSSession Cmdlet creates a default PSSession configuration untied to a specific PowerShell version.
message:
Expected a value, but got $null or empty.
stack-trace:
at <ScriptBlock>, C:\projects\powershell\test\powershell\Modules\Microsoft.PowerShell.Core\PSSessionConfiguration.Tests.ps1: line 855
855:             $matchedEndpoint | Should -Not -BeNullOrEmpty
1 tests in test/powershell failed

The same failure is happening in the daily build, so it's not caused by chnages in this PR.

@iSazonov
Copy link
Collaborator

iSazonov commented Aug 24, 2018

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 Get-PSSessionConfiguration | Unregister-PSSessionConfiguration

    Describe "Validate Enable-PSSession Cmdlet" -Tags @("Feature", 'RequireAdminOnWindows') {
        BeforeAll {
            if ($IsNotSkipped) {
                Get-PSSessionConfiguration  | Unregister-PSSessionConfiguration
                Enable-PSRemoting
            }
        }

@daxian-dbw daxian-dbw merged commit 86469bd into PowerShell:master Aug 26, 2018
@daxian-dbw daxian-dbw deleted the byreflike branch August 26, 2018 19:51
@TravisEz13 TravisEz13 added this to the v6.1.0 milestone Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Maintainers-Documentation specific to documentation in this repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The latest daily throws Cannot create boxed ByRef-like values when accessing $OutputEncoding

4 participants