NotificationsYou must be signed in to change notification settings
Fork748
Star5.1k

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

filmor merged 6 commits intopythonnet:releasefromfilmor:fix-intptr-conversion

Jul 11, 2022

Merged

Fix (U)IntPtr construction#1861

filmor merged 6 commits intopythonnet:releasefromfilmor:fix-intptr-conversion

Jul 11, 2022

Conversation

Copy link

Member

filmor commentedJul 8, 2022

What does this implement/fix? Explain your changes.

Allows creating anIntPtr from Python. This is currently impossible, because we rely on automatic conversion (asIntPtr 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 in87d4db9

	if(type.IsPrimitive\|\|type==typeof(string))
	{
	if(Runtime.PyTuple_Size(args)!=1)
	{
	Exceptions.SetError(Exceptions.TypeError,"no constructors match given arguments");
	returndefault;
	}

	BorrowedReferenceop=Runtime.PyTuple_GetItem(args,0);

	if(!Converter.ToManaged(op,type,outvarresult,true))
	{
	returndefault;
	}

	returnCLRObject.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 toAUTHORS
Updated theCHANGELOG

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 thefix-intptr-conversion branch from54e5a7d to1c21526Compare

July 8, 2022 18:41

Special-case construction of (U)IntPtr

004232f

filmor requested a review fromlostmsu

July 9, 2022 14:05

filmor marked this pull request as ready for review

July 9, 2022 14:05

Copy link

MemberAuthor

filmor commentedJul 9, 2022

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

Move ptr construction to ClassObject

c88b1f6

Copy link

Member

lostmsu commentedJul 10, 2022

Re: IntPtr

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

Copy link

Member

lostmsu commentedJul 10, 2022

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

Copy link

MemberAuthor

filmor commentedJul 10, 2022

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.

Copy link

Member

lostmsu commentedJul 11, 2022•
edited
Loading

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 justIsPrimitive 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"

lostmsu reviewed

Jul 11, 2022

View reviewed changes

tests/test_conversion.py OutdatedShow resolvedHide resolved

filmor added2 commits

July 11, 2022 11:41

Add int constructor for (U)IntPtr

65a6408

Extend intptr conversion tests and exceptions

775efd5

filmor force-pushed thefix-intptr-conversion branch 2 times, most recently froma6cdc35 to43f748bCompare

July 11, 2022 12:23

Check (U)IntPtr size explicitly

ae5e074

filmor force-pushed thefix-intptr-conversion branch from43f748b toae5e074Compare

July 11, 2022 12:30

filmor merged commit332f8e7 intopythonnet: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

Copy link

Member

lostmsu commentedJul 11, 2022•
edited
Loading

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

Jul 11, 2022

View reviewed changes

src/runtime/Types/ClassObject.cs

		}
		}

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

Copy link

Member

lostmsuJul 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 forIntPtr constructor call with this path (e.g. parameter is neither CLR object nor Pythonint)

Copy link

MemberAuthor

filmorJul 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

Member

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

lostmsu reviewed

Jul 11, 2022

View reviewed changes

src/runtime/Types/ClassObject.cs

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

Copy link

Member

lostmsuJul 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

MemberAuthor

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

Copy link

MemberAuthor

filmor commentedJul 11, 2022

@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

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

Labels

None yet

2 participants

Movatterモバイル変換

Fix (U)IntPtr construction#1861

Fix (U)IntPtr construction#1861

Uh oh!

Conversation

filmor commentedJul 8, 2022

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

filmor commentedJul 9, 2022

Uh oh!

lostmsu commentedJul 10, 2022

Uh oh!

lostmsu commentedJul 10, 2022

Uh oh!

filmor commentedJul 10, 2022

Uh oh!

lostmsu commentedJul 11, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Uh oh!

lostmsu commentedJul 11, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

lostmsuJul 11, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmorJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

lostmsuJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

lostmsuJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

filmorJul 11, 2022

Choose a reason for hiding this comment

Uh oh!

filmor commentedJul 11, 2022

Uh oh!

Uh oh!

lostmsu commentedJul 11, 2022•
edited
Loading

lostmsu commentedJul 11, 2022•
edited
Loading

lostmsuJul 11, 2022•
edited
Loading