gh-119357: Increase test coverage for keymap in _pyrepl (#119358)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>

Author: eugenetriguba

Diff for: Lib/_pyrepl/completing_reader.py

@@ -30,7 +30,7 @@
 # types
 Command = commands.Command
 if False:
-    from .types import Callback, SimpleContextManager, KeySpec, CommandName
+    from .types import KeySpec, CommandName
 
 
 def prefix(wordlist: list[str], j: int = 0) -> str:

Diff for: Lib/_pyrepl/keymap.py

@@ -19,38 +19,32 @@
 # CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 
 """
-functions for parsing keyspecs
+Keymap contains functions for parsing keyspecs and turning keyspecs into
+appropriate sequences.
 
-Support for turning keyspecs into appropriate sequences.
+A keyspec is a string representing a sequence of key presses that can
+be bound to a command. All characters other than the backslash represent
+themselves. In the traditional manner, a backslash introduces an escape
+sequence.
 
-pyrepl uses it's own bastardized keyspec format, which is meant to be
-a strict superset of readline's \"KEYSEQ\" format (which is to say
-that if you can come up with a spec readline accepts that this
-doesn't, you've found a bug and should tell me about it).
-
-Note that this is the `\\C-o' style of readline keyspec, not the
-`Control-o' sort.
-
-A keyspec is a string representing a sequence of keypresses that can
-be bound to a command.
-
-All characters other than the backslash represent themselves.  In the
-traditional manner, a backslash introduces a escape sequence.
+pyrepl uses its own keyspec format that is meant to be a strict superset of
+readline's KEYSEQ format. This means that if a spec is found that readline
+accepts that this doesn't, it should be logged as a bug. Note that this means
+we're using the `\\C-o' style of readline's keyspec, not the `Control-o' sort.
 
 The extension to readline is that the sequence \\<KEY> denotes the
-sequence of charaters produced by hitting KEY.
+sequence of characters produced by hitting KEY.
 
 Examples:
-
-`a'     - what you get when you hit the `a' key
+`a'      - what you get when you hit the `a' key
 `\\EOA'  - Escape - O - A (up, on my terminal)
 `\\<UP>' - the up arrow key
-`\\<up>' - ditto (keynames are case insensitive)
+`\\<up>' - ditto (keynames are case-insensitive)
 `\\C-o', `\\c-o'  - control-o
 `\\M-.'  - meta-period
 `\\E.'   - ditto (that's how meta works for pyrepl)
 `\\<tab>', `\\<TAB>', `\\t', `\\011', '\\x09', '\\X09', '\\C-i', '\\C-I'
-   - all of these are the tab character.  Can you think of any more?
+   - all of these are the tab character.
 """
 
 _escapes = {
@@ -111,7 +105,17 @@ class KeySpecError(Exception):
     pass
 
 
-def _parse_key1(key, s):
+def parse_keys(keys: str) -> list[str]:
+    """Parse keys in keyspec format to a sequence of keys."""
+    s = 0
+    r: list[str] = []
+    while s < len(keys):
+        k, s = _parse_single_key_sequence(keys, s)
+        r.extend(k)
+    return r
+
+
+def _parse_single_key_sequence(key: str, s: int) -> tuple[list[str], int]:
     ctrl = 0
     meta = 0
     ret = ""
@@ -183,20 +187,11 @@ def _parse_key1(key, s):
             ret = f"ctrl {ret}"
         else:
             raise KeySpecError("\\C- followed by invalid key")
-    if meta:
-        ret = ["\033", ret]
-    else:
-        ret = [ret]
-    return ret, s
 
-
-def parse_keys(key: str) -> list[str]:
-    s = 0
-    r = []
-    while s < len(key):
-        k, s = _parse_key1(key, s)
-        r.extend(k)
-    return r
+    result = [ret], s
+    if meta:
+        result[0].insert(0, "\033")
+    return result
 
 
 def compile_keymap(keymap, empty=b""):

Diff for: Lib/test/test_pyrepl/test_keymap.py

@@ -1,41 +1,78 @@
+import string
 import unittest
 
-from _pyrepl.keymap import parse_keys, compile_keymap
+from _pyrepl.keymap import _keynames, _escapes, parse_keys, compile_keymap, KeySpecError
 
 
 class TestParseKeys(unittest.TestCase):
     def test_single_character(self):
-        self.assertEqual(parse_keys("a"), ["a"])
-        self.assertEqual(parse_keys("b"), ["b"])
-        self.assertEqual(parse_keys("1"), ["1"])
+        """Ensure that single ascii characters or single digits are parsed as single characters."""
+        test_cases = [(key, [key]) for key in string.ascii_letters + string.digits]
+        for test_key, expected_keys in test_cases:
+            with self.subTest(f"{test_key} should be parsed as {expected_keys}"):
+                self.assertEqual(parse_keys(test_key), expected_keys)
+
+    def test_keynames(self):
+        """Ensure that keynames are parsed to their corresponding mapping.
+
+        A keyname is expected to be of the following form: \\<keyname> such as \\<left>
+        which would get parsed as "left".
+        """
+        test_cases = [(f"\\<{keyname}>", [parsed_keyname]) for keyname, parsed_keyname in _keynames.items()]
+        for test_key, expected_keys in test_cases:
+            with self.subTest(f"{test_key} should be parsed as {expected_keys}"):
+                self.assertEqual(parse_keys(test_key), expected_keys)
 
     def test_escape_sequences(self):
-        self.assertEqual(parse_keys("\\n"), ["\n"])
-        self.assertEqual(parse_keys("\\t"), ["\t"])
-        self.assertEqual(parse_keys("\\\\"), ["\\"])
-        self.assertEqual(parse_keys("\\'"), ["'"])
-        self.assertEqual(parse_keys('\\"'), ['"'])
+        """Ensure that escaping sequences are parsed to their corresponding mapping."""
+        test_cases = [(f"\\{escape}", [parsed_escape]) for escape, parsed_escape in _escapes.items()]
+        for test_key, expected_keys in test_cases:
+            with self.subTest(f"{test_key} should be parsed as {expected_keys}"):
+                self.assertEqual(parse_keys(test_key), expected_keys)
 
     def test_control_sequences(self):
-        self.assertEqual(parse_keys("\\C-a"), ["\x01"])
-        self.assertEqual(parse_keys("\\C-b"), ["\x02"])
-        self.assertEqual(parse_keys("\\C-c"), ["\x03"])
+        """Ensure that supported control sequences are parsed successfully."""
+        keys = ["@", "[", "]", "\\", "^", "_", "\\<space>", "\\<delete>"]
+        keys.extend(string.ascii_letters)
+        test_cases = [(f"\\C-{key}", chr(ord(key) & 0x1F)) for key in []]
+        for test_key, expected_keys in test_cases:
+            with self.subTest(f"{test_key} should be parsed as {expected_keys}"):
+                self.assertEqual(parse_keys(test_key), expected_keys)
 
     def test_meta_sequences(self):
         self.assertEqual(parse_keys("\\M-a"), ["\033", "a"])
         self.assertEqual(parse_keys("\\M-b"), ["\033", "b"])
         self.assertEqual(parse_keys("\\M-c"), ["\033", "c"])
 
-    def test_keynames(self):
-        self.assertEqual(parse_keys("\\<up>"), ["up"])
-        self.assertEqual(parse_keys("\\<down>"), ["down"])
-        self.assertEqual(parse_keys("\\<left>"), ["left"])
-        self.assertEqual(parse_keys("\\<right>"), ["right"])
-
     def test_combinations(self):
         self.assertEqual(parse_keys("\\C-a\\n\\<up>"), ["\x01", "\n", "up"])
         self.assertEqual(parse_keys("\\M-a\\t\\<down>"), ["\033", "a", "\t", "down"])
 
+    def test_keyspec_errors(self):
+        cases = [
+            ("\\Ca", "\\C must be followed by `-'"),
+            ("\\ca", "\\C must be followed by `-'"),
+            ("\\C-\\C-", "doubled \\C-"),
+            ("\\Ma", "\\M must be followed by `-'"),
+            ("\\ma", "\\M must be followed by `-'"),
+            ("\\M-\\M-", "doubled \\M-"),
+            ("\\<left", "unterminated \\<"),
+            ("\\<unsupported>", "unrecognised keyname"),
+            ("\\大", "unknown backslash escape"),
+            ("\\C-\\<backspace>", "\\C- followed by invalid key")
+        ]
+        for test_keys, expected_err in cases:
+            with self.subTest(f"{test_keys} should give error {expected_err}"):
+                with self.assertRaises(KeySpecError) as e:
+                    parse_keys(test_keys)
+                self.assertIn(expected_err, str(e.exception))
+
+    def test_index_errors(self):
+        test_cases = ["\\", "\\C", "\\C-\\C"]
+        for test_keys in test_cases:
+            with self.assertRaises(IndexError):
+                parse_keys(test_keys)
+
 
 class TestCompileKeymap(unittest.TestCase):
     def test_empty_keymap(self):
@@ -72,3 +109,12 @@ def test_nested_multiple_keymaps(self):
         keymap = {b"a": {b"b": {b"c": "action"}}}
         result = compile_keymap(keymap)
         self.assertEqual(result, {b"a": {b"b": {b"c": "action"}}})
+
+    def test_clashing_definitions(self):
+        km = {b'a': 'c', b'a' + b'b': 'd'}
+        with self.assertRaises(KeySpecError):
+            compile_keymap(km)
+
+    def test_non_bytes_key(self):
+        with self.assertRaises(TypeError):
+            compile_keymap({123: 'a'})