Skip to content

gh-133363: Fix Cmd completion for lines beginning with ! #133364

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

Merged
merged 4 commits into from
May 4, 2025

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented May 4, 2025

When a line begins with ! and there's no do_shell method defined, parseline returns None as the cmd, which incorrectly leads to None being concatenated to complete_ and triggering a TypeError.

Instead, recognize None as a sentinel that means we should call completedefault, as an empty string already is.

When a line begins with `!` and there's no `do_shell` method defined,
`parsecmd` returns `None` as the `cmd`, which incorrectly leads to
`None` being concatenated to `complete_` and triggering a `TypeError`.

Instead, recognize `None` as a sentinel that means we should call
`completedefault`, as an empty string already is.

output = run_pty(script, input)

self.assertIn(b'ello', output)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ello because that's what is completed? hello is not part of the output?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because that's what is used in the previous test.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ello because that's what's completed, yes. hello is part of the output, but I'm just matching the test above that I copy-pasted from, which only checks for ab_completion_test, heh

The tests do both pass if I adjust them to check for hello and tab_completion_test respectively.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized I co-authored that test. Ha.

The :class:`cmd.Cmd` class has been fixed to call the ``completedefault``
method whenever the ``do_shell`` method is not defined and tab completion is
requested for a line beginning with ``!``. Previously ``completedefault``
was called only if there were no spaces between the ``!`` and the cursor
Copy link
Member

@gaogaotiantian gaogaotiantian May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously completedefault was called only if there were no spaces between the ! and the cursor position where tab completion was attempted.

Is that even true? I thought the completion never works:

        elif line[0] == '!':
            if hasattr(self, 'do_shell'):
                line = 'shell ' + line[1:]
            else:
                return None, None, line

Anyway, normally in the news we only need to say what is fixed, we don't need to do a complete description of what's the previous behavior. News entry should be a concise sentence.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that even true? I thought the completion never works:

Yes, because parsecmd is only called if begidx > 0, so in the !foo<Tab> case (begidx=0 endidx=3) we don't hit this buggy codepath.

Anyway, normally in the news we only need to say what is fixed, we don't need to do a complete description of what's the previous behavior. News entry should be a concise sentence.

OK, I've shortened it and just say that it's fixed to reliably call the method, since previously it was inconsistent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay but it's still not good, because that's not really a name. It still won't give us the correct completion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not worry about that, we can do that in pdb I believe. I don't want to change cmd so close to beta freeze. I'll loop back to this after beta freeze.

Copy link
Contributor Author

@godlygeek godlygeek May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay but it's still not good, because that's not really a name. It still won't give us the correct completion.

True, though that seems to need a bigger change if you want to fix that... Do you want to special case ! at the start of the line, and do:

            if begidx>0:
                ...
            elif line.startswith("!"):
                compfunc = self.completedefault
            else:
                compfunc = self.completenames

Even that's not enough to make PDB's completion work without the space, though, because that'll give a text="!h" and PDB will say that none of the object names it knows of start with "!". Which means that making that work requires changes in the PDB module, too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wrong. ! was by default a delimiter and that's why your current code passes. cmd is handling this correctly because begidx will >0 when you complete !h<tab>. pdb somehow removed ! from delimiters and I did not figure out why. However, we can deal with in pdb only, this is not a cmd issue.

""")

# '! h' and complete 'ello' to '! hello'
input = b"! h\t\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do add check here for !h\t\n (without space) as well. No need for another method, just another input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does need to be another method, because the assertions are:

        self.assertIn(b'hello', output)
        self.assertIn(b'tab completion success', output)

and those would pass if either of the two inputs succeeded, not only if both of them did.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we could use a subtest for this? I know how to avoid duplication with pytest, but I'm not really familiar with unittest. Does this work?

        # '! h' or '!h' and complete 'ello' to 'hello'
        for input in [b"! h\t\n", b"!h\t\n"]:
            with self.subTest(input=input):
                output = run_pty(script, input)
                self.assertIn(b'hello', output)
                self.assertIn(b'tab completion success', output)

Seems to...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean just use the same script and do two run_pty - I don't think you even need a subtest. It's a very small case.

@gaogaotiantian gaogaotiantian merged commit 1d9406e into python:main May 4, 2025
43 checks passed
@godlygeek godlygeek deleted the fix_cmd_bang_completion branch May 4, 2025 03:34
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot PPC64LE RHEL8 3.x (tier-2) has failed when building commit 1d9406e.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/559/builds/5937) and take a look at the build logs.
  4. Check if the failure is related to this commit (1d9406e) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/#/builders/559/builds/5937

Failed tests:

  • test.test_multiprocessing_spawn.test_processes

Failed subtests:

  • test_interrupt - test.test_multiprocessing_spawn.test_processes.WithProcessesTestProcess.test_interrupt

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "<string>", line 1, in <module>
    from multiprocessing.spawn import spawn_main; spawn_main(tracker_fd=6, pipe_handle=8)
                                                  ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/spawn.py", line 122, in spawn_main
    exitcode = _main(fd, parent_sentinel)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/spawn.py", line 132, in _main
    self = reduction.pickle.load(from_parent)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/test/test_multiprocessing_spawn/test_processes.py", line 2, in <module>
    from test._test_multiprocessing import install_tests_in_module_dict
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/test/_test_multiprocessing.py", line 53, in <module>
    import multiprocessing.managers
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/managers.py", line 1182, in <module>
    _BaseDictProxy = MakeProxyType('_BaseDictProxy', (
        '__contains__', '__delitem__', '__getitem__', '__ior__', '__iter__',
    ...<2 lines>...
        'keys', 'pop', 'popitem', 'setdefault', 'update', 'values'
        ))
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/managers.py", line 978, in MakeProxyType
    exec('''def %s(self, /, *args, **kwds):
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    return self._callmethod(%r, args, kwds)''' % (meth, meth), dic)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<string>", line 0, in <module>
    
    
    from multiprocessing.spawn import spawn_main; spawn_main(tracker_fd=6, pipe_handle=8)
    
KeyboardInterrupt
FAIL


Traceback (most recent call last):
  File "<string>", line 1, in <module>
    from multiprocessing.spawn import spawn_main; spawn_main(tracker_fd=5, pipe_handle=9)
                                                  ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/spawn.py", line 122, in spawn_main
    exitcode = _main(fd, parent_sentinel)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/multiprocessing/spawn.py", line 132, in _main
    self = reduction.pickle.load(from_parent)
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/test/test_multiprocessing_spawn/test_processes.py", line 2, in <module>
    from test._test_multiprocessing import install_tests_in_module_dict
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/test/_test_multiprocessing.py", line 6, in <module>
    import unittest.mock
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/unittest/mock.py", line 27, in <module>
    import asyncio
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/asyncio/__init__.py", line 8, in <module>
    from .base_events import *
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/asyncio/base_events.py", line 45, in <module>
    from . import staggered
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/asyncio/staggered.py", line 10, in <module>
    from . import tasks
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/asyncio/tasks.py", line 29, in <module>
    from . import timeouts
  File "<frozen importlib._bootstrap>", line 1371, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1342, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 938, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 758, in exec_module
  File "<frozen importlib._bootstrap_external>", line 891, in get_code
  File "<frozen importlib._bootstrap_external>", line 514, in _compile_bytecode
KeyboardInterrupt
FAIL


Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-ppc64le/build/Lib/test/_test_multiprocessing.py", line 578, in test_interrupt
    self.assertEqual(exitcode, 1)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
AssertionError: -2 != 1

diegorusso added a commit to diegorusso/cpython that referenced this pull request May 4, 2025
* origin/main: (111 commits)
  pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385)
  pythongh-131178: Add tests for `ast` command-line interface (python#133329)
  Regenerate pcbuild.sln in Visual Studio 2022 (python#133394)
  pythongh-133042: disable HACL* HMAC on Emscripten (python#133064)
  pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387)
  pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946)
  pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388)
  pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830)
  pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368)
  pythongh-132457: make staticmethod and classmethod generic (python#132460)
  pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812)
  pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490)
  pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517)
  pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628)
  pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358)
  bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226)
  pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287)
  pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364)
  pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027)
  pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants