- Notifications
You must be signed in to change notification settings - Fork748
#2240 Recursion error when doing reversed python operations on C# types#2327
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1ae96dd
toc256746
Compare
lostmsu left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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?
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
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. |
d4e1fe9
to4ea33fe
Compare4ea33fe
to5277ed9
CompareUh oh!
There was an error while loading.Please reload this page.
src/runtime/MethodBinder.cs Outdated
/// <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) |
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?
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...
3409d88
to38fa8f2
Compare38fa8f2
toed2505c
Compare
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 shouldresolve#2240.
...
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORS
CHANGELOG