Skip to content

gh-112105: Make completer delims work on libedit #112106

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.

Sign up for GitHub

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 3 commits into from
Nov 28, 2023
Merged

gh-112105: Make completer delims work on libedit #112106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7b05fc
Make completer delims work on libedit
gaogaotiantian Nov 15, 2023
b0841a7
📜🤖 Added by blurb_it.
blurb-it[bot] Nov 15, 2023
1941e94
Reduce side effects for rl_basic_word_break_characters
gaogaotiantian Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions Lib/test/test_readline.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import sys
import tempfile
import textwrap
import unittest
from test.support import verbose
from test.support.import_helper import import_module
Expand Down Expand Up @@ -163,6 +164,25 @@ def test_auto_history_disabled(self):
# end, so don't expect it in the output.
self.assertIn(b"History length: 0", output)

def test_set_complete_delims(self):
script = textwrap.dedent("""
import readline
def complete(text, state):
if state == 0 and text == "$":
return "$complete"
return None
if "libedit" in getattr(readline, "__doc__", ""):
readline.parse_and_bind(r'bind "\\t" rl_complete')
else:
readline.parse_and_bind(r'"\\t": complete')
readline.set_completer_delims(" \\t\\n")
readline.set_completer(complete)
print(input())
""")

output = run_pty(script, input=b"$\t\n")
self.assertIn(b"$complete", output)

def test_nonascii(self):
loc = locale.setlocale(locale.LC_CTYPE, None)
if loc in ('C', 'POSIX'):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2023-11-15-04-53-37.gh-issue-112105.I3RcVN.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Make :func:`readline.set_completer_delims` work with libedit
16 changes: 16 additions & 0 deletions Modules/readline.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,6 +576,13 @@ readline_set_completer_delims(PyObject *module, PyObject *string)
if (break_chars) {
free(completer_word_break_characters);
completer_word_break_characters = break_chars;
#ifdef WITH_EDITLINE
rl_basic_word_break_characters = break_chars;
Copy link
Member

Choose a reason for hiding this comment

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

Okay so it's kinds of implementation detail.
Since we enable the flag for libedit in following way.

cpython/configure.ac

Line 5916 in 2e632fa

AH_TEMPLATE([WITH_EDITLINE], [Define to build the readline module against libedit.])

What about changing the code for the libedit implementation only?

Suggested change
rl_basic_word_break_characters = break_chars;
#ifdef WITH_EDITLINE
// Comment why this is needed
rl_basic_word_break_characters = break_chars;
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

From the comments in the file:

 * This emulation library is not 100% API compatible with the "real" readline
 * and cannot be detected at compile-time,
 * hence we use a runtime check to detect if the Python readline module is
 * linked to libedit.

It seems like we are not able to detect if libedit is used at compile time. I doubt if the macro protection would actually work in all cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do our official binaries build with libedit ever?

Copy link
Member

@corona10 corona10 Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like we are not able to detect if libedit is used at compile time

I am not sure when the comment was added.
But we can detect editline at build-time from Python 3.10
https://docs.python.org/3/using/configure.html?highlight=with_editline#cmdoption-with-readline

Copy link
Member

Choose a reason for hiding this comment

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

#24189

Copy link
Member

@corona10 corona10 Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do our official binaries build with libedit ever?

Even if our official build does not use it, we should consider downstream distros :)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually not what I meant. What I meant is - we could have been using libedit without WITH_EDITLINE. So even if we do not define WITH_EDITLINE, we could still be using libedit. I just confirmed on my mac that this change will break the test.

The reason I quoted that comment was that I believed such case exists - where we don't know if libedit is being used at compile time.

Copy link
Member

@corona10 corona10 Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

we could have been using libedit without WITH_EDITLINE.
From what I can observe, writing to both variables has no unexpected side effect, because rl_basic_word_break_characters is not used on CPython with GNU deadline.

Got it, then should we consider using both using_libedit_emulation and WITH_EDITLINE?
If you think that we don't have to consider side effect from the implementation detail, then we can just add a comment why we do this.

Copy link
Member

@corona10 corona10 Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I checked that following suggestion pass the test on my macOS.

Suggested change
rl_basic_word_break_characters = break_chars;
#ifdef WITH_EDITLINE
rl_basic_word_break_characters = break_chars;
#else
if (using_libedit_emulation) rl_basic_word_break_characters = break_chars;
#endif

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I did not use your suggestion directly. I do like the brackets for the if statement. I applied the protection to avoid side effects for GNU readline on both cases.

#else
if (using_libedit_emulation) {
rl_basic_word_break_characters = break_chars;
}
#endif
rl_completer_word_break_characters = break_chars;
Py_RETURN_NONE;
}
Expand Down Expand Up @@ -1267,6 +1274,15 @@ setup_readline(readlinestate *mod_state)
completer_word_break_characters =
strdup(" \t\n`~!@#$%^&*()-=+[{]}\\|;:'\",<>/?");
/* All nonalphanums except '.' */
#ifdef WITH_EDITLINE
// libedit uses rl_basic_word_break_characters instead of
// rl_completer_word_break_characters as complete delimiter
rl_basic_word_break_characters = completer_word_break_characters;
Copy link
Member

Choose a reason for hiding this comment

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

ditto
https://github.com/python/cpython/pull/112106/files#r1406981744

#else
if (using_libedit_emulation) {
rl_basic_word_break_characters = completer_word_break_characters;
}
#endif
rl_completer_word_break_characters = completer_word_break_characters;

mod_state->begidx = PyLong_FromLong(0L);
Expand Down