-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
bpo-31478: Fix an assertion failure in random.seed() in case the 'a' arg has a bad __abs__() method #3596
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
bpo-31478: Fix an assertion failure in random.seed() in case the 'a' arg has a bad __abs__() method #3596
Conversation
Modules/_randommodule.c
Outdated
} | ||
if (!PyLong_Check(n)) { | ||
PyErr_Format(PyExc_TypeError, | ||
"abs(a) must return an integer, not '%.200s'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abs(seed)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree this seems weird, but that's what the docs say (https://docs.python.org/3.7/library/random.html#random.seed).
should i change to abs(seed)
anyway? (or should i also update the docs?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc is about random.Random.seed()
. This is _random.Random.seed()
. It doesn't have named arguments. I don't know what is the best wording, but mentioning a
whose name is arbitrary and not related to this function looks incorrect to me.
I leave up this to @rhettinger.
@@ -0,0 +1,2 @@ | |||
Fix an assertion failure in `random.seed()` in case the `a` argument has a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is `a`?
Lib/test/test_random.py
Outdated
class BadInt(int): | ||
def __abs__(self): | ||
return None | ||
with self.assertRaises(TypeError): | ||
self.gen.seed(BadInt()) | ||
self.gen.seed(BadInt()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that if wrap this with try: ... except TypeError: pass
, the cpython_only
decorator can be removed.
Modules/_randommodule.c
Outdated
Py_TYPE(n)->tp_name); | ||
goto Done; | ||
} | ||
n = PyLong_Type.tp_as_number->nb_absolute(arg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a short comment explaining why this is written in such way.
@rhettinger, if you don't have comments I'll merge this PR. |
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the |
Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to |
…seed has a bad __abs__() method. (pythonGH-3596) (cherry picked from commit d780b2d)
GH-3794 is a backport of this pull request to the 3.6 branch. |
Would you mind to create a backport to the 2.7 branch @orenmn? |
sure |
The behavior is different in 2.7, so i wasn't sure how to proceed, and asked for advice in https://bugs.python.org/issue31478#msg303207.. |
GH-3845 is a backport of this pull request to the 2.7 branch. |
_randommodule.c
: add a check whetherPyNumber_Absolute()
hasn't returned an integer.test_random.py
: add a test to verify that the assertion failure is no more.https://bugs.python.org/issue31478