GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — #2240 Recursion error when doing reversed python operations on C# types by gertdreyer · Pull Request #2327 · pythonnet/pythonnet

Conversation

@gertdreyer

@gertdreyer gertdreyer commented Feb 23, 2024

Copy link
Copy Markdown
Contributor

What does this fix? Explain your changes.

Reverse operators were not implemented correctly for C# types which resulted in Recursion errors when embedding C# in Python. Changed the model binding process to add reverse operators for all applicable types. Added a flag to MethodBinder that signals the operation is a reversed operation and that the arguments to the operator needs to be reversed.

Added tests and an wrapper object for int without overloaded operator methods and a wrapper codec to test the reverse operations (and forward operator on the same type for safety).
...

Does this close any currently open issues?

This should resolve #2240.
...

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
gertdreyer added 2 commits February 23, 2024 09:35
@gertdreyer
Tests for operators on type CS type with codec
5d551fb
@gertdreyer
Bugfix: RecursionError when reverse/righthand operations invoked. e.g…
e7a3aba
…. __rsub__, __rmul__
@gertdreyer gertdreyer changed the title Bugfix/recursion error #2240 Recursion error when doing reversed python operations on C# types Feb 23, 2024
@gertdreyer
Update Authors and Changelog
c256746
@gertdreyer
gertdreyer force-pushed the bugfix/recursionError branch from 1ae96dd to c256746 Compare February 23, 2024 13:34
lostmsu
lostmsu reviewed Feb 24, 2024
View reviewed changes

@lostmsu lostmsu left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you explain what the issue was (e.g. what sequence of actions caused the error, and how codecs were a part of it) and how this fix addresses this scenario?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/embed_tests/TestOperator.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/embed_tests/TestOperator.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/embed_tests/TestOperator.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/runtime/MethodBinder.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/runtime/MethodBinder.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gertdreyer

gertdreyer commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Changes made...

I traced it down to the nb_add, nb_mul, etc slots only having the forward operators defined/linked for C# types being used in Python, this somehow caused the RecursionError when any reverse operators are called as they are probably just pointing to random pointers.

py * cs calls py.__mul__(cs) which is returns not implemented for the cs type and codecs does not get called prior to the mul call which results in cs.__rmul__(py) being called which caused the recursion error. I do not believe that there are problems with the codec system but it is needed to test this due to being on both sides of the CLR boundary.

It seems there were only catered for forward operators when both are the different (with overloaded operators, i.e. OwnInt - Int64 and Int64 - OwnInt). Thus all the code to detect whether a call is a reverse operator tested based on calling and passed types. Thus when called with the same types it could never detect it was a reversed operation even if it were defined. This needed some changes to the MethodBinding and ModelBinding to ensure the reversed calls are linked to the right C# operators and called with the arguments swapped.

lostmsu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gertdreyer
gertdreyer force-pushed the bugfix/recursionError branch from d4e1fe9 to 4ea33fe Compare February 26, 2024 11:26
@gertdreyer
Normalized names. Added HashCode and Equals to testing objects
5277ed9
@gertdreyer
gertdreyer force-pushed the bugfix/recursionError branch from 4ea33fe to 5277ed9 Compare February 26, 2024 11:30
lostmsu
lostmsu reviewed Feb 26, 2024
View reviewed changes
Comment thread src/runtime/MethodBinder.cs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/runtime/MethodBinder.cs Outdated
/// <param name="argsReversed">Reverse arguments of methods. Used for methods such as __radd__, __rsub__, __rmod__ etc</param>
/// <returns>A Binding if successful. Otherwise null.</returns>
internal Binding? Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw)
internal Binding? Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw, bool argsReversed = false)

@lostmsu lostmsu Feb 26, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you need to pass this parameter at all? Don't you have the field?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@gertdreyer gertdreyer Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Cleaned it up a bit. I missed that... The one method is static though so we will need to still pass it there...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lostmsu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@gertdreyer
gertdreyer force-pushed the bugfix/recursionError branch from 3409d88 to 38fa8f2 Compare February 26, 2024 18:58
@gertdreyer
Cleanup Codec/Argument Reversing Passing.
ed2505c
@gertdreyer
gertdreyer force-pushed the bugfix/recursionError branch from 38fa8f2 to ed2505c Compare February 26, 2024 19:01
lostmsu
lostmsu approved these changes Feb 26, 2024
View reviewed changes
@lostmsu
lostmsu merged commit 71ca063 into pythonnet:master Feb 26, 2024
@dependabot dependabot Bot mentioned this pull request Jan 16, 2026
Bump pythonnet from 3.0.3 to 3.0.5 mattemangia/Deep3DStudio#99
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lostmsu lostmsu lostmsu approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RecursionError when substracting System Decimal from Python Decimal

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@gertdreyer @lostmsu
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.