Skip to content

Fix (U)IntPtr construction #1861

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.

Sign up for GitHub

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

Merged
filmor merged 6 commits into from
Jul 11, 2022
+174 −26
Merged

Fix (U)IntPtr construction #1861

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c21526
Add unit tests for (U)IntPtr conversions
filmor Jul 8, 2022
004232f
Special-case construction of (U)IntPtr
filmor Jul 9, 2022
c88b1f6
Move ptr construction to ClassObject
filmor Jul 9, 2022
65a6408
Add int constructor for (U)IntPtr
filmor Jul 11, 2022
775efd5
Extend intptr conversion tests and exceptions
filmor Jul 11, 2022
ae5e074
Check (U)IntPtr size explicitly
filmor Jul 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Move ptr construction to ClassObject
  • Loading branch information
@filmor
filmor committed Jul 9, 2022
commit c88b1f6fbfe8cf585b15590122f13cd6199b9d79
32 changes: 1 addition & 31 deletions src/runtime/Converter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,36 +298,6 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType,
{
case CLRObject co:
object tmp = co.inst;
if (obType == typeof(IntPtr))
{
switch (tmp)
{
case nint val:
result = new IntPtr(val);
return true;
case Int64 val:
result = new IntPtr(val);
return true;
case Int32 val:
result = new IntPtr(val);
return true;
}
}
if (obType == typeof(UIntPtr))
{
switch (tmp)
{
case nuint val:
result = new UIntPtr(val);
return true;
case UInt64 val:
result = new UIntPtr(val);
return true;
case UInt32 val:
result = new UIntPtr(val);
return true;
}
}
if (obType.IsInstanceOfType(tmp))
{
result = tmp;
Expand Down Expand Up @@ -391,7 +361,7 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType,
// conversions (Python string -> managed string).
if (obType == objectType)
{
if (Runtime.IsStringType(value))
if (Runtime.PyString_Check(value))
{
return ToPrimitive(value, stringType, out result, setError);
}
Expand Down
7 changes: 0 additions & 7 deletions src/runtime/Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,13 +1281,6 @@ internal static bool PyFloat_Check(BorrowedReference ob)
//====================================================================
// Python string API
//====================================================================
internal static bool IsStringType(BorrowedReference op)
{
BorrowedReference t = PyObject_TYPE(op);
return (t == PyStringType)
|| (t == PyUnicodeType);
}

internal static bool PyString_Check(BorrowedReference ob)
{
return PyObject_TYPE(ob) == PyStringType;
Expand Down
117 changes: 102 additions & 15 deletions src/runtime/Types/ClassObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,9 @@ static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, Bo
// Primitive types do not have constructors, but they look like
// they do from Python. If the ClassObject represents one of the
// convertible primitive types, just convert the arg directly.
if (type.IsPrimitive || type == typeof(string))
if (type.IsPrimitive)
{
if (Runtime.PyTuple_Size(args) != 1)
{
Exceptions.SetError(Exceptions.TypeError, "no constructors match given arguments");
return default;
}

BorrowedReference op = Runtime.PyTuple_GetItem(args, 0);

if (!Converter.ToManaged(op, type, out var result, true))
{
return default;
}

return CLRObject.GetReference(result!, tp);
return NewPrimitive(tp, args, type);
}

if (type.IsAbstract)
Expand All @@ -99,6 +86,11 @@ static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, Bo
return NewEnum(type, args, tp);
}

if (type == typeof(string))
{
return NewString(args, tp);
}

if (IsGenericNullable(type))
{
// Nullable<T> has special handling in .NET runtime.
Expand All @@ -112,6 +104,101 @@ static NewReference tp_new_impl(BorrowedReference tp, BorrowedReference args, Bo
return self.NewObjectToPython(obj, tp);
}

/// <summary>
/// Construct a new .NET String object from Python args
/// </summary>
private static NewReference NewString(BorrowedReference args, BorrowedReference tp)
{
if (Runtime.PyTuple_Size(args) == 1)
{
BorrowedReference ob = Runtime.PyTuple_GetItem(args, 0);
if (Runtime.PyString_Check(ob))
{
if (Runtime.GetManagedString(ob) is string val)
return CLRObject.GetReference(val, tp);
}

// TODO: Initialise using constructors instead

Exceptions.SetError(Exceptions.TypeError, "no constructors match given arguments");
return default;
}

return default;
}

/// <summary>
/// Create a new Python object for a primitive type
///
/// The primitive types are Boolean, Byte, SByte, Int16, UInt16,
/// Int32, UInt32, Int64, UInt64, IntPtr, UIntPtr, Char, Double,
/// and Single.
///
/// All numeric types and Boolean can be handled by a simple
/// conversion, (U)IntPtr has to be handled separately as we
/// do not want to convert them automically to/from integers.
/// </summary>
/// <param name="type">.NET type to construct</param>
/// <param name="tp">Corresponding Python type</param>
/// <param name="args">Constructor arguments</param>
private static NewReference NewPrimitive(BorrowedReference tp, BorrowedReference args, Type type)
{
// TODO: Handle IntPtr
Copy link
Copy Markdown
Member

@lostmsu lostmsu Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dead TODO?

Copy link
Copy Markdown
Member Author

@filmor filmor Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll remove it.

if (Runtime.PyTuple_Size(args) != 1)
{
Exceptions.SetError(Exceptions.TypeError, "no constructors match given arguments");
return default;
}

BorrowedReference op = Runtime.PyTuple_GetItem(args, 0);
object? result = null;

if (type == typeof(IntPtr))
{
if (ManagedType.GetManagedObject(op) is CLRObject clrObject)
{
switch (clrObject.inst)
{
case nint val:
result = new IntPtr(val);
break;
case Int64 val:
result = new IntPtr(val);
break;
case Int32 val:
result = new IntPtr(val);
break;
}
}
}

if (type == typeof(UIntPtr))
{
if (ManagedType.GetManagedObject(op) is CLRObject clrObject)
{
switch (clrObject.inst)
{
case nuint val:
result = new UIntPtr(val);
break;
case UInt64 val:
result = new UIntPtr(val);
break;
case UInt32 val:
result = new UIntPtr(val);
break;
}
}
}

if (result == null && !Converter.ToManaged(op, type, out result, true))
Copy link
Copy Markdown
Member

@lostmsu lostmsu Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a test for IntPtr constructor call with this path (e.g. parameter is neither CLR object nor Python int)

Copy link
Copy Markdown
Member Author

@filmor filmor Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you expect to happen? It fails to convert, this is essentially the "old" code path. I'll add the test, though.

Copy link
Copy Markdown
Member

@lostmsu lostmsu Jul 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filmor actually, nothing specific. Just want to ensure it does not error in a weird way and the error is actually helpful.

{
return default;
}

return CLRObject.GetReference(result!, tp);
}

protected virtual void SetTypeNewSlot(BorrowedReference pyType, SlotsHolder slotsHolder)
{
TypeManager.InitializeSlotIfEmpty(pyType, TypeOffset.tp_new, new Interop.BBB_N(tp_new_impl), slotsHolder);
Expand Down