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

Problem with partial and pickle #925

Open
renefritze opened this issue Mar 29, 2021 · 5 comments · Fixed by #935 · May be fixed by #1122
Open

Problem with partial and pickle #925

renefritze opened this issue Mar 29, 2021 · 5 comments · Fixed by #935 · May be fixed by #1122
Labels
Enhancement ✨ Improvement to a component

Comments

@renefritze
Copy link
Contributor

I'm not using astroid directly, but discovered this issue downstream using sphinx-autoapi.
It seems I've run into an edge case with partial and the pickle module. I've added a new test case to astroid to showcase my problem: master...renefritze:conditional_test_pickle

With the last commit on that branch the test suite succeeds: https://travis-ci.com/github/renefritze/astroid/jobs/487475875
Dropping the last commit, the suite fails: https://travis-ci.com/github/renefritze/astroid/jobs/487452325
The difference being 307f3ac

@hippo91
Copy link
Contributor

hippo91 commented Apr 9, 2021

@renefritze thanks for your report. Would you mind opening a PR on astroid please?

@renefritze
Copy link
Contributor Author

Sure thing.

@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Jun 14, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.6.0 milestone Jun 14, 2021
@Pierre-Sassoulas Pierre-Sassoulas linked a pull request Jun 14, 2021 that will close this issue
@cdce8p cdce8p removed this from the 2.6.0 milestone Jun 21, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.6.7 milestone Aug 5, 2021
@Pierre-Sassoulas
Copy link
Member

Reopening because I did not understand that the merge request was only illustrative

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.6.7 milestone Aug 5, 2021
@renefritze renefritze linked a pull request Aug 9, 2021 that will close this issue
@Pierre-Sassoulas Pierre-Sassoulas linked a pull request Aug 10, 2021 that will close this issue
@renefritze
Copy link
Contributor Author

Is there anything more I can do to help that doesn't require in-depth astroid knowledge?

@DanielNoord
Copy link
Collaborator

I have investigated this a little.

 def test_conditional_definition_partial_pickle(self) -> None:
        """Regression test for a conditional defintion of a partial function wrapping pickle.

        Reported in https://github.com/PyCQA/astroid/issues/925
        """
        module = resources.build_file(
            "data/conditional_pickle_import/conditional_importer.py"
        )

        dump_nodes = module.lookup("dump")[1]
        assert len(dump_nodes) == 2

        # Check the function defintion node
        func_def_nodes = list(dump_nodes[0].infer())
        assert len(func_def_nodes) == 1
        assert isinstance(func_def_nodes[0], FunctionDef)

        # Check the partial node
        partial_node = list(dump_nodes[1].infer())
        assert len(partial_node) == 1
        assert isinstance(partial_node[0], objects.PartialFunction)

This should pass with:

# conditional_importer
import pickle

if False:

    def dump(obj, file, protocol=None):
        pass


else:
    from functools import partial

    dump = partial(pickle.dump, protocol=0)

The problem is that pickle.dump can be either pickle.dump or _pickle.dump.
https://github.com/PyCQA/astroid/blob/0d1211558670cfefd95b39984b8d5f7f34837f32/astroid/brain/brain_functools.py#L77

On that line in the functools brain we take the first value, which is the _pickle one (I believe). We don't infer its arguments and therefore we fail here:
https://github.com/PyCQA/astroid/blob/0d1211558670cfefd95b39984b8d5f7f34837f32/astroid/brain/brain_functools.py#L98-L99

We will need a tip for a pickle brain I think to solve this. I don't have much experience with adding brains so I probably won't get to this very soon but for anybody looking to fix this: I believe this is where we should be looking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants