From cc1a10c4d31a31656f900d54681f43da64deffee Mon Sep 17 00:00:00 2001 From: Wolfgang Maier Date: Wed, 28 Mar 2018 16:32:35 +0200 Subject: [PATCH 1/4] random.Random and subclasses: split _randbelow implementation Splits the getrandbits-dependent and -independent branches of random.Random._randbelow into separate methods and selects the implementation to be used by Random and its subclasses at class creation time for increased performance. --- Lib/random.py | 46 ++++++++++++------ Lib/test/test_random.py | 101 ++++++++++++++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 29 deletions(-) diff --git a/Lib/random.py b/Lib/random.py index 0bc24174e13f14..06f312abe61e46 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -38,7 +38,7 @@ """ from warnings import warn as _warn -from types import MethodType as _MethodType, BuiltinMethodType as _BuiltinMethodType +from types import FunctionType as _FunctionType from math import log as _log, exp as _exp, pi as _pi, e as _e, ceil as _ceil from math import sqrt as _sqrt, acos as _acos, cos as _cos, sin as _sin from os import urandom as _urandom @@ -94,6 +94,18 @@ def __init__(self, x=None): self.seed(x) self.gauss_next = None + def __init_subclass__(cls, **kwargs): + # Only call self.getrandbits if the original random() builtin method + # has not been overridden or if a new getrandbits() was supplied. + if type(cls.__dict__.get('getrandbits')) is _FunctionType: + cls._randbelow = cls._randbelow_with_getrandbits + elif 'random' in cls.__dict__: + # There's an overridden random() method but no new getrandbits() method, + # so we can only use random() from here. + cls._randbelow = cls._randbelow_without_getrandbits + else: + cls._randbelow = getattr(cls, cls._randbelow.__name__) + def seed(self, a=None, version=2): """Initialize internal state from hashable object. @@ -221,22 +233,26 @@ def randint(self, a, b): return self.randrange(a, b+1) - def _randbelow(self, n, int=int, maxsize=1<= n: - r = getrandbits(k) - return r - # There's an overridden random() method but no new getrandbits() method, - # so we can only use random() from here. + k = n.bit_length() # don't use (n-1) here because n can be 1 + r = getrandbits(k) # 0 <= r < 2**k + while r >= n: + r = getrandbits(k) + return r + + def _randbelow_without_getrandbits(self, n, int=int, maxsize=1<= maxsize: _warn("Underlying random() generator does not supply \n" "enough bits to choose from a population range this large.\n" @@ -251,6 +267,8 @@ def _randbelow(self, n, int=int, maxsize=1< n > 2**(k-1)) # note the stronger assertion - @unittest.mock.patch('random.Random.random') - def test_randbelow_overridden_random(self, random_mock): + def test_randbelow_without_getrandbits(self): # Random._randbelow() can only use random() when the built-in one # has been overridden but no new getrandbits() method was supplied. - random_mock.side_effect = random.SystemRandom().random maxsize = 1<= maxsize) - self.gen._randbelow(maxsize+1, maxsize = maxsize) - self.gen._randbelow(5640, maxsize = maxsize) + self.gen._randbelow_without_getrandbits( + maxsize+1, maxsize=maxsize + ) + self.gen._randbelow_without_getrandbits(5640, maxsize=maxsize) # issue 33203: test that _randbelow raises ValueError on # n == 0 also in its getrandbits-independent branch. with self.assertRaises(ValueError): - self.gen._randbelow(0, maxsize=maxsize) + self.gen._randbelow_without_getrandbits(0, maxsize=maxsize) + # This might be going too far to test a single line, but because of our # noble aim of achieving 100% test coverage we need to write a case in # which the following line in Random._randbelow() gets executed: @@ -672,8 +673,10 @@ def test_randbelow_overridden_random(self, random_mock): n = 42 epsilon = 0.01 limit = (maxsize - (maxsize % n)) / maxsize - random_mock.side_effect = [limit + epsilon, limit - epsilon] - self.gen._randbelow(n, maxsize = maxsize) + with unittest.mock.patch.object(random.Random, 'random') as random_mock: + random_mock.side_effect = [limit + epsilon, limit - epsilon] + self.gen._randbelow_without_getrandbits(n, maxsize=maxsize) + self.assertEqual(random_mock.call_count, 2) def test_randrange_bug_1590891(self): start = 1000000000000 @@ -926,6 +929,81 @@ def test_betavariate_return_zero(self, gammavariate_mock): gammavariate_mock.return_value = 0.0 self.assertEqual(0.0, random.betavariate(2.71828, 3.14159)) +class TestRandomSubclassing(unittest.TestCase): + def test_random_subclass_with_kwargs(self): + # SF bug #1486663 -- this used to erroneously raise a TypeError + class Subclass(random.Random): + def __init__(self, newarg=None): + random.Random.__init__(self) + Subclass(newarg=1) + + def test_overriding_random(self): + # First, let's assert our base class gets the selection of its + # _randbelow implementation right. + self.assertIs( + random.Random._randbelow, random.Random._randbelow_with_getrandbits + ) + + # Subclasses with a random method that got more recently defined than + # their getrandbits method should not rely on getrandbits in + # _randbelow, but select the getrandbits-independent implementation + # of _randbelow instead. + + # Subclass doesn't override any of the methods => keep using + # original getrandbits-dependent version of _randbelow + class SubClass1(random.Random): + pass + self.assertIs( + SubClass1._randbelow, random.Random._randbelow_with_getrandbits + ) + # subclass providing its own random **and** getrandbits methods + # like random.SystemRandom does => keep relying on getrandbits for + # _randbelow + class SubClass2(random.Random): + def random(self): + pass + + def getrandbits(self): + pass + self.assertIs( + SubClass2._randbelow, random.Random._randbelow_with_getrandbits + ) + # subclass providing only random => switch to getrandbits-independent + # version of _randbelow + class SubClass3(random.Random): + def random(self): + pass + self.assertIs( + SubClass3._randbelow, random.Random._randbelow_without_getrandbits + ) + # subclass defining getrandbits to complement its inherited random + # => can now rely on getrandbits for _randbelow again + class SubClass4(SubClass3): + def getrandbits(self): + pass + self.assertIs( + SubClass4._randbelow, random.Random._randbelow_with_getrandbits + ) + # subclass overriding the getrandbits-dependent implementation of + # _randbelow => make sure it is used + class SubClass5(SubClass4): + def _randbelow_with_getrandbits(self): + pass + self.assertIs( + SubClass5._randbelow, SubClass5._randbelow_with_getrandbits + ) + self.assertIsNot( + SubClass5._randbelow, random.Random._randbelow_with_getrandbits + ) + # subclass defining random making it more recent than its inherited + # getrandbits => switch back to getrandbits-independent implementaion + class SubClass6(SubClass5): + def random(self): + pass + self.assertIs( + SubClass6._randbelow, random.Random._randbelow_without_getrandbits + ) + class TestModule(unittest.TestCase): def testMagicConstants(self): self.assertAlmostEqual(random.NV_MAGICCONST, 1.71552776992141) @@ -937,13 +1015,6 @@ def test__all__(self): # tests validity but not completeness of the __all__ list self.assertTrue(set(random.__all__) <= set(dir(random))) - def test_random_subclass_with_kwargs(self): - # SF bug #1486663 -- this used to erroneously raise a TypeError - class Subclass(random.Random): - def __init__(self, newarg=None): - random.Random.__init__(self) - Subclass(newarg=1) - @unittest.skipUnless(hasattr(os, "fork"), "fork() required") def test_after_fork(self): # Test the global Random instance gets reseeded in child From 270fec15c8cd774c2f2e1fa6878e285eafed9dbb Mon Sep 17 00:00:00 2001 From: Wolfgang Maier Date: Mon, 9 Apr 2018 18:41:37 +0200 Subject: [PATCH 2/4] Fix rebase artefact --- Lib/random.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/Lib/random.py b/Lib/random.py index 06f312abe61e46..7a76dd17b64c35 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -249,9 +249,6 @@ def _randbelow_without_getrandbits(self, n, int=int, maxsize=1<= maxsize: _warn("Underlying random() generator does not supply \n" From fba740b9ab2faddbcdad4a443b05a2b99fb85758 Mon Sep 17 00:00:00 2001 From: Wolfgang Maier Date: Tue, 10 Apr 2018 13:12:19 +0200 Subject: [PATCH 3/4] Simplify subclassing logic and improve tests --- Lib/random.py | 27 +++++++---- Lib/test/test_random.py | 99 ++++++++++++++++------------------------- 2 files changed, 57 insertions(+), 69 deletions(-) diff --git a/Lib/random.py b/Lib/random.py index 7a76dd17b64c35..0ed5511e9f63c6 100644 --- a/Lib/random.py +++ b/Lib/random.py @@ -38,7 +38,6 @@ """ from warnings import warn as _warn -from types import FunctionType as _FunctionType from math import log as _log, exp as _exp, pi as _pi, e as _e, ceil as _ceil from math import sqrt as _sqrt, acos as _acos, cos as _cos, sin as _sin from os import urandom as _urandom @@ -95,16 +94,26 @@ def __init__(self, x=None): self.gauss_next = None def __init_subclass__(cls, **kwargs): - # Only call self.getrandbits if the original random() builtin method - # has not been overridden or if a new getrandbits() was supplied. - if type(cls.__dict__.get('getrandbits')) is _FunctionType: + """Control how subclasses generate random integers. + + The algorithm a subclass can use depends on the random() and/or + getrandbits() implementation available to it and determines + whether it can generate random integers from arbitrarily large + ranges. + """ + + if (cls.random is _random.Random.random) or ( + cls.getrandbits is not _random.Random.getrandbits): + # The original random() builtin method has not been overridden + # or a new getrandbits() was supplied. + # The subclass can use the getrandbits-dependent implementation + # of _randbelow(). cls._randbelow = cls._randbelow_with_getrandbits - elif 'random' in cls.__dict__: - # There's an overridden random() method but no new getrandbits() method, - # so we can only use random() from here. - cls._randbelow = cls._randbelow_without_getrandbits else: - cls._randbelow = getattr(cls, cls._randbelow.__name__) + # There's an overridden random() method but no new getrandbits(), + # so the subclass can only use the getrandbits-independent + # implementation of _randbelow(). + cls._randbelow = cls._randbelow_without_getrandbits def seed(self, a=None, version=2): """Initialize internal state from hashable object. diff --git a/Lib/test/test_random.py b/Lib/test/test_random.py index 5c78307ef2f1ac..d91908b03d0211 100644 --- a/Lib/test/test_random.py +++ b/Lib/test/test_random.py @@ -5,6 +5,7 @@ import time import pickle import warnings +import logging from functools import partial from math import log, exp, pi, fsum, sin, factorial from test import support @@ -619,6 +620,16 @@ def test_genrandbits(self): self.assertRaises(ValueError, self.gen.getrandbits, 0) self.assertRaises(ValueError, self.gen.getrandbits, -1) + def test_randrange_uses_getrandbits(self): + # Verify use of getrandbits by randrange + # Use same seed as in the cross-platform repeatability test + # in test_genrandbits above. + self.gen.seed(1234567) + # If randrange uses getrandbits, it should pick getrandbits(100) + # when called with a 100-bits stop argument. + self.assertEqual(self.gen.randrange(2**99), + 97904845777343510404718956115) + def test_randbelow_logic(self, _log=log, int=int): # check bitcount transition points: 2**i and 2**(i+1)-1 # show that: k = int(1.001 + _log(n, 2)) @@ -937,72 +948,40 @@ def __init__(self, newarg=None): random.Random.__init__(self) Subclass(newarg=1) - def test_overriding_random(self): - # First, let's assert our base class gets the selection of its - # _randbelow implementation right. - self.assertIs( - random.Random._randbelow, random.Random._randbelow_with_getrandbits - ) - - # Subclasses with a random method that got more recently defined than - # their getrandbits method should not rely on getrandbits in - # _randbelow, but select the getrandbits-independent implementation - # of _randbelow instead. + def test_subclasses_overriding_methods(self): + # Subclasses with an overridden random, but only the original + # getrandbits method should not rely on getrandbits in for randrange, + # but should use a getrandbits-independent implementation instead. - # Subclass doesn't override any of the methods => keep using - # original getrandbits-dependent version of _randbelow - class SubClass1(random.Random): - pass - self.assertIs( - SubClass1._randbelow, random.Random._randbelow_with_getrandbits - ) # subclass providing its own random **and** getrandbits methods # like random.SystemRandom does => keep relying on getrandbits for - # _randbelow - class SubClass2(random.Random): + # randrange + class SubClass1(random.Random): def random(self): - pass - - def getrandbits(self): - pass - self.assertIs( - SubClass2._randbelow, random.Random._randbelow_with_getrandbits - ) - # subclass providing only random => switch to getrandbits-independent - # version of _randbelow - class SubClass3(random.Random): + return super().random() + + def getrandbits(self, n): + logging.getLogger('getrandbits').info('used getrandbits') + return super().getrandbits(n) + with self.assertLogs('getrandbits'): + SubClass1().randrange(42) + + # subclass providing only random => can only use random for randrange + class SubClass2(random.Random): def random(self): - pass - self.assertIs( - SubClass3._randbelow, random.Random._randbelow_without_getrandbits - ) + logging.getLogger('random').info('used random') + return super().random() + with self.assertLogs('random'): + SubClass2().randrange(42) + # subclass defining getrandbits to complement its inherited random - # => can now rely on getrandbits for _randbelow again - class SubClass4(SubClass3): - def getrandbits(self): - pass - self.assertIs( - SubClass4._randbelow, random.Random._randbelow_with_getrandbits - ) - # subclass overriding the getrandbits-dependent implementation of - # _randbelow => make sure it is used - class SubClass5(SubClass4): - def _randbelow_with_getrandbits(self): - pass - self.assertIs( - SubClass5._randbelow, SubClass5._randbelow_with_getrandbits - ) - self.assertIsNot( - SubClass5._randbelow, random.Random._randbelow_with_getrandbits - ) - # subclass defining random making it more recent than its inherited - # getrandbits => switch back to getrandbits-independent implementaion - class SubClass6(SubClass5): - def random(self): - pass - self.assertIs( - SubClass6._randbelow, random.Random._randbelow_without_getrandbits - ) + # => can now rely on getrandbits for randrange again + class SubClass3(SubClass2): + def getrandbits(self, n): + logging.getLogger('getrandbits').info('used getrandbits') + return super().getrandbits(n) + with self.assertLogs('getrandbits'): + SubClass3().randrange(42) class TestModule(unittest.TestCase): def testMagicConstants(self): From 83aab0dc990615db9a274f94b2ef0999140b7f14 Mon Sep 17 00:00:00 2001 From: Wolfgang Maier Date: Tue, 10 Apr 2018 14:52:02 +0200 Subject: [PATCH 4/4] Add NEWS blurb --- .../next/Library/2018-04-10-14-50-30.bpo-33144.iZr4et.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2018-04-10-14-50-30.bpo-33144.iZr4et.rst diff --git a/Misc/NEWS.d/next/Library/2018-04-10-14-50-30.bpo-33144.iZr4et.rst b/Misc/NEWS.d/next/Library/2018-04-10-14-50-30.bpo-33144.iZr4et.rst new file mode 100644 index 00000000000000..eb6b9b7fe08d66 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-04-10-14-50-30.bpo-33144.iZr4et.rst @@ -0,0 +1,4 @@ +``random.Random()`` and its subclassing mechanism got optimized to check only +once at class/subclass instantiation time whether its ``getrandbits()`` method +can be relied on by other methods, including ``randrange()``, for the +generation of arbitrarily large random integers. Patch by Wolfgang Maier.