Skip to content

[WIP] Drop tp_call implementation from metatype #496

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from

Conversation

filmor
Copy link
Member

@filmor filmor commented Jun 16, 2017

This seems to be a patch for broken behaviour in Python. Removing it
should solve #495 (__init__ called twice on construction).

What does this implement/fix? Explain your changes.

#495

Does this close any currently open issues?

#495

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
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@filmor filmor changed the title Drop tp_call implementation from metatype [WIP] Drop tp_call implementation from metatype Jun 16, 2017
This seems to be a patch for broken behaviour in Python. Removing it
should solve pythonnet#495 (`__init__` called twice on construction).
@filmor filmor force-pushed the drop-metatype-tp_call branch from 350b65a to 9f64f56 Compare June 16, 2017 18:03
@filmor filmor changed the title [WIP] Drop tp_call implementation from metatype Drop tp_call implementation from metatype Jun 16, 2017
@codecov
Copy link

codecov bot commented Jun 16, 2017

Codecov Report

Merging #496 into master will decrease coverage by 0.46%.
The diff coverage is 73.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
- Coverage   76.41%   75.95%   -0.47%     
==========================================
  Files          64       64              
  Lines        5572     5572              
  Branches      895      894       -1     
==========================================
- Hits         4258     4232      -26     
- Misses       1020     1044      +24     
- Partials      294      296       +2
Flag Coverage Δ
#setup_linux 75.71% <ø> (ø) ⬆️
#setup_windows 75.3% <73.68%> (-0.47%) ⬇️
Impacted Files Coverage Δ
src/runtime/metatype.cs 68.29% <100%> (-2.44%) ⬇️
src/runtime/classderived.cs 78.01% <66.66%> (-2.42%) ⬇️
src/runtime/methodbinding.cs 75% <0%> (-5%) ⬇️
src/runtime/pythonengine.cs 76.52% <0%> (-2.18%) ⬇️
src/runtime/delegatemanager.cs 87.71% <0%> (-0.88%) ⬇️
src/runtime/pyscope.cs 57.92% <0%> (-0.79%) ⬇️
src/runtime/runtime.cs 91.22% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ae358...8f02a04. Read the comment docs.

@filmor filmor requested review from vmuriart and den-run-ai June 16, 2017 18:18
@filmor filmor changed the title Drop tp_call implementation from metatype [WIP] Drop tp_call implementation from metatype Jun 16, 2017
@filmor
Copy link
Member Author

filmor commented Jun 16, 2017

Don't merge this yet, apparently the example given by @rickardraysearch is not tested. When running his test:

class X(System.Object):
    __namespace__ = "PyTest"

    def __init__(self, *args):
        print("Running X constructor, args = {0}".format(args))

x = X(1,2)

one can see that without the tp_call implementation given here, __init__ is always called without parameters. I'll add the test and try to make it work.

@rickardraysearch
Copy link
Contributor

My very naive understanding of what is being done in pythonnet is as follows: Since there's no exact equivalence to a constructor in python indicating which base constructor to use (apart from calling one in __new__), pythonnet solves that by copying all base constructors to the subclass (AddConstructor in ClassDerivedObject.CreateDerivedType), but tries to run the python-defined __init__ with any remaining arguments from the constructor call (the eargs thingy in ConstructorBinder.InvokeRaw). That fails in this case, since InvokeRaw is called by the created CLR constructor, which has already lost the extra arguments.

Wouldn't it make more sense to have a ClassDerived.tp_init method that calls __init__ with whatever arguments are passed there? I have a commit at rickardraysearch/pythonnet@ a06fc32 that does more or less that with some bugs.

@filmor
Copy link
Member Author

filmor commented Jun 18, 2017

This is exactly what I was looking into right now :)

@rickardraysearch rickardraysearch mentioned this pull request Jun 21, 2017
3 tasks
@filmor filmor closed this Jun 21, 2017
@filmor filmor deleted the drop-metatype-tp_call branch June 21, 2017 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants