Skip to content
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

Allow method and property invocations of signatures containing pointer types #15767

Open
SeeminglyScience opened this issue Jul 13, 2021 · 10 comments
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif WG-Engine core PowerShell engine, interpreter, and runtime WG-Language parser, language semantics

Comments

@SeeminglyScience
Copy link
Collaborator

SeeminglyScience commented Jul 13, 2021

Summary of the new feature / enhancement

As someone who writes in C#, I want to be able to use PowerShell to prototype as much as possible. Currently, this comes to a grinding halt whenever a pointer (void* or T*) is involved. It is not possible to enable this outside of the engine with custom type conversions, but it is possible from within the engine itself.

Proposed technical implementation details (optional)

Binder support

On a whim I gave adding support for "boxing" void* to System.Reflection.Pointer to see if it were feasible. Turns out with just 9 additional lines, the engine can get very minimal support:

diff --git a/src/System.Management.Automation/engine/CoreAdapter.cs b/src/System.Management.Automation/engine/CoreAdapter.cs
index 48077bbaf..ef3c5c573 100644
--- a/src/System.Management.Automation/engine/CoreAdapter.cs
+++ b/src/System.Management.Automation/engine/CoreAdapter.cs
@@ -841,6 +841,11 @@ namespace System.Management.Automation
             Type fromType = null;
             ConversionRank rank = ConversionRank.None;

+            if (parameterType.IsPointer && argument is Pointer boxedPointer)
+            {
+                return ConversionRank.Assignable;
+            }
+
             if (allowCastingToByRefLikeType && parameterType.IsByRefLike)
             {
                 // When resolving best method for use in binders, we can accept implicit/explicit casting conversions to
diff --git a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs
index 275529b7b..6690823dd 100644
--- a/src/System.Management.Automation/engine/runtime/Binding/Binders.cs
+++ b/src/System.Management.Automation/engine/runtime/Binding/Binders.cs
@@ -262,6 +262,11 @@ namespace System.Management.Automation.Language
                 return Expression.Call(CachedReflectionInfo.PSObject_Base, target.Expression.Cast(typeof(PSObject)));
             }

+            if (parameterType.IsPointer && argType == typeof(Pointer))
+            {
+                return Expression.Call(typeof(Pointer).GetMethod("Unbox"), target.Expression.Cast(typeof(Pointer)));
+            }
+
             // If the conversion can't fail, skip wrapping the conversion in try/catch so we generate less code.
             if (parameterType.IsAssignableFrom(argType))
             {
@@ -6927,6 +6932,11 @@ namespace System.Management.Automation.Language
                     return new DynamicMetaObject(expr, restrictions);
                 }

+                if (expr.Type.IsPointer)
+                {
+                    expr = Expression.Call(typeof(Pointer).GetMethod("Box"), expr, Expression.Constant(expr.Type));
+                }
+
                 expr = expr.Cast(typeof(object));

                 // Likewise, when calling methods in types defined by PowerShell, we don't

After this, the following works:

Add-Type -CompilerOptions '-unsafe' -TypeDefinition '
    public static unsafe class Test
    {
        public static void* Get() => (void*)System.Runtime.InteropServices.Marshal.AllocHGlobal(4);
    }'

$ptr = [Test]::Get()
[IntPtr]::new($ptr)
# 140702897041244

Now the above diff isn't exactly the best way to go about it, but it shows that adding support to the binder is now pretty easy with CAS gone.


PSPointer API Proposal (click to expand)

There are a lot more hurdles to getting approval for a public API like this then just lighting up support in the binder, so this is more of a "stretch goal".

Why

System.Reflection.Pointer is by design pretty bare. Pretty much all you can do with it is unbox it, which makes sense, but makes it less desirable for the use case of prototyping.

API

namespace System.Managment.Automation
{
	public class PSPointer
	{
        private protected PSPointer(nint value)

		public static unsafe PSPointer Box(void* ptr);

		public static unsafe PSPointer<T> Box(T* ptr) where T : unmanaged;

		public static unsafe PSPointer Box(void* ptr, Type type);

		public static bool operator ==(PSPointer left, PSPointer right);

		public static bool operator !=(PSPointer left, PSPointer right);

		public static bool operator <(PSPointer left, PSPointer right);

		public static bool operator >(PSPointer left, PSPointer right);

		public static PSPointer operator +(PSPointer left, PSPointer right);

		public static PSPointer operator -(PSPointer left, PSPointer right);

		public static implicit operator PSPointer(int value);

		public static implicit operator PSPointer(nint value);

        public nint Value { get; }

		public override string ToString();

		// This method will be hidden in PowerShell since it would just return
		// a new instance of itself.
		[Hidden]
		public unsafe void* Unbox();
	}

    public sealed class PSPointer<T> : PSPointer
	{
		public PSPointer(nint value);

		public T this[int index] { get; set; }

		// This method will be hidden in PowerShell since it would just return
		// a new instance of itself.
		[Hidden]
		public new unsafe T* Unbox();
	}
}
@SeeminglyScience SeeminglyScience added Issue-Enhancement the issue is more of a feature request than a bug Needs-Triage The issue is new and needs to be triaged by a work group. labels Jul 13, 2021
@iSazonov
Copy link
Collaborator

We would need something like Set-StrictMode -EnableKillFeatures 😸

@iSazonov iSazonov added Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif WG-Engine core PowerShell engine, interpreter, and runtime WG-Language parser, language semantics and removed Issue-Enhancement the issue is more of a feature request than a bug labels Jul 13, 2021
@SeeminglyScience
Copy link
Collaborator Author

SeeminglyScience commented Jul 13, 2021

We would need something like Set-StrictMode -EnableKillFeatures 😸

Nah nothing like that is needed. This isn't about supporting things like stackalloc or ref 😁

I went ahead and changed the example to not confuse the issue, this isn't really any different than what you can already do with Marshal and IntPtr.

@fMichaleczek
Copy link

nice hack !

@SeeminglyScience
Copy link
Collaborator Author

SeeminglyScience commented Jul 13, 2021

Thanks! Note that it's not really a hack. I mean it is, but this is also how the reflection API's in general handle pointers. The engine just doesn't account for them.

@fMichaleczek
Copy link

Obviously, it was hack in the noble sense of the term 🙂

@SeeminglyScience
Copy link
Collaborator Author

SeeminglyScience commented Apr 29, 2022

The Engine WG discussed this and agree it would be good to enable these scenarios under an experimental feature if it is easy to do so.

I'm going to leave up for grabs off of this one because the benefit to complexity ratio is very delicate here. Some effort should be taken to introduce the least amount of maintenance requirements that still enables this scenario.

@SeeminglyScience SeeminglyScience removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Apr 29, 2022
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

2 similar comments
Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

Copy link
Contributor

This issue has not had any activity in 6 months, if this is a bug please try to reproduce on the latest version of PowerShell and reopen a new issue and reference this issue if this is still a blocker for you.

@microsoft-github-policy-service microsoft-github-policy-service bot added Resolution-No Activity Issue has had no activity for 6 months or more labels Nov 16, 2023
Copy link
Contributor

This issue has been marked as "No Activity" as there has been no activity for 6 months. It has been closed for housekeeping purposes.

@SeeminglyScience SeeminglyScience removed the Resolution-No Activity Issue has had no activity for 6 months or more label Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Discussion the issue may not have a clear classification yet. The issue may generate an RFC or may be reclassif WG-Engine core PowerShell engine, interpreter, and runtime WG-Language parser, language semantics
Projects
None yet
Development

No branches or pull requests

3 participants