Re: [PATCH] PR python/16699: GDB Python command completion with overriden complete vs. completer class

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Tuesday, May 20 2014, Tom Tromey wrote:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> Unfortunately I couldn't come up with a good way to do that.  readline
> Sergio> messes with the text/word being parsed while it is parsing them, so it
> Sergio> is hard to restart the parsing because we may not have the full context
> Sergio> in all situations.  Or at least that's what I found.
>
> I looked through the readline docs here.
> Ok, I see now, the completion API is irredeemably wacky.

:-/

> Sergio> But I fixed my patch according to your comment above, and I think now it
> Sergio> is right.  What I did is simple: instead of providing dummy arguments to
> Sergio> the completer, I am now passing the real arguments.  As far as I have
> Sergio> tested, it works.
>
> Cute.
>
> Sergio> What do you think?  Is it too hacky?
>
> Someday we will invent a hackiness metric to let us know for sure.
> "M(h) is greater than 0.98!  Patch rejected by the robot."

Haha, I can only imagine how the source code of this bot would be...
Probably too hacky?  ;-)

> Seriously, at first I thought this was probably a bad idea.
> And it is a little weird, since first it word-breaks some random way,
> then redoes the breaking later.
>
> Is there a way to call the Python function just once and store the
> results in the non-enum-return case?  Since otherwise it seems that
> every completion request requires two calls to a
> possibly-expensive-though-we-hope-not completer.

Hm, OK, I came up with the following solution.  Basically, the code that
calls the Python function is isolated and, through a static variable and
a flag to inform whether we are handling brkchars or doing a completion
itself, the function now "caches" the result object of the function
call.

This code is built on the assumption that every completion will handle
brkchars first, and then do the completion itself second, in that order.
I guess it's a safe assumption given the logic behind the code.

> Anyway I'm ok-enough with it I suppose.
>
> Sergio> +void
> Sergio> +set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd,
> Sergio> +	  void (*completer_handle_brkchars) (struct cmd_list_element *self,
> Sergio> +					     const char *text,
> Sergio> +					     const char *word))
> Sergio> +{
> Sergio> +  cmd->completer_handle_brkchars = completer_handle_brkchars; /* Ok.  */
>
> I think the "Ok" comment usually is there as a note to the ARI.
> However, does ARI actually check this line?
> If not -> no comment needed.

Done.

> Sergio> +/* Set the completer_handle_brkchars callback.  */
> Sergio> +
> Sergio> +extern void set_cmd_completer_handle_brkchars (struct cmd_list_element *,
> Sergio> +					       void (*f)
> Sergio> +					       (struct cmd_list_element *,
> Sergio> +						const char *,
> Sergio> +						const char *));
>
> I think the "f" argument should have type "completer_ftype *" rather
> than being spelled out.

I created a new "completer_ftype_void", because the "handle_brkchars"
function does not return anything.

> Sergio> +static void
> Sergio> +cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
> Sergio> +				 const char *text, const char *word)
> Sergio> +{
> Sergio> +  cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
> Sergio> +  PyObject *textobj, *wordobj, *resultobj = NULL;
> Sergio> +  /*  const char dummy_text[] = "dummytext";
> Sergio> +      const char dummy_word[] = "dummyword"; */
>
> No need for the commented-out bits.

Ops, sorry.  Removed.

> Sergio> +# This one should always pass.
> Sergio> +send_gdb "completefileinit ${testdir_complete}\t"
> Sergio> +gdb_test_multiple "" "completefileinit completion" {
> Sergio> +    -re "^completefileinit ${testdir_regex}$" {
> Sergio> +        pass "completefileinit completion"
> Sergio> +    }
>
> FWIW I generally find it simpler to test the "complete" command rather
> than the send_gdb dance.

Me too, but given:

<https://sourceware.org/bugzilla/show_bug.cgi?id=16265>

And:

<https://sourceware.org/ml/gdb-patches/2013-05/msg00567.html>

I am now using TAB whenever I can in the testcases.

> Either way is ok though.

Thanks.  WDYT of the following patch?

-- 
Sergio

gdb/
2014-05-20  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR python/16699
	* cli/cli-decode.c (set_cmd_completer_handle_brkchars): New
	function.
	(add_cmd): Set "completer_handle_brkchars" to NULL.
	* cli/cli-decode.h (struct cmd_list_element)
	<completer_handle_brkchars>: New field.
	* command.h (set_cmd_completer_handle_brkchars): New prototype.
	* completer.c (set_gdb_completion_word_break_characters): New
	function.
	(complete_line_internal): Call "completer_handle_brkchars"
	callback from command.
	* completer.h: Include "command.h".
	(set_gdb_completion_word_break_characters): New prototype.
	* python/py-cmd.c (cmdpy_completer_helper): New function.
	(cmdpy_completer_handle_brkchars): New function.
	(cmdpy_completer): Adjust to use cmdpy_completer_helper.
	(cmdpy_init): Set completer_handle_brkchars to
	cmdpy_completer_handle_brkchars.

gdb/testsuite/
2014-05-20  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR python/16699
	* gdb.python/py-completion.exp: New file.
	* gdb.python/py-completion.py: Likewise.

diff --git a/gdb/cli/cli-decode.c b/gdb/cli/cli-decode.c
index d36ab4e..ba61fa8 100644
--- a/gdb/cli/cli-decode.c
+++ b/gdb/cli/cli-decode.c
@@ -164,6 +164,15 @@ set_cmd_completer (struct cmd_list_element *cmd, completer_ftype *completer)
   cmd->completer = completer; /* Ok.  */
 }
 
+/* See definition in commands.h.  */
+
+void
+set_cmd_completer_handle_brkchars (struct cmd_list_element *cmd,
+			       completer_ftype_void *completer_handle_brkchars)
+{
+  cmd->completer_handle_brkchars = completer_handle_brkchars;
+}
+
 /* Add element named NAME.
    Space for NAME and DOC must be allocated by the caller.
    CLASS is the top level category into which commands are broken down
@@ -239,6 +248,7 @@ add_cmd (const char *name, enum command_class class, void (*fun) (char *, int),
   c->prefix = NULL;
   c->abbrev_flag = 0;
   set_cmd_completer (c, make_symbol_completion_list_fn);
+  c->completer_handle_brkchars = NULL;
   c->destroyer = NULL;
   c->type = not_set_cmd;
   c->var = NULL;
diff --git a/gdb/cli/cli-decode.h b/gdb/cli/cli-decode.h
index c6edc87..684fea8 100644
--- a/gdb/cli/cli-decode.h
+++ b/gdb/cli/cli-decode.h
@@ -176,6 +176,15 @@ struct cmd_list_element
        "baz/foo", return "baz/foobar".  */
     completer_ftype *completer;
 
+    /* Handle the word break characters for this completer.  Usually
+       this function need not be defined, but for some types of
+       completers (e.g., Python completers declared as methods inside
+       a class) the word break chars may need to be redefined
+       depending on the completer type (e.g., for filename
+       completers).  */
+
+    completer_ftype_void *completer_handle_brkchars;
+
     /* Destruction routine for this command.  If non-NULL, this is
        called when this command instance is destroyed.  This may be
        used to finalize the CONTEXT field, if needed.  */
diff --git a/gdb/command.h b/gdb/command.h
index a5040a4..05b286b 100644
--- a/gdb/command.h
+++ b/gdb/command.h
@@ -158,8 +158,16 @@ extern void set_cmd_sfunc (struct cmd_list_element *cmd,
 typedef VEC (char_ptr) *completer_ftype (struct cmd_list_element *,
 					 const char *, const char *);
 
+typedef void completer_ftype_void (struct cmd_list_element *,
+				   const char *, const char *);
+
 extern void set_cmd_completer (struct cmd_list_element *, completer_ftype *);
 
+/* Set the completer_handle_brkchars callback.  */
+
+extern void set_cmd_completer_handle_brkchars (struct cmd_list_element *,
+					       completer_ftype_void *);
+
 /* HACK: cagney/2002-02-23: Code, mostly in tracepoints.c, grubs
    around in cmd objects to test the value of the commands sfunc().  */
 extern int cmd_cfunc_eq (struct cmd_list_element *cmd,
diff --git a/gdb/completer.c b/gdb/completer.c
index 94f70a9..db074af 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -450,6 +450,21 @@ expression_completer (struct cmd_list_element *ignore,
   return location_completer (ignore, p, word);
 }
 
+/* See definition in completer.h.  */
+
+void
+set_gdb_completion_word_break_characters (completer_ftype *fn)
+{
+  /* So far we are only interested in differentiating filename
+     completers from everything else.  */
+  if (fn == filename_completer)
+    rl_completer_word_break_characters
+      = gdb_completer_file_name_break_characters;
+  else
+    rl_completer_word_break_characters
+      = gdb_completer_command_word_break_characters;
+}
+
 /* Here are some useful test cases for completion.  FIXME: These
    should be put in the test suite.  They should be tested with both
    M-? and TAB.
@@ -678,6 +693,9 @@ complete_line_internal (const char *text,
 			   p--)
 			;
 		    }
+		  if (reason == handle_brkchars
+		      && c->completer_handle_brkchars != NULL)
+		    (*c->completer_handle_brkchars) (c, p, word);
 		  if (reason != handle_brkchars && c->completer != NULL)
 		    list = (*c->completer) (c, p, word);
 		}
@@ -751,6 +769,9 @@ complete_line_internal (const char *text,
 		       p--)
 		    ;
 		}
+	      if (reason == handle_brkchars
+		  && c->completer_handle_brkchars != NULL)
+		(*c->completer_handle_brkchars) (c, p, word);
 	      if (reason != handle_brkchars && c->completer != NULL)
 		list = (*c->completer) (c, p, word);
 	    }
diff --git a/gdb/completer.h b/gdb/completer.h
index 5b90773..cb9c389 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -18,6 +18,7 @@
 #define COMPLETER_H 1
 
 #include "gdb_vecs.h"
+#include "command.h"
 
 extern VEC (char_ptr) *complete_line (const char *text,
 				      char *line_buffer,
@@ -48,6 +49,13 @@ extern char *get_gdb_completer_quote_characters (void);
 
 extern char *gdb_completion_word_break_characters (void);
 
+/* Set the word break characters array to the corresponding set of
+   chars, based on FN.  This function is useful for cases when the
+   completer doesn't know the type of the completion until some
+   calculation is done (e.g., for Python functions).  */
+
+extern void set_gdb_completion_word_break_characters (completer_ftype *fn);
+
 /* Exported to linespec.c */
 
 extern const char *skip_quoted_chars (const char *, const char *,
diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
index 524ba5a..7d89131 100644
--- a/gdb/python/py-cmd.c
+++ b/gdb/python/py-cmd.c
@@ -208,45 +208,149 @@ cmdpy_function (struct cmd_list_element *command, char *args, int from_tty)
   do_cleanups (cleanup);
 }
 
+/* Helper function for the Python command completers (both "pure"
+   completer and brkchar handler).  This function takes COMMAND, TEXT
+   and WORD and tries to call the Python method for completion with
+   these arguments.  It also takes HANDLE_BRKCHARS_P, an argument to
+   identify whether it is being called from the brkchar handler or
+   from the "pure" completer.  In the first case, it effectively calls
+   the Python method for completion, and records the PyObject in a
+   static variable (used as a "cache").  In the second case, it just
+   returns that variable, without actually calling the Python method
+   again.  This saves us one Python method call.
+
+   It is important to mention that this function is built on the
+   assumption that the calls to cmdpy_completer_handle_brkchars and
+   cmdpy_completer will be subsequent with nothing intervening.  This
+   is true for our completer mechanism.
+
+   This function returns the PyObject representing the Python method
+   call.  */
+
+static PyObject *
+cmdpy_completer_helper (struct cmd_list_element *command,
+			const char *text, const char *word,
+			int handle_brkchars_p)
+{
+  cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
+  PyObject *textobj, *wordobj;
+  /* This static variable will server as a "cache" for us, in order to
+     store the PyObject that results from calling the Python
+     function.  */
+  static PyObject *resultobj = NULL;
+
+  if (handle_brkchars_p)
+    {
+      /* If we were called to handle brkchars, it means this is the
+	 first function call of two that will happen in a row.
+	 Therefore, we need to call the completer ourselves, and cache
+	 the return value in the static variable RESULTOBJ.  Then, in
+	 the second call, we can just use the value of RESULTOBJ to do
+	 our job.  */
+      resultobj = NULL;
+      if (!obj)
+	error (_("Invalid invocation of Python command object."));
+      if (!PyObject_HasAttr ((PyObject *) obj, complete_cst))
+	{
+	  /* If there is no complete method, don't error.  */
+	  return NULL;
+	}
+
+      textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL);
+      if (!textobj)
+	error (_("Could not convert argument to Python string."));
+      wordobj = PyUnicode_Decode (word, sizeof (word), host_charset (), NULL);
+      if (!wordobj)
+	error (_("Could not convert argument to Python string."));
+
+      resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
+					      textobj, wordobj, NULL);
+      Py_DECREF (textobj);
+      Py_DECREF (wordobj);
+      if (!resultobj)
+	{
+	  /* Just swallow errors here.  */
+	  PyErr_Clear ();
+	}
+    }
+
+  /* The caller is responsible for deciding whether to call Py_XDECREF
+     or not.  */
+  return resultobj;
+}
+
+/* Python function called to determine the break characters of a
+   certain completer.  We are only interested in knowing if the
+   completer registered by the user will return one of the integer
+   codes (see COMPLETER_* symbols).  */
+
+static void
+cmdpy_completer_handle_brkchars (struct cmd_list_element *command,
+				 const char *text, const char *word)
+{
+  PyObject *resultobj = NULL;
+  struct cleanup *cleanup;
+
+  cleanup = ensure_python_env (get_current_arch (), current_language);
+
+  /* Calling our helper to obtain the PyObject of the Python
+     function.  */
+  resultobj = cmdpy_completer_helper (command, text, word, 1);
+
+  /* Check if there was an error.  */
+  if (resultobj == NULL)
+    goto done;
+
+  if (PyInt_Check (resultobj))
+    {
+      /* User code may also return one of the completion constants,
+	 thus requesting that sort of completion.  We are only
+	 interested in this kind of return.  */
+      long value;
+
+      if (!gdb_py_int_as_long (resultobj, &value))
+	{
+	  /* Ignore.  */
+	  PyErr_Clear ();
+	}
+      else if (value >= 0 && value < (long) N_COMPLETERS)
+	{
+	  /* This is the core of this function.  Depending on which
+	     completer type the Python function returns, we have to
+	     adjust the break characters accordingly.  */
+	  set_gdb_completion_word_break_characters
+	    (completers[value].completer);
+	}
+    }
+
+ done:
+
+  /* We do not call Py_XDECREF here because RESULTOBJ will be used in
+     the subsequent call to cmdpy_completer function.  */
+  do_cleanups (cleanup);
+}
+
 /* Called by gdb for command completion.  */
 
 static VEC (char_ptr) *
 cmdpy_completer (struct cmd_list_element *command,
 		 const char *text, const char *word)
 {
-  cmdpy_object *obj = (cmdpy_object *) get_cmd_context (command);
-  PyObject *textobj, *wordobj, *resultobj = NULL;
+  PyObject *resultobj = NULL;
   VEC (char_ptr) *result = NULL;
   struct cleanup *cleanup;
 
   cleanup = ensure_python_env (get_current_arch (), current_language);
 
-  if (! obj)
-    error (_("Invalid invocation of Python command object."));
-  if (! PyObject_HasAttr ((PyObject *) obj, complete_cst))
-    {
-      /* If there is no complete method, don't error -- instead, just
-	 say that there are no completions.  */
-      goto done;
-    }
+  /* Calling our helper to obtain the PyObject of the Python
+     function.  */
+  resultobj = cmdpy_completer_helper (command, text, word, 0);
 
-  textobj = PyUnicode_Decode (text, strlen (text), host_charset (), NULL);
-  if (! textobj)
-    error (_("Could not convert argument to Python string."));
-  wordobj = PyUnicode_Decode (word, strlen (word), host_charset (), NULL);
-  if (! wordobj)
-    error (_("Could not convert argument to Python string."));
-
-  resultobj = PyObject_CallMethodObjArgs ((PyObject *) obj, complete_cst,
-					  textobj, wordobj, NULL);
-  Py_DECREF (textobj);
-  Py_DECREF (wordobj);
-  if (! resultobj)
-    {
-      /* Just swallow errors here.  */
-      PyErr_Clear ();
-      goto done;
-    }
+  /* If the result object of calling the Python function is NULL, it
+     means that there was an error.  In this case, just give up and
+     return NULL.  */
+  if (resultobj == NULL)
+    goto done;
 
   result = NULL;
   if (PyInt_Check (resultobj))
@@ -548,6 +652,9 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
       set_cmd_context (cmd, self);
       set_cmd_completer (cmd, ((completetype == -1) ? cmdpy_completer
 			       : completers[completetype].completer));
+      if (completetype == -1)
+	set_cmd_completer_handle_brkchars (cmd,
+					   cmdpy_completer_handle_brkchars);
     }
   if (except.reason < 0)
     {
diff --git a/gdb/testsuite/gdb.python/py-completion.exp b/gdb/testsuite/gdb.python/py-completion.exp
new file mode 100644
index 0000000..b8b821e
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-completion.exp
@@ -0,0 +1,70 @@
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set testfile "py-completion"
+
+load_lib gdb-python.exp
+
+gdb_exit
+gdb_start
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+
+gdb_test_no_output "source ${srcdir}/${subdir}/${testfile}.py"
+
+# Create a temporary directory
+set testdir "${objdir}/${subdir}/py-completion-testdir/"
+set testdir_regex [string_to_regexp $testdir]
+set testdir_complete "${objdir}/${subdir}/py-completion-test"
+file mkdir $testdir
+
+# This one should always pass.
+send_gdb "completefileinit ${testdir_complete}\t"
+gdb_test_multiple "" "completefileinit completion" {
+    -re "^completefileinit ${testdir_regex}$" {
+        pass "completefileinit completion"
+    }
+}
+
+# Just discarding whatever we typed.
+send_gdb "\n"
+gdb_test "print" ".*"
+
+# This is the problematic one.
+send_gdb "completefilemethod ${testdir_complete}\t"
+gdb_test_multiple "" "completefilemethod completion" {
+    -re "^completefilemethod ${testdir_regex} $" {
+        fail "completefilemethod completion (completed filename as wrong command arg)"
+    }
+    -re "^completefilemethod ${testdir_regex}$" {
+        pass "completefilemethod completion"
+    }
+}
+
+# Discarding again
+send_gdb "\n"
+gdb_test "print" ".*"
+
+# Another problematic
+send_gdb "completefilecommandcond ${objdir}/${subdir}/py-completion-t\t"
+gdb_test_multiple "" "completefilecommandcond completion" {
+    -re "^completefilecommandcond ${testdir}$" {
+	fail "completefilecommandcond completion (completed filename instead of command)"
+    }
+    -re "^completefilecommandcond ${objdir}/${subdir}/py-completion-t$" {
+	pass "completefilecommandcond completion"
+    }
+}
diff --git a/gdb/testsuite/gdb.python/py-completion.py b/gdb/testsuite/gdb.python/py-completion.py
new file mode 100644
index 0000000..23592d0
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-completion.py
@@ -0,0 +1,58 @@
+# Copyright (C) 2014 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This testcase tests PR python/16699
+
+import gdb
+
+class CompleteFileInit(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completefileinit',gdb.COMMAND_USER,gdb.COMPLETE_FILENAME)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+class CompleteFileMethod(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completefilemethod',gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		return gdb.COMPLETE_FILENAME
+
+class CompleteFileCommandCond(gdb.Command):
+	def __init__(self):
+		gdb.Command.__init__(self,'completefilecommandcond',gdb.COMMAND_USER)
+
+	def invoke(self,argument,from_tty):
+		raise gdb.GdbError('not implemented')
+
+	def complete(self,text,word):
+		# This is a test made to know if the command
+		# completion still works fine.  When the user asks to
+		# complete something like "completefilecommandcond
+		# /path/to/py-completion-t", it should not complete to
+		# "/path/to/py-completion-test/", but instead just
+		# wait for input.
+		if "py-completion-t" in text:
+			return gdb.COMPLETE_COMMAND
+		else:
+			return gdb.COMPLETE_FILENAME
+
+CompleteFileInit()
+CompleteFileMethod()
+CompleteFileCommandCond()