Skip to content
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

Pyproxy mixins #1264

Merged
merged 9 commits into from
Mar 5, 2021
Merged

Pyproxy mixins #1264

merged 9 commits into from
Mar 5, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Feb 18, 2021

This PR improves several things about PyProxy:

  1. Reduce stray properties to the absolute minimum. As much as possible attribute access should go to the Python object.
  2. Extra methods defined on Proxies of particular types of Python objects should overridden by properties on the python object (as discussed in ENH PyProxy getattr/etc traps & added getitem/etc for PyMappings #1175)
  3. Share prototypes between PyProxies of the same type to reduce memory usage.
  4. Organization, explanations, more consistent behavior.

Prior to this PR, fields called "name", "length", or "prototype" all showed weird behavior ("prototype" would be shadowed entirely, "name" and "length" show inconsistent results with the "in" keyword). Likewise "caller" and "arguments" were in all cases shadowed. Now the "name" and "length" inconsistencies are fixed, "prototype" is no longer shadowed (and setting and deleting the "prototype" field works as expected) but "prototype" shows some minor (and unremoveable) inconsistency with the "in" keyword. "caller" and "arguments" are now only shadowed if the PyProxy wraps a callable Python object, in other cases they behave normally.

I also added a lot of comments and organized and simplified the logic for testing which abstract object protocols the Python object supports.

This is on top of #1175.

Copy link
Contributor

@phorward phorward left a comment

Choose a reason for hiding this comment

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

Hello @hoodmane ,
thank for this pull request, it looks very impressive. From my first view I don't see any problems, the tests look nice on the mixins and the documentation is very well. The only "little" thing which bothers me are the comments about when you're not sure about something. I think these kind of thoughts must not be part of the source code, but that's only my opinion.

Separating the Javascript-parts from pyproxy.c into a new pyproxy.js could also be done within this pull request, but IMHO having all related C-JS-stuff in one file is okay.

src/core/pyproxy.c Show resolved Hide resolved
@phorward
Copy link
Contributor

Addresses #1049, #1006, #693, I'll do a test soon on my original issue 👍

@hoodmane
Copy link
Member Author

hoodmane commented Mar 2, 2021

@phorward Thanks for the review.

I don't think this PR addresses the GC issues. I'll probably do that after this is merged.

@phorward
Copy link
Contributor

phorward commented Mar 2, 2021

I don't think this PR addresses the GC issues. I'll probably do that after this is merged.

Yes, I found this already out, and I'm very excited about that change.

@phorward
Copy link
Contributor

phorward commented Mar 2, 2021

Thank you @hoodmane that is ok for me. Let's wait for CI, and can you please merge the current master into your branch?

@hoodmane
Copy link
Member Author

hoodmane commented Mar 5, 2021

So what's the status on this? Is this okay to merge @phorward?

@phorward phorward merged commit 40b63d6 into pyodide:master Mar 5, 2021
@hoodmane hoodmane deleted the pyproxy-mixins branch March 5, 2021 17:23
tytgatlieven pushed a commit to tytgatlieven/pyodide that referenced this pull request Apr 7, 2021
* Set up mixins in pyproxy.c

* Add a bunch of documentation comments to pyproxy

* PyProxy should only subclass Function when the Python object is Callable

* Don't leak name, length, or prototype. Add tests

* More tests, some final tuneups, more comments

* Remove tests that show different behavior between Firefox and Chrome

* Reduce repitition in getPyProxyClass

* Update comment
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