-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
Deprecate support for string
named-parameter in hash functions constructors
#134978
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
Comments
Is it worth the bother of using deprecation here? Is there a maintenance cost for us? Deprecations definitely incur maintenance cost for us (twice! once to write/review the deprecation code (6 files modified!), and again in a few years when removing the deprecated part of the API), and for old 3rd party code that still happens to use I'd honestly leave this one alone unless you can prove that it's a bug magnet. |
the maintenance cost for us of not doing deprecation is the additional complexity in a pile of hashlib code to maintain the dual-named exclusive argument acceptance hacks, including carrying those forward to any new additions. Deprecation is a way to reduce our burden in the long run by removing the unusual odd-duck argument handling complexity that our long past API inconsistency "oopsies" led to. We already had to jump through hoops to add the complexity to ameliorate those bugs. It'd be nice to not have to retain that in the long run. I don't think we'll see significant new change friction maintenance burden foisted upon users from this deprecation - they already cannot reliably depend on the name of some hashlib parameters like this given the varying build modes and platforms with multiple different API implementations where a couple of parameter have sometimes unintentionally changed name or positional-only status. The bug magnet is the complexity behind hashlib. We've got multiple different implementations of various APIs (for good reason) and have routinely unintentionally under-tested the exact pedantic all possible Hyrum's Law nature of those APIs so differences. Leading warts like this to creep in and cause issues. I expect more of that to happen as time goes on the more complex the permutations our API surface remains. |
the _hashlib.new as EVP_new
name as name_obj: object
string as data_obj: object(c_default="NULL") = b''
*
usedforsecurity: bool = True So using Line 144 in 461ca2c
Line 152 in 461ca2c
which exposes I'll actually need to amend my NEWS entry because it's a bit wrong. It wasn't possible to do However, depending on the hash functions, some could be called with I previously said that there was some issue import hashlib
import _hashlib
import sys
import _md5
import _sha3
hashlib.new("md5", data=b"") # ok
_hashlib.new("md5", data=b"") # not ok
_hashlib.new("md5", string=b"") # but this ok
_md5.md5(string=b"") # and this is ok as well
hashlib.md5(string=b"") # and this one is also ok!
hashlib.md5(data=b"") # but this one is not ok!
hashlib.new("sha3_224", data=b"") # ok
_hashlib.new("sha3_224", data=b"") # not ok
_hashlib.new("sha3_224", string=b"") # ok
hashlib.sha3_224(string=b"") # ok (if openssl-based)
hashlib.sha3_224(data=b"") # not ok..
_sha3.sha3_224(string=b"") # this is not ok!?
# this is STILL not ok even if help(_sha3.sha3_224) shows:
# sha3_224([data], *, usedforsecurity=True) -> SHA3 object
_sha3.sha3_224(data=b"")
_sha3.sha3_224(b"") # positional-only but was incorrectly documented as not
_hashlib.openssl_sha3_224(data=b"") # and this one is also NOT ok!???
_hashlib.openssl_sha3_224(string=b"") # but this one is OK??
# if we disable '_hashlib'
sys.modules["_hashlib"] = None
sys.modules.pop("hashlib")
import hashlib
hashlib.sha3_224(string=b"") # hey it's no more working!!
hashlib.sha3_224(data=b"") # still not working So, it's really difficult to iterate over the different constructors and using the same 'args/kwargs'. Namely, a dispatching mechanism is hard to maintain. Now, on 3.13+, it's possible to use any of the above combinations without any issues, although when one uses both data and string, there's still an error being raised. If someone just uses string or data, it's fine. |
Okay, now I understand. Thanks for explaining! |
Uh oh!
There was an error while loading. Please reload this page.
Feature or enhancement
This is a follow-up to #134696. After c6e63d9, it's now possible to use the following forms:
hashlib.new(name, data)
hashlib.new(name, data=...)
hashlib.new(name, string=...)
and, taking MD5 as an example,
hashlib.md5(data)
,hashlib.md5(data=...)
andhashlib.md5(string=...)
.In the docs we only expose the following signatures:
So, it would make sense to remove support for
string=...
. Ideally, I want to make it positional-only, but I don't think it's worth the shot and it's too much for a breaking change.Note that we could also deprecate data itself because originally, PEP-247 and PEP-452 were using
string
and notdata
, but this is probably confusing as the PEP says:I assume that the docs decided to use 'data' to remove this ambiguity as 'data' is more common for bytes-like objects.
cc @gpshead
Linked PRs
string
keyword parameter for hash function constructors #134979The text was updated successfully, but these errors were encountered: