Skip to content

FIX reduce method of unbound cython method#2969

Merged
scoder merged 10 commits intocython:masterfrom
pierreglaser:fix-unbound-cython-method-reduce
May 29, 2019
Merged

FIX reduce method of unbound cython method#2969
scoder merged 10 commits intocython:masterfrom
pierreglaser:fix-unbound-cython-method-reduce

Conversation

@pierreglaser
Copy link
Contributor

@pierreglaser pierreglaser commented May 24, 2019

Closes #2972
This was spotted thanks to cloudpipe/cloudpickle#259

@pierreglaser pierreglaser force-pushed the fix-unbound-cython-method-reduce branch from 90f088e to f26c01f Compare May 27, 2019 12:29
@pierreglaser pierreglaser changed the title [WIP] FIX reduce method of unbound cython method FIX reduce method of unbound cython method May 27, 2019
@pierreglaser
Copy link
Contributor Author

OK @scoder I think this PR is ready for review :)

@pierreglaser
Copy link
Contributor Author

pierreglaser commented May 27, 2019

Note that the test is skipped for early Python versions because for Python versions <= 3.4, pickling unbound methods also fails for pure python classes. This bugfix simply fixes a regression.

@scoder scoder added this to the 3.0 milestone May 27, 2019
@scoder scoder added defect Python Semantics General Python (3) language semantics labels May 27, 2019
@scoder
Copy link
Contributor

scoder commented May 27, 2019

Hmmm … I guess crossing the Py2/3 boundary with pickles isn't (well) supported anyway, so it's probably ok to make this incompatible. Does this also impact bound methods? Is there a test for those already?

@pierreglaser
Copy link
Contributor Author

Bound methods are method instances (as opposed to unbound methods, which are cyfunction instances), and the method class implements a __reduce__ method already. But it does not harm to add a line checking this in the doctest.

@@ -300,5 +300,5 @@ if sys.version_info[:2] >= (3, 5):
True
>>> bound_method = pickle.loads(pickle.dumps(MyClass().my_method))
Copy link
Contributor

Choose a reason for hiding this comment

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

And now that the method takes an argument, you could even call bound_method(42) here and see if it does the right thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I encountered an obscure failure in the CI that I cannot reproduce locally after 1e2c7ee, so I am enriching the tests very incrementally to figure out what could have cause the failure. As soon as this turns green I'll add an assertion on the method's result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's looking good now, so I'll add the assertion.

@scoder scoder merged commit fa297da into cython:master May 29, 2019
@scoder
Copy link
Contributor

scoder commented May 29, 2019

Thanks!

@jakirkham
Copy link
Contributor

Would it be possible to backport this to 0.29? Happy to submit a PR if that would help :)

scoder pushed a commit that referenced this pull request Apr 29, 2021
@scoder scoder modified the milestones: 3.0, 0.29.24 Apr 29, 2021
@scoder
Copy link
Contributor

scoder commented Apr 29, 2021

Seems reasonable. I've picked it over.

@jakirkham
Copy link
Contributor

Great thank you Stefan! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

defect Python Semantics General Python (3) language semantics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cython-generated unbound class methods cannot be pickled

3 participants