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.

Sign up for GitHub

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

Jump to bottom

BUGFIX: Bind functions to window on from js import func (#461) #1126

Merged
merged 18 commits into from
Jan 16, 2021
Merged

BUGFIX: Bind functions to window on from js import func (#461) #1126

merged 18 commits into from
Jan 16, 2021

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Jan 12, 2021 •
edited
Loading

Instead of making ad-hoc duplicate definitions of getattr and dir in jsimport.c, make a JsProxy wrapping window and then define JsImport_GetAttr and JsImport_Dir to be transparent passthroughs to JsProxy_GetAttr and JsProxy_Dir. (This current implementation of JsImport_Dir leaves out some fields that are on the actual PyModule object, but the previous implementation also leaves those out.

I plan to rewrite this whole file anyways in #972, this is just the minimal change that fixes the bug.
Resolves #461.

@dalcde
Copy link
Contributor

dalcde commented Jan 12, 2021 via email

@hoodmane
Copy link
Member Author

About to, just checking if they actually work first.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 12, 2021 •
edited
Loading

I have a bunch of tests that only pass with both this and #1124, so I have to sort through carefully to find the ones that test just this bug separately.

@hoodmane
Copy link
Member Author

Currently this always crashes on initialization.

@hoodmane hoodmane mentioned this pull request Jan 12, 2021
Merged
@hoodmane
Copy link
Member Author

Test failures are expected: this won't work without #1124 being applied first. I guess I'll merge that here now so that the tests can pass.

@hoodmane
Copy link
Member Author

@rth @dalcde I added pytest.mark.xfail to the test to show that #261 has been resolved because this bug complex also interacts with #974 which I have a fix for in #1033 so I figure we can re enable the key word arguments test when we merge #1033.

@hoodmane hoodmane changed the title Fix #461 BUGFIX: Bind functions to window on from js import func (#461) Jan 12, 2021
@rth rth mentioned this pull request Jan 13, 2021
Merged
@dalcde
Copy link
Contributor

dalcde commented Jan 14, 2021

@hoodmane test_jsproxy_call_kwargs is currently failing

@hoodmane
Copy link
Member Author

Right this is because JsBoundMethod_Call does not implement the keyword arguments handling but this change makes it so that all functions are always wrapped in JsBoundMethod and never in JsProxy. Correct solution is to move JsProxy_Vectorcall and JsProxy_New to JsBoundMethod and also perhaps to rename JsBoundMethod to JsMethod or something.

@hoodmane hoodmane force-pushed the bind-imports branch 2 times, most recently from cc0ebf8 to ceccde7 Compare January 14, 2021 18:48
@hoodmane
Copy link
Member Author

@dalcde correctly fixing test_jsproxy_call_kwargs is moderately complicated so I think the best thing is to xfail it here and open a second PR that fixes it.

@hoodmane
Copy link
Member Author

hoodmane commented Jan 14, 2021 •
edited
Loading

Funnily enough, PR #461 which would have also fixed this also broke test_jsproxy_kwargs at some intermediate stage. I guess it's not surprising.

@dalcde
Copy link
Contributor

dalcde commented Jan 14, 2021 via email

@hoodmane
Copy link
Member Author

It was working before this PR though

Yes, do you prefer:

  • a moderate-complexity set of changes added to this PR, or
  • same changes in a separate PR?

I guess I can just git checkout -b and open a separate PR with more changes on this same branch and then you can take your pick.

@dalcde
Copy link
Contributor

dalcde commented Jan 15, 2021

I would prefer a fix in a separate PR, assuming the fix doesn't depend on this. Then this can be merged after the fix is merged.

@hoodmane
Copy link
Member Author

The fix will break ~15 tests until this one is merged.

@dalcde
Copy link
Contributor

dalcde commented Jan 15, 2021

Then perhaps, as you said, a separate PR on top of this, and then I can merge them consecutively? I would very much prefer master to not regress, but it would be nice to logically separate the changes in history.

@hoodmane
Copy link
Member Author

Yeah, this is a pretty tricky tangle of changes. Will do that.

@hoodmane hoodmane mentioned this pull request Jan 15, 2021
Merged
@dalcde dalcde merged commit c533b07 into pyodide:master Jan 16, 2021
@hoodmane hoodmane deleted the bind-imports branch January 16, 2021 06:33
@hoodmane hoodmane mentioned this pull request Jan 16, 2021
Closed
@hoodmane hoodmane mentioned this pull request Apr 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dalcde dalcde dalcde 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.

from js import fetch treats fetch as a free function
2 participants
@hoodmane @dalcde