`Module/readline.c` has suspicious macro

[sobolevn]

Bug report

readline has two identical function signatures under different macro branches:

https://github.com/python/cpython/blob/f75cefd402c4c830228d85ca3442377ebaf09454/Modules/readline.c#L1019-L1024

and

https://github.com/python/cpython/blob/f75cefd402c4c830228d85ca3442377ebaf09454/Modules/readline.c#L1033-L1039

Looks like that they became identical after https://github.com/python/cpython/commit/a9305b5e80e414b8b9b2bd366e96b43add662d70

Original issue when this macro was introduced: https://github.com/python/cpython/issues/64573 But, new versions of readline do not ship _RL_FUNCTION_TYPEDEF anymore: https://git.savannah.gnu.org/cgit/readline.git/tree/rltypedefs.h#n50

So, what should we do?

1. Remove #if defined
2. Revert https://github.com/python/cpython/commit/a9305b5e80e414b8b9b2bd366e96b43add662d70

[CorvinM]

If we care about support for old readline versions, then option 2 as the commit likely broke it. If we don't then I'd say option 1 for just a bit of code cleanup.

It looks like readline library has always defined _RL_FUNCTION_TYPEDEF since at least version 4.2 (circa 2001)

[sobolevn]

cc @iritkatriel

[iritkatriel]

I made that commit to silence some warning. I don't have an opinion on this.

[erlend-aasland]

I currently get a compiler warning for readline.c on my Mac. The warning disappears if I revert a9305b5e80e414b8b9b2bd366e96b43add662d70.

$ touch Modules/readline.c
$ make
gcc  -fno-strict-overflow -g -Og -Wall -fprofile-instr-generate -fcoverage-mapping -Og   -std=c11 -Werror=implicit-function-declaration -fvisibility=hidden  -I/Users/erlend.aasland/src/cpython.git/Include/internal -IObjects -IInclude -IPython -I. -I/Users/erlend.aasland/src/cpython.git/Include     -c /Users/erlend.aasland/src/cpython.git/Modules/readline.c -o Modules/readline.o
/Users/erlend.aasland/src/cpython.git/Modules/readline.c:1257:21: warning: incompatible function pointer types assigning to 'Function *' (aka 'int (*)(const char *, int)') from 'int (void)' [-Wincompatible-function-pointer-types]
    rl_startup_hook = on_startup_hook;
                    ^ ~~~~~~~~~~~~~~~
/Users/erlend.aasland/src/cpython.git/Modules/readline.c:1259:23: warning: incompatible function pointer types assigning to 'Function *' (aka 'int (*)(const char *, int)') from 'int (void)' [-Wincompatible-function-pointer-types]
    rl_pre_input_hook = on_pre_input_hook;
                      ^ ~~~~~~~~~~~~~~~~~
2 warnings generated.
gcc -bundle -undefined dynamic_lookup -fprofile-instr-generate -fcoverage-mapping     Modules/readline.o -lreadline  -o Modules/readline.cpython-313d-darwin.so
Checked 107 modules (31 built-in, 76 shared, 0 n/a on macosx-13.4-arm64, 0 disabled, 0 missing, 0 failed on import)
$ git revert a9305b5e80e414b8b9b2bd366e96b43add662d70
$ make
gcc  -fno-strict-overflow -g -Og -Wall -fprofile-instr-generate -fcoverage-mapping -Og   -std=c11 -Werror=implicit-function-declaration -fvisibility=hidden  -I/Users/erlend.aasland/src/cpython.git/Include/internal -IObjects -IInclude -IPython -I. -I/Users/erlend.aasland/src/cpython.git/Include     -c /Users/erlend.aasland/src/cpython.git/Modules/readline.c -o Modules/readline.o
gcc -bundle -undefined dynamic_lookup -fprofile-instr-generate -fcoverage-mapping     Modules/readline.o -lreadline  -o Modules/readline.cpython-313d-darwin.so
Checked 107 modules (31 built-in, 76 shared, 0 n/a on macosx-13.4-arm64, 0 disabled, 0 missing, 0 failed on import)

[erlend-aasland]

See #105323, which appeared a few days after a9305b5e80e414b8b9b2bd366e96b43add662d70

[erlend-aasland]

@iritkatriel, do you get a compiler warning if you revert a9305b5e80e414b8b9b2bd366e96b43add662d70?

[iritkatriel]

Go ahead and revert it. I don't remember the details of why I put it in.

[iritkatriel]

I do get a warning actually when I revert it:

./Modules/readline.c:1023:16: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_startup_hook()
               ^
                void
./Modules/readline.c:1038:18: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_pre_input_hook()
                 ^
                  void

[erlend-aasland]

@sobolevn, are you on it?

[erlend-aasland]

I do get a warning actually when I revert it:

./Modules/readline.c:1023:16: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_startup_hook()
               ^
                void
./Modules/readline.c:1038:18: warning: a function declaration without a prototype is deprecated in all versions of C [-Wstrict-prototypes]
on_pre_input_hook()
                 ^
                  void

Right, so we should try and find out the real fix, then.

[erlend-aasland]

What's the output of grep READLINE Makefile config.log, @iritkatriel?

[sobolevn]

@erlend-aasland no, I don't work on this issue, I don't have any knowledge about readline :)

[erlend-aasland]

@corona10: suggesting to close #105323 as superseded, since we've got more information in this discussion.

[iritkatriel]

% grep READLINE Makefile config.log
Makefile:MODULE_READLINE_STATE=yes
Makefile:MODULE_READLINE_CFLAGS=
Makefile:MODULE_READLINE_LDFLAGS=-lreadline
Makefile:Modules/readline.o: $(srcdir)/Modules/readline.c $(MODULE_READLINE_DEPS) $(MODULE_DEPS_SHARED) $(PYTHON_HEADERS); $(CC) $(MODULE_READLINE_CFLAGS) $(PY_STDMODULE_CFLAGS) $(CCSHARED) -c $(srcdir)/Modules/readline.c -o Modules/readline.o
Makefile:Modules/readline$(EXT_SUFFIX):  Modules/readline.o; $(BLDSHARED)  Modules/readline.o $(MODULE_READLINE_LDFLAGS)  -o Modules/readline$(EXT_SUFFIX)
config.log:configure:23783: checking for LIBREADLINE
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:| #define HAVE_READLINE_READLINE_H 1
config.log:ac_cv_env_LIBREADLINE_CFLAGS_set=
config.log:ac_cv_env_LIBREADLINE_CFLAGS_value=
config.log:ac_cv_env_LIBREADLINE_LIBS_set=
config.log:ac_cv_env_LIBREADLINE_LIBS_value=
config.log:LIBREADLINE_CFLAGS=''
config.log:LIBREADLINE_LIBS=''
config.log:MODULE_READLINE_CFLAGS=
config.log:MODULE_READLINE_FALSE='#'
config.log:MODULE_READLINE_LDFLAGS=-lreadline
config.log:MODULE_READLINE_STATE=yes
config.log:MODULE_READLINE_TRUE=''
config.log:#define HAVE_READLINE_READLINE_H 1

[corona10]

See https://github.com/python/cpython/issues/105323, which appeared a few days after https://github.com/python/cpython/commit/a9305b5e80e414b8b9b2bd366e96b43add662d70

This issue is not related to whether we have to declare the params as void or not, it's just difference between explict declaration or not.

@corona10: suggesting to close https://github.com/python/cpython/issues/105323 as superseded, since we've got more information in this discussion.

I am not sure, so it looks like, the right solution is detecting different signatures between different readline versions and compiling with conditional compilation as we discussed from https://github.com/python/cpython/issues/105323? no?

[erlend-aasland]

I am not sure, so it looks like, the right solution is detecting different signatures between different readline versions and compiling with conditional compilation as we discussed from #105323? no?

Your issue is solved for me if I revert Irit's commit, so I do think they are related.

[erlend-aasland]

@corona10, see https://github.com/python/cpython/issues/108588#issuecomment-1697181118.

[corona10]

Your issue is solved for me if I revert Irit's commit, so I do think they are related.

If you are arguing that the issue is just about ignoring the compiler warning, you are right. But if we are focusing on the on_pre_input_hook and on_startup_hook are not declared as the proper params for each readline versions, the issue is not yet solved.

[erlend-aasland]

All right, I leave it to you to consider if this issue, #105323, and #105240 are related :) I don't have the bandwidth for this. FTR, here's some info from my machine:

$ echo '#include <editline/readline.h>' | gcc -E - | grep "typedef.*Function"
typedef int Function(const char *, int);
typedef void VFunction(void);
typedef void VCPFunction(char *);
typedef char *CPFunction(const char *, int);
typedef char **CPPFunction(const char *, int, int);
$ echo '#include <readline/readline.h>' | gcc -E - | grep "typedef.*Function"
typedef int Function(const char *, int);
typedef void VFunction(void);
typedef void VCPFunction(char *);
typedef char *CPFunction(const char *, int);
typedef char **CPPFunction(const char *, int, int);

[corona10]

Yeah, when I reviewed the https://github.com/python/cpython/pull/105241, I didn't deeply investigate the issue. It was not a proper patch at that moment, but we discovered the root cause from https://github.com/python/cpython/issues/105323 :(

I think that it's still worth to solve the issue.

[corona10]

Patch is submitted for macOS: https://github.com/python/cpython/pull/108633

[ned-deily]

I don't have a lot of insight into this and I likely won't have time to look at it further for the next two weeks. But, AFAIK, there are at least three cases of interest that need to be covered: 1. linking the Python readline module with GNU readline; 2. linking with the latest BSD editline (libedit) module; and 3. linking with the version of BSD editline (libedit) that Apple ships with macOS. Note that editline provides an old GNU readline compatibility shim and that is what we link to when using any version of libedit, i.e. we do not support the native editline/libedit API. All three of those cases are being used today on macOS by various distributors so we should be sure they all work. The first two are used on other platforms and should also be tested. There may be other configurations of interest.

[erlend-aasland]

I think we can close this now that #117870 has landed.