davidhalter / parso

A Python Parser
https://parso.readthedocs.org/en/latest/
Other
614 stars 107 forks source link

Possible misuse of `hashlib.sha256` #88

Closed aivarannamaa closed 4 years ago

aivarannamaa commented 4 years ago

Parso includes calls to hashlib.sha256 with an argument.

The documentation doesn't mention arguments. In my Windows Python 3.8 it does take arguments, but here is a log from a package build in Fedora, which failed with:

parser = <class 'parso.python.parser.Parser'>
diff_parser = <class 'parso.python.diff.DiffParser'>
    def __init__(self, text, tokenizer, parser=BaseParser, diff_parser=None):
        self._pgen_grammar = generate_grammar(
            text,
            token_namespace=self._get_token_namespace()
        )
        self._parser = parser
        self._tokenizer = tokenizer
        self._diff_parser = diff_parser
>       self._hashed = hashlib.sha256(text.encode("utf-8")).hexdigest()
E       TypeError: sha256() takes no arguments
/usr/lib/python3.8/site-packages/parso/grammar.py:39: TypeError
davidhalter commented 4 years ago

That's strange. It works for me on Python 3.8 and also it's mentioned in the docs:

>>> hashlib.sha224(b"Nobody inspects the spammish repetition").hexdigest()
'a4337bc45a8fc544c03f52dc550cd6e1e87021bc896588bd79e901e2'

So while I'm happy to change this, why is this still documented? At the same time the documentation seems to be lacking, because it doesn't really document these sha* objects.

davidhalter commented 4 years ago

Also typeshed allows the bytes argument: https://github.com/python/typeshed/blob/master/stdlib/3/hashlib.pyi#L27

aivarannamaa commented 4 years ago

Indeed, now I noticed this in the documentation:

As a shortcut, you can pass the first chunk of data to update directly to the constructor as the positional argument.

Python 3.8 in my Ubuntu 18.06 seems to support the argument. I'll see what Fedora people say about it.

davidhalter commented 4 years ago

Also it would be interesting if other people that are using Python3.8 (especially if they have Windows) could chime in.

aivarannamaa commented 4 years ago

In Fedora Rawhide, Python 3.8.0 hashlib.sha256 does accept bytes argument.

@hroncok , do you know why this build failed with "TypeError: sha256() takes no arguments"?

hroncok commented 4 years ago

No idea, sorry. Can you please share the link to the build, not just the log?

aivarannamaa commented 4 years ago

@hroncok , I guess you meant this: https://koji.fedoraproject.org/koji/taskinfo?taskID=39137097

(The notification email I got initially directed me to this page: https://koschei.fedoraproject.org/package/thonny?collection=f32)

hroncok commented 4 years ago

Here's some fun:

$ grep -r sha256
thonny/plugins/micropython/api_stubs/uhashlib.py:class sha256:
Binary file thonny/plugins/micropython/api_stubs/__pycache__/hashlib.cpython-38.pyc matches
thonny/plugins/micropython/api_stubs/hashlib.py:class sha256:
build/lib/thonny/plugins/micropython/api_stubs/uhashlib.py:class sha256:
build/lib/thonny/plugins/micropython/api_stubs/hashlib.py:class sha256:

$ xvfb-run py.test-3 thonny/test
...
3 passed, 1 warnings in 0.25 seconds

$ xvfb-run py.test-3 thonny
...
thonny/plugins/micropython/api_stubs/lcd160cr_test.py ...
thonny/test/test_common.py .
thonny/test/plugins/test_locals_marker.py F
thonny/test/plugins/test_name_highlighter.py .
...
E       TypeError: sha256() takes no arguments
...
1 failed, 5 passed, 1 warnings in 0.66 seconds

$ rm thonny/plugins/micropython/api_stubs/hashlib.py
$ xvfb-run py.test-3 thonny 
...
6 passed, 1 warnings in 0.53 seconds

https://github.com/thonny/thonny/blob/master/thonny/plugins/micropython/api_stubs/hashlib.py

Somehow takes precedence in the sys.path.

aivarannamaa commented 4 years ago

Thanks, Miro!

The problem seems to be caused by micropython\api_stubs\lcd160cr_test.py, picked up by pytest as test file. Per its policies this directory is added to sys.path. The bad thing is that it remains there for other test(s) as well. This behavior occurs only with Python 3.8, not with 3.7.

Anyway it looks like neither Parso nor Fedora can be blamed, so I'm closing this issue here. Thanks for your help!