Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Operator overloads support#1324

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

Merged

Conversation

christabella
Copy link
Contributor

@christabellachristabella commentedDec 16, 2020•
edited
Loading

What does this implement/fix? Explain your changes.

Adapts@amos402's PR#907 for the latest Pythonnet version. I can also modify Amos's PR directly if that is more convenient for@amos402@lostmsu who have already made comments on it.

  • Tests are kept the same, as well as the links in classmanager, typemanager, and runtime.
  • However, the methodbinder logic in Amos's PR is quite different from the current methodbinder and cannot be copied directly.
    • Renamed 2 variablespynargs andclrnargs for readability and consistency with the rest of methodbinder.
  • Addedspecialized slot types (e.g. nb_subtract, nb_and) from the PyNumberMethods struct. Because a recent PRhttps://github.com/pythonnet/pythonnet/pull/1292/files defined the TypeOffset and ITypeOffset classes separately such that they no longer depends on interop3x.cs of a specific Python version.
  • Put bit shift operators (<< and>>) a separate test because they are quite different from the rest of the operators (+/-/& etc.)---the C# language specification is such that that shift operators always need an int for the second arg:https://stackoverflow.com/questions/7586887/c-sharp-shift-operator-overload. Instead of doingc = a >> b, we have to doc = a >> b.Num or get an ArgumentException.
  • Added other operators including reverse operators (e.g.__radd__).

Although Amos's PR had some params array tests, it looks like params array handling is currently being looked into by@filmor (#1304 and#1311), so I removed those for now, since they are not related to operator overloading.

Does this close any currently open issues?

Fixes#906.

Suggestions/future work

  • Unary operators

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
  • Can add docs once reverse operators are handled.
  • Add yourself toAUTHORS
  • Updated theCHANGELOG

@christabella
Copy link
ContributorAuthor

christabella commentedDec 17, 2020•
edited
Loading

2 failing tests for Python 3.9 on windows, but it seems to be an unrelated testtest_subclass.py:test_events().

image

I tried reproduce this locally on my windows laptop using python 3.9.0 but got errors such as
System.NotSupportedException : Python ABI v3.9.0 is not supported

@christabellachristabella changed the title[WIP] Operator overloads supportOperator overloads supportDec 17, 2020
@@ -280,6 +280,7 @@ internal static IntPtr CreateType(ManagedType impl, Type clrType)
{
throw new PythonException();
}
OperatorMethod.FixupSlots(type, clrType);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please address the comment that was made on this file in PR#907

christabella reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I have moved it up before pytype_ready

Copy link
Member

@lostmsulostmsu left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Overall looks good as a starting point. We need to keep in mind, that his does not give us full operator support.

  1. In C# and .NET only one argument of a binary operator must match the type, that defines it, and it does not have to be the first argument. This needs to be fixed in this PR: when adding operator methods only the methods where the left argument is of the parent type must be considered (and maybe also inherited ones). I think without this attempts to call the following overloaded operator on two instances ofOperableObject would fail from Python:
classOperableObject{  ...OperableObjectoperator+(intleft,OperableObjectright)=> ...;  ...OperableObject operator+(OperableObjectleft,OperableObjectright)=> ...;}

  1. Binary operators, that have the first argument of different type need to be handled correctly. I think this should be done in a separate PR, because I expect the amount of work to be comparable or even greater: loading a new type might add overloads to existing type's__add__ for example.

  2. A few operators are not covered here. The full list in C# is:https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/operators/operator-overloading . Notably missing comparison, bitwise, and obscure stuff like conversion totrue/false. I suggest we add them in a separate PR.

@christabellachristabellaforce-pushed thefeat/operator-overloads branch 3 times, most recently fromc0b5569 to659ad56CompareDecember 20, 2020 14:10
@christabella
Copy link
ContributorAuthor

Thank you for the review. Handling operators with asymmetrical types is a really good point. I have added the "forward" case where the arguments are OperableObject and int; I will add the "reverse" case where the arguments are int and OperableObject.

@lostmsu
Copy link
Member

@christabella I think the reverse case can be done in a separate PR. Until it is in, do you want me to review this one?

@christabella
Copy link
ContributorAuthor

@lostmsu Yes please, would appreciate your review once again.

Copy link
Member

@amos402amos402 left a comment

Choose a reason for hiding this comment

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

The original PR#907 reduced some iterations, redundant checking, and heap allocation in order to improve the performance for the binding behavior, also it included the new style params matching, so it was quite different from the master branch.
If you don't need those enhance you don't need to port them all.

christabella reacted with thumbs up emoji
@christabellachristabellaforce-pushed thefeat/operator-overloads branch 2 times, most recently from2dd951d to90cd7d3CompareDecember 21, 2020 11:13
Copy link
Member

@lostmsulostmsu left a comment

Choose a reason for hiding this comment

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

@christabella please, revert rename topynargs too.

@lostmsu
Copy link
Member

@christabella this looks good to me. But considering the upcoming forward operator support and theOperatorBinder are going to rewrite parts of this PR, it will make sense to just continue updating this PR.

@christabella
Copy link
ContributorAuthor

@christabella this looks good to me. But considering the upcoming forward operator support and theOperatorBinder are going to rewrite parts of this PR, it will make sense to just continue updating this PR.

@lostmsu sure I can do that, if you think it makes sense having seen the upcomingchristabella#1

@lostmsu
Copy link
Member

lostmsu commentedDec 29, 2020•
edited
Loading

@christabella actually, I suggested to create anOperatorBinder class and possiblyOperatorBinding class similar to existingConstructorBinder andConstructorBinding, which also do special argument handling, but for constructors.

You could probably also move some/all ofoperatormethod.cs into it.

But! It is not a requirement, you can also just leave code as-is.

@christabella
Copy link
ContributorAuthor

@christabella actually, I suggested to create anOperatorBinder class and possiblyOperatorBinding class similar to existingConstructorBinder andConstructorBinding, which also do special argument handling, but for constructors.

You could probably also move some/all ofoperatormethod.cs into it.

But! It is not a requirement, you can also just leave code as-is.

@lostmsu thanks for clarifying, I understand now. So that would be quite a refactor, for now I would try to keep this PR as-is.

// Only methods where the left operand is the declaring type.
ci.members[pyName] = new MethodObject(type, name, forwardMethods);
// Only methods where only the right operand is the declaring type.
ci.members[pyNameReverse] = new MethodObject(type, name, reverseMethods);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if there are no reverseMethods?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Currently it will just create a method that's never used; I can fix that by adding a hasReverse field in the SlotDefinition struct.

Copy link
ContributorAuthor

@christabellachristabellaDec 30, 2020•
edited
Loading

Choose a reason for hiding this comment

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

So if C# reverse operator isn't defined, so the Python reverse name will have an empty method list in MethodObject

So if a user callsb.__radd__(a) by writinga + b where there is no forward +() defined ona so Python resorts to__radd__, AND no reverse +() defined onb, what happens? Wouldn't hurt to add a check for an empty reverseMethods before doingci.members[pyNameReverse] = new MethodObject(type, name, reverseMethods);

Another unrelated issue is that of doing unnecessary work, if the Python slot doesn't support reverse e.g. unary operators (__rtruediv__ exists even though python docs don't list them)

Copy link
Member

Choose a reason for hiding this comment

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

It is also possible to have no "forward" methods.

If no reverse methods are defined, the corresponding slot should not be set. Same for forward.

Python slot doesn't support reverse e.g.__truediv__

Not sure what you mean. There's__rtruediv__

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That's true, I've just put a check then to not set the slot method if the method list is empty

cls = {name}
a = cls(2)
b = cls(10)
c = a + b
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, both of these calls will succeed (and do the same thing):

a.op_Addition(b)a.op_Addition(a,b)

Is that desirable behavior?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

So the first statement would have previously failed and now succeeds. I guess this is a question for@lostmsu if we want to let something that's wrongly written, not fail

Copy link
Member

Choose a reason for hiding this comment

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

a.op_Addition(a, b) must be failing for operators, defined in C#.

Copy link
Contributor

@tminkatminkaDec 30, 2020•
edited
Loading

Choose a reason for hiding this comment

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

The code I wrote is Python code. Ifa.op_Addition(a,b) now fails in Python.NET, then it will break existing code using Python.NET.

Copy link
Member

Choose a reason for hiding this comment

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

Breaking should not be a big deal, since we are making major version bump with (hopefully) a good overhaul.
But I don't thinka.op_Addition(a, b) should be working now.

christabella reacted with thumbs up emoji
@lostmsulostmsu merged commit9f01ebb intopythonnet:masterJan 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@amos402amos402amos402 left review comments

@tminkatminkatminka left review comments

@lostmsulostmsulostmsu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CLR operator and Python operator are not bound
4 participants
@christabella@lostmsu@amos402@tminka
[8]ページ先頭
©2009-2025 Movatter.jp