Fix (U)IntPtr construction #1861

filmor · 2022-07-08T18:39:03Z

What does this implement/fix? Explain your changes.

Allows creating an IntPtr from Python. This is currently impossible, because we rely on automatic conversion (as IntPtr is a primitive type) while we don't actually implement one for this type (rightly so!).

The relevant code section is:

pythonnet/src/runtime/Types/ClassObject.cs

Lines 73 to 89 in 87d4db9

    
           if (type.IsPrimitive || type == typeof(string)) 
        
           { 
        
               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); 
        
           }

Does this close any currently open issues?

#1116

Any other comments?

...

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Ensure you have signed the .NET Foundation CLA
Add yourself to AUTHORS
Updated the CHANGELOG

filmor · 2022-07-09T14:07:32Z

I'm not entirely happy with this, yet. I think we might want to do the special-casing in ClassObject instead, like it's done for enums and bigints. Thoughts?

lostmsu · 2022-07-10T02:19:40Z

Re: IntPtr

Why can't we just treat IntPtr as non-primitive type? Then its constructor will be invoked by normal means.

lostmsu · 2022-07-10T02:22:21Z

Maybe we should have an explicit set of types for which this constructor masking behavior should be used, instead of relying on IsPrimitive which was created for a totally different purpose.

filmor · 2022-07-10T08:56:02Z

Re: IntPtr

Why can't we just treat IntPtr as non-primitive type? Then its constructor will be invoked by normal means.

Because then it's always default constructed, apparently calling a cobstructor on a primitive type after it has been created "uninitialised" is a no-op. I can check again, though I'm quite certain that that was among the first things I've tried.

lostmsu · 2022-07-11T03:29:34Z

apparently calling a cobstructor on a primitive type after it has been created "uninitialised" is a no-op

@filmor this would be highly problematic depending on what types this behavior applies to. If it is just IsPrimitive types, then we just need to adjust handling explicitly. However, if it is the case for any value types, we are in trouble.

UPD. Just checked rc2 for the record, values types in general seem fine: TimeSpan(3, 2, 1).ToString() prints '03:02:01'.

IntPtr gives "TypeError: No match found for given type params"

tests/test_conversion.py

lostmsu · 2022-07-11T16:46:18Z

@filmor this PR was rushed. There are few artifacts of development left in the final code, the latest commits never reviewed, and the earliest not reviewed completely. This would be OK if there was no active response for a few days, but simply bypassing review on new code is not a good idea.

lostmsu · 2022-07-11T16:54:48Z

src/runtime/Types/ClassObject.cs

+                }
+            }
+
+            if (result == null && !Converter.ToManaged(op, type, out result, true))


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

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

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

lostmsu · 2022-07-11T17:27:54Z

src/runtime/Types/ClassObject.cs

+        /// <param name="args">Constructor arguments</param>
+        private static NewReference NewPrimitive(BorrowedReference tp, BorrowedReference args, Type type)
+        {
+            // TODO: Handle IntPtr


True, I'll remove it.

filmor · 2022-07-11T17:28:57Z

@filmor this PR was rushed. There are few artifacts of development left in the final code, the latest commits never reviewed, and the earliest not reviewed completely. This would be OK if there was no active response for a few days, but simply bypassing review on new code is not a good idea.

It might have been a bit rushed, but that was mostly because I wanted to tackle the other remaining issues (regressions from the constructor refactor) and get a new rc out soon for people to test with. This one would have actually bitten myself for a change if we had released this as stable. :)

filmor mentioned this pull request Jul 8, 2022

Release 3.0 meta-issue #1860

Closed

23 tasks

filmor changed the title ~~Add unit tests for (U)IntPtr conversions~~ Fix (U)IntPtr conversions Jul 8, 2022

filmor changed the title ~~Fix (U)IntPtr conversions~~ Fix (U)IntPtr construction Jul 8, 2022

Add unit tests for (U)IntPtr conversions

1c21526

filmor force-pushed the fix-intptr-conversion branch from 54e5a7d to 1c21526 Compare July 8, 2022 18:41

Special-case construction of (U)IntPtr

004232f

filmor requested a review from lostmsu July 9, 2022 14:05

filmor marked this pull request as ready for review July 9, 2022 14:05

Move ptr construction to ClassObject

c88b1f6

lostmsu reviewed Jul 11, 2022

View reviewed changes

tests/test_conversion.py Outdated Show resolved Hide resolved

filmor added 2 commits July 11, 2022 11:41

Add int constructor for (U)IntPtr

65a6408

Extend intptr conversion tests and exceptions

775efd5

filmor force-pushed the fix-intptr-conversion branch 2 times, most recently from a6cdc35 to 43f748b Compare July 11, 2022 12:23

Check (U)IntPtr size explicitly

ae5e074

filmor force-pushed the fix-intptr-conversion branch from 43f748b to ae5e074 Compare July 11, 2022 12:30

filmor merged commit 332f8e7 into pythonnet:release Jul 11, 2022

filmor deleted the fix-intptr-conversion branch July 11, 2022 12:38

filmor mentioned this pull request Jul 11, 2022

Fix docstring for directly constructed types #1865

Merged

5 tasks

lostmsu reviewed Jul 11, 2022

View reviewed changes

filmor mentioned this pull request Aug 12, 2022

Copy .NET to numpy array: python 3 breaks constructor IntPtr.__overloads__[int](np_arr.__array_interface__['data'][0]) #1116

Closed

filmor mentioned this pull request Sep 16, 2022

Config-less CoreCLR and improved runtime load message #1942

Merged

5 tasks

filmor mentioned this pull request Sep 17, 2022

Add __version__ #1946

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix (U)IntPtr construction #1861

Fix (U)IntPtr construction #1861

filmor commented Jul 8, 2022

filmor commented Jul 9, 2022

lostmsu commented Jul 10, 2022

lostmsu commented Jul 10, 2022

filmor commented Jul 10, 2022

lostmsu commented Jul 11, 2022 •

edited

Loading

lostmsu commented Jul 11, 2022 •

edited

Loading

lostmsu Jul 11, 2022 •

edited

Loading

filmor Jul 11, 2022

lostmsu Jul 11, 2022

lostmsu Jul 11, 2022

filmor Jul 11, 2022

filmor commented Jul 11, 2022

	if (type.IsPrimitive \|\| type == typeof(string))
	{
	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);
	}

Fix (U)IntPtr construction #1861

Fix (U)IntPtr construction #1861

Conversation

filmor commented Jul 8, 2022

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

filmor commented Jul 9, 2022

lostmsu commented Jul 10, 2022

lostmsu commented Jul 10, 2022

filmor commented Jul 10, 2022

lostmsu commented Jul 11, 2022 • edited Loading

lostmsu commented Jul 11, 2022 • edited Loading

lostmsu Jul 11, 2022 • edited Loading

Choose a reason for hiding this comment

filmor Jul 11, 2022

Choose a reason for hiding this comment

lostmsu Jul 11, 2022

Choose a reason for hiding this comment

lostmsu Jul 11, 2022

Choose a reason for hiding this comment

filmor Jul 11, 2022

Choose a reason for hiding this comment

filmor commented Jul 11, 2022

lostmsu commented Jul 11, 2022 •

edited

Loading

lostmsu commented Jul 11, 2022 •

edited

Loading

lostmsu Jul 11, 2022 •

edited

Loading