bpo-23867: Argument Clinic: inline parsing code for a single positional parameter.

[serhiy-storchaka]

https://bugs.python.org/issue23867

[ammaraskar]

Really great work, I think adding parse_arg to argument clinic converters is
a solid API.

It seems a lot of these converters (e.g single positional subclass_of) are
currently not used, which means a lot of the generated code remains untested.
At the very least I think we should add as many indirect tests for the newly added
code as possible. Here's a non-exhaustive check list:

bool_converter

• from integer (tested by curses.use_env)
• from truthy object (could be tested by _winapi.Overlapped.GetOverlappedResult but no tests exist for this)

char_converter

• from bytes or bytearray (could be tested by msvcrt.putch but no tests exist for this)

unsigned_char_converter

• from integer (tested by curses.halfdelay)
• from unsigned long mask (unused)

short_converter

• from integer (tested by curses.color_content)

int_converter

• from integer (tested by test_builtin chr())
• from single unicode character (tested by unicodedata.bidirectional)

unsigned_int_converter

• from integer (unused)

long_converter

• from integer (tested by curses.attroff)

long_long_converter

• from integer (unused)

unsigned_long_long_converter

• from integer (unused)

Py_ssize_t_converter

• from integer (tested by os.urandom())

size_t_converter

• from integer (unused)

float_converter

• from float (unused)

double_converter

• from float (tested by math.degrees)

Py_complex_converter

• from complex number (tested by cmathmodule.acos)

str_converter

• from string (tested by codecsmodule.lookup)

unicode_converter

• from string (used in a lot of places, didn't write test)

Py_buffer_converter

• from pybuf_simple (already tested in marshal.loads)
• from string (already tested in array.fromstring)
• from pybuf_writable (tested by io.readinto1)

For the unused ones/hard to test cases, I personally think that it would be wise to
split this change into two commits, each fully tested. One where all the single positional
ones have at least one test case, and then when argument clinic can handle generating
inline parsing code for multiple arguments/kwargs/defaults, then add the rest of the
converters with accompanying test cases.

Here are the test cases I came up with:

Test Cases

diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py
index dafcf004c0..0e40829395 100644
--- a/Lib/test/test_builtin.py
+++ b/Lib/test/test_builtin.py
@@ -306,6 +306,9 @@ class BuiltinTest(unittest.TestCase):
         self.assertRaises(ValueError, chr, 0x00110000)
         self.assertRaises((OverflowError, ValueError), chr, 2**32)

+        with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+            chr(1.0)
+
     def test_cmp(self):
         self.assertTrue(not hasattr(builtins, "cmp"))

diff --git a/Lib/test/test_codecs.py b/Lib/test/test_codecs.py
index 00b5d317c4..529ebaf9dd 100644
--- a/Lib/test/test_codecs.py
+++ b/Lib/test/test_codecs.py
@@ -1758,6 +1758,10 @@ class CodecsModuleTest(unittest.TestCase):
         self.assertRaises(TypeError, codecs.lookup)
         self.assertRaises(LookupError, codecs.lookup, "__spam__")
         self.assertRaises(LookupError, codecs.lookup, " ")
+        with self.assertRaisesRegex(TypeError, 'argument must be str, not list'):
+            codecs.lookup([])
+        with self.assertRaisesRegex(ValueError, 'embedded null character'):
+            codecs.lookup('abcd\0ef')

     def test_getencoder(self):
         self.assertRaises(TypeError, codecs.getencoder)
diff --git a/Lib/test/test_curses.py b/Lib/test/test_curses.py
index 3b442fe6a4..ae0ec49703 100644
--- a/Lib/test/test_curses.py
+++ b/Lib/test/test_curses.py
@@ -109,6 +109,9 @@ class TestCurses(unittest.TestCase):
         stdscr.addnstr(4,4, '1234', 3)
         stdscr.addnstr(5,5, '1234', 3, curses.A_BOLD)

+        with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+            stdscr.attron(1.0)
+
         stdscr.attron(curses.A_BOLD)
         stdscr.attroff(curses.A_BOLD)
         stdscr.attrset(curses.A_BOLD)
@@ -250,6 +253,14 @@ class TestCurses(unittest.TestCase):
             f.seek(0)
             curses.getwin(f)

+        # Make sure type errors in halfdelay are caught
+        with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+            curses.halfdelay(1.0)
+        with self.assertRaisesRegex(OverflowError, 'unsigned byte integer is less than minimum'):
+            curses.halfdelay(-1)
+        with self.assertRaisesRegex(OverflowError, 'unsigned byte integer is greater than maximum'):
+            curses.halfdelay(5000)
+
         curses.halfdelay(1)
         curses.intrflush(1)
         curses.meta(1)
@@ -272,6 +283,11 @@ class TestCurses(unittest.TestCase):
         curses.unctrl('a')
         curses.ungetch('a')
         if hasattr(curses, 'use_env'):
+            # Make sure type errors in use_env are caught
+            with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+                curses.use_env(1.0)
+            with self.assertRaisesRegex(TypeError, r'an integer is required \(got type list\)'):
+                curses.use_env([])
             curses.use_env(1)

     # Functions only available on a few platforms
@@ -285,6 +301,14 @@ class TestCurses(unittest.TestCase):
         curses.pair_content(curses.COLOR_PAIRS - 1)
         curses.pair_number(0)

+        # Make sure type errors in color_content are caught
+        with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+            curses.color_content(1.0)
+        with self.assertRaisesRegex(OverflowError, 'signed short integer is less than minimum'):
+            curses.color_content(-100000)
+        with self.assertRaisesRegex(OverflowError, 'signed short integer is greater than maximum'):
+            curses.color_content(100000)
+
         if hasattr(curses, 'use_default_colors'):
             curses.use_default_colors()

@@ -301,6 +325,11 @@ class TestCurses(unittest.TestCase):
         (availmask, oldmask) = curses.mousemask(curses.BUTTON1_PRESSED)
         if availmask == 0:
             self.skipTest('mouse stuff not available')
+
+        # type check arguments to mousemask
+        with self.assertRaisesRegex(TypeError, 'argument must be int, not float'):
+            curses.mousemask(1.0)
+
         curses.mouseinterval(10)
         # just verify these don't cause errors
         curses.ungetmouse(0, 0, 0, 0, curses.BUTTON1_PRESSED)
diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py
index c68b2fea85..97865fade9 100644
--- a/Lib/test/test_io.py
+++ b/Lib/test/test_io.py
@@ -1312,6 +1312,7 @@ class BufferedReaderTest(unittest.TestCase, CommonBufferedTests):
         self.assertEqual(bufio.readinto1(b), 6)
         self.assertEqual(b[:6], b"fghjkl")
         self.assertEqual(rawio._reads, 4)
+        self.assertRaises(TypeError, bufio.readinto1, [])

     def test_readinto_array(self):
         buffer_size = 60
diff --git a/Lib/test/test_math.py b/Lib/test/test_math.py
index 9b2f55e1f4..0730738afc 100644
--- a/Lib/test/test_math.py
+++ b/Lib/test/test_math.py
@@ -271,6 +271,7 @@ class MathTests(unittest.TestCase):

     def testAcos(self):
         self.assertRaises(TypeError, math.acos)
+        self.assertRaises(TypeError, math.acos, 'a')
         self.ftest('acos(-1)', math.acos(-1), math.pi)
         self.ftest('acos(0)', math.acos(0), math.pi/2)
         self.ftest('acos(1)', math.acos(1), 0)
@@ -475,6 +476,8 @@ class MathTests(unittest.TestCase):

     def testDegrees(self):
         self.assertRaises(TypeError, math.degrees)
+        with self.assertRaisesRegex(TypeError, 'must be real number, not str'):
+            math.degrees('a')
         self.ftest('degrees(pi)', math.degrees(math.pi), 180.0)
         self.ftest('degrees(pi/2)', math.degrees(math.pi/2), 90.0)
         self.ftest('degrees(-pi/4)', math.degrees(-math.pi/4), -45.0)
diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index 6dbc255612..6bb81ab7dd 100644
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -1332,6 +1332,15 @@ class URandomTests(unittest.TestCase):
         data2 = self.get_urandom_subprocess(16)
         self.assertNotEqual(data1, data2)

+    def test_uranom_typechecking(self):
+        with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+            os.urandom(1.0)
+        with self.assertRaisesRegex(TypeError, 'object cannot be interpreted as an integer'):
+            os.urandom([])
+        # This test should be rewritten to reliably overflow ssize_t on any platform
+        with self.assertRaisesRegex(OverflowError, 'Python int too large to convert to C ssize_t'):
+            os.urandom(2**128)
+

 @unittest.skipUnless(hasattr(os, 'getrandom'), 'need os.getrandom()')
 class GetRandomTests(unittest.TestCase):
diff --git a/Lib/test/test_unicodedata.py b/Lib/test/test_unicodedata.py
index 170778fa97..ee7587e0d8 100644
--- a/Lib/test/test_unicodedata.py
+++ b/Lib/test/test_unicodedata.py
@@ -158,6 +158,9 @@ class UnicodeFunctionsTest(UnicodeDatabaseTest):
         self.assertRaises(TypeError, self.db.bidirectional)
         self.assertRaises(TypeError, self.db.bidirectional, 'xx')

+        with self.assertRaisesRegex(TypeError, 'argument must be a unicode character, not list'):
+            self.db.bidirectional([])
+
     def test_decomposition(self):
         self.assertEqual(self.db.decomposition('\uFFFE'),'')
         self.assertEqual(self.db.decomposition('\u00bc'), '<fraction> 0031 2044 0034')

[serhiy-storchaka]

Thank you for your review @ammaraskar.

Do you mind to add tests proposed by you in a separate PR?