From 5990d2864c73e1abc951c1d17741eab3e1b30be1 Mon Sep 17 00:00:00 2001
From: Mark Dickinson <dickinsm@gmail.com>
Date: Thu, 10 Apr 2014 09:29:39 -0400
Subject: [PATCH] Issue #20539: Improve math.factorial error messages and types
 for large inputs. - Better message for the OverflowError in large positive
 inputs. - Changed exception type from OverflowError to ValueError for large
 negative inputs.

---
 Lib/test/test_math.py | 12 ++++++++++--
 Misc/NEWS             |  4 ++++
 Modules/mathmodule.c  | 16 ++++++++++++----
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/Lib/test/test_math.py b/Lib/test/test_math.py
index 48f84ba7324..c9f3f161286 100644
--- a/Lib/test/test_math.py
+++ b/Lib/test/test_math.py
@@ -422,9 +422,17 @@ class MathTests(unittest.TestCase):
             self.assertEqual(math.factorial(i), py_factorial(i))
         self.assertRaises(ValueError, math.factorial, -1)
         self.assertRaises(ValueError, math.factorial, -1.0)
+        self.assertRaises(ValueError, math.factorial, -10**100)
+        self.assertRaises(ValueError, math.factorial, -1e100)
         self.assertRaises(ValueError, math.factorial, math.pi)
-        self.assertRaises(OverflowError, math.factorial, sys.maxsize+1)
-        self.assertRaises(OverflowError, math.factorial, 10e100)
+
+    # Other implementations may place different upper bounds.
+    @support.cpython_only
+    def testFactorialHugeInputs(self):
+        # Currently raises ValueError for inputs that are too large
+        # to fit into a C long.
+        self.assertRaises(OverflowError, math.factorial, 10**100)
+        self.assertRaises(OverflowError, math.factorial, 1e100)
 
     def testFloor(self):
         self.assertRaises(TypeError, math.floor)
diff --git a/Misc/NEWS b/Misc/NEWS
index f7490e98a4e..dc61927775d 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -31,6 +31,10 @@ Core and Builtins
 Library
 -------
 
+- Issue #20539: Improved math.factorial error message for large positive inputs
+  and changed exception type (OverflowError -> ValueError) for large negative
+  inputs.
+
 - Issue #21172: isinstance check relaxed from dict to collections.Mapping.
 
 - Issue #21155: asyncio.EventLoop.create_unix_server() now raises a ValueError
diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c
index 7f094ffb537..7f525ea66fc 100644
--- a/Modules/mathmodule.c
+++ b/Modules/mathmodule.c
@@ -1408,6 +1408,7 @@ static PyObject *
 math_factorial(PyObject *self, PyObject *arg)
 {
     long x;
+    int overflow;
     PyObject *result, *odd_part, *two_valuation;
 
     if (PyFloat_Check(arg)) {
@@ -1421,15 +1422,22 @@ math_factorial(PyObject *self, PyObject *arg)
         lx = PyLong_FromDouble(dx);
         if (lx == NULL)
             return NULL;
-        x = PyLong_AsLong(lx);
+        x = PyLong_AsLongAndOverflow(lx, &overflow);
         Py_DECREF(lx);
     }
     else
-        x = PyLong_AsLong(arg);
+        x = PyLong_AsLongAndOverflow(arg, &overflow);
 
-    if (x == -1 && PyErr_Occurred())
+    if (x == -1 && PyErr_Occurred()) {
         return NULL;
-    if (x < 0) {
+    }
+    else if (overflow == 1) {
+        PyErr_Format(PyExc_OverflowError,
+                     "factorial() argument should not exceed %ld",
+                     LONG_MAX);
+        return NULL;
+    }
+    else if (overflow == -1 || x < 0) {
         PyErr_SetString(PyExc_ValueError,
                         "factorial() not defined for negative values");
         return NULL;