[PATCH v1] gdb/DAP - Add completionsRequest

[PATCH v1] gdb/DAP - Add completionsRequest

[Simon Farre]

Self-explanatory.
---
 gdb/data-directory/Makefile.in       |  1 +
 gdb/python/lib/gdb/dap/__init__.py   |  1 +
 gdb/python/lib/gdb/dap/completion.py | 45 ++++++++++++++++++++++++++++
 3 files changed, 47 insertions(+)
 create mode 100644 gdb/python/lib/gdb/dap/completion.py

diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index a3775a4a666..c1794b5acc7 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -89,6 +89,7 @@ PYTHON_FILE_LIST = \
 	gdb/command/xmethods.py \
 	gdb/dap/breakpoint.py \
 	gdb/dap/bt.py \
+	gdb/dap/completion.py \
 	gdb/dap/disassemble.py \
 	gdb/dap/evaluate.py \
 	gdb/dap/events.py \
diff --git a/gdb/python/lib/gdb/dap/__init__.py b/gdb/python/lib/gdb/dap/__init__.py
index f3dd3ff7ea8..e2dcde251ee 100644
--- a/gdb/python/lib/gdb/dap/__init__.py
+++ b/gdb/python/lib/gdb/dap/__init__.py
@@ -32,6 +32,7 @@ from . import pause
 from . import scopes
 from . import sources
 from . import threads
+from . import completion
 
 from .server import Server
 
diff --git a/gdb/python/lib/gdb/dap/completion.py b/gdb/python/lib/gdb/dap/completion.py
new file mode 100644
index 00000000000..861d1b28ac9
--- /dev/null
+++ b/gdb/python/lib/gdb/dap/completion.py
@@ -0,0 +1,45 @@
+# Copyright 2023 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/>.
+
+import gdb
+
+from .server import request, capability
+from .startup import send_gdb_with_response
+
+from typing import Optional
+
+
+def _completions(text: str, column: int, line: Optional[int], frameId: Optional[int]):
+    cmd_output = gdb.execute("complete " + text, to_string=True)
+    result = []
+    for line in cmd_output.splitlines():
+        if "List may be truncated" not in line:
+            result.append({"label": line, "type": "function", "length": len(text)})
+    return {"targets": result}
+
+
+@request("completions")
+@capability("supportsCompletionsRequest")
+@capability("completionTriggerCharacters", [" ", "."])
+def completions(
+    *,
+    text: str,
+    column: int,
+    line: Optional[int] = None,
+    frameId: Optional[int] = None,
+    **extra
+):
+
+    return send_gdb_with_response(lambda: _completions(text, column, line, frameId))
-- 
2.41.0

Re: [PATCH v1] gdb/DAP - Add completionsRequest

[Lancelot SIX]

> +def _completions(text: str, column: int, line: Optional[int], frameId: Optional[int]):
> +    cmd_output = gdb.execute("complete " + text, to_string=True)
> +    result = []
> +    for line in cmd_output.splitlines():
> +        if "List may be truncated" not in line:

Hi Simon,

Instead of filtering out this message, would it make sense to allow GDB
to list all possible completions without size limit?

This can be done with:

    cmd_output = gdb.execute("with max-completions unlimited -- complete " + text,
                             to_string=True)

I am not very familiar with the DAP protocol, but looking at it I did
not see a mechanism to limit the number of items returned, or a way for
GDB to let the client know that more options are available.  Is there a
way to do this?

Best,
Lancelot.

> +            result.append({"label": line, "type": "function", "length": len(text)})
> +    return {"targets": result}
> +
> +
> +@request("completions")
> +@capability("supportsCompletionsRequest")
> +@capability("completionTriggerCharacters", [" ", "."])
> +def completions(
> +    *,
> +    text: str,
> +    column: int,
> +    line: Optional[int] = None,
> +    frameId: Optional[int] = None,
> +    **extra
> +):
> +
> +    return send_gdb_with_response(lambda: _completions(text, column, line, frameId))
> -- 
> 2.41.0
> 

Re: [PATCH v1] gdb/DAP - Add completionsRequest

[Simon Farre]

> Hi Simon,
>
> Instead of filtering out this message, would it make sense to allow GDB
> to list all possible completions without size limit?
>
> This can be done with:
>
>      cmd_output = gdb.execute("with max-completions unlimited -- complete " + text,
>                               to_string=True)
>
> I am not very familiar with the DAP protocol, but looking at it I did
> not see a mechanism to limit the number of items returned, or a way for
> GDB to let the client know that more options are available.  Is there a
> way to do this?

That is a great idea, I had no idea that it could be done like that. 
Like you said, unfortunately (for some reason) a "max count"
is not provided by the request in the protocol.

I'll fix this in v2 and for good measure throw in tests for the request 
as well.

Thanks for your input!

Re: [PATCH v1] gdb/DAP - Add completionsRequest

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Self-explanatory.

I think it's preferable to write something anyway.

Simon> +
Simon> +def _completions(text: str, column: int, line: Optional[int], frameId: Optional[int]):

This should be marked @in_gdb_thread.

Since 'line' isn't used, I think it can just be removed here and also
from 'completions' itself, as it is optional anyway.

Shouldn't 'column' be used?  Like perhaps the text should be truncated
at the given column.  (It's also fine to do this work in the main
thread.)

Simon> +def completions(
Simon> +    *,
Simon> +    text: str,
Simon> +    column: int,
Simon> +    line: Optional[int] = None,
Simon> +    frameId: Optional[int] = None,

I notice that frameId doesn't do anything either.  Since it's optional,
I suppose it could just be dropped here.  gdb doesn't really have this
concept, though we did add it for expression evaluation to support DAP,
so maybe it should also be added for completion.

Tom

Re: [PATCH v1] gdb/DAP - Add completionsRequest

[Simon Farre]

> I think it's preferable to write something anyway.
I'll update next patch with descriptive commit message.
>
> Simon> +
> Simon> +def _completions(text: str, column: int, line: Optional[int], frameId: Optional[int]):
>
> This should be marked @in_gdb_thread.
>
> Since 'line' isn't used, I think it can just be removed here and also
> from 'completions' itself, as it is optional anyway.

Right, I added it to make it look 1-to-1 with the DAP spec, _even 
though_ we don't actually use them in GDB,
as I find this approach less confusing - if a GDB contributor (or user) 
were to look up the signature, then look at the DAP spec
and not see the same things. But I'll remove the things that doesn't 
make sense, as it were, in GDB-land in the next patch.

> Shouldn't 'column' be used?  Like perhaps the text should be truncated
> at the given column.  (It's also fine to do this work in the main
> thread.)
I tried doing that, and no matter how or what I did, VSCode ended up 
just vomitting out the wrong result. I'm not sure
if that's an error I made or VSCode not being exactly compliant with the 
spec. This patch was the one that finally made
the request work in VSCode. I'll have another look at it though.

Thanks!

Re: [PATCH v1] gdb/DAP - Add completionsRequest

[Tom Tromey]

>>>>> "Simon" == Simon Farre via Gdb-patches <gdb-patches@sourceware.org> writes:

>> Since 'line' isn't used, I think it can just be removed here and also
>> from 'completions' itself, as it is optional anyway.

Simon> Right, I added it to make it look 1-to-1 with the DAP spec, _even
Simon> though_ we don't actually use them in GDB,
Simon> as I find this approach less confusing - if a GDB contributor (or
Simon> user) were to look up the signature, then look at the DAP spec
Simon> and not see the same things. But I'll remove the things that doesn't
Simon> make sense, as it were, in GDB-land in the next patch.

For the DAP side (the @request) this is arguable but also fine.
I personally think it's clearer to only name parameters that are
actually used, but if you want to do it that way, I'll approve it.

On the gdb side (@in_gdb_thread) I think it's better to only accept
things that are actually used.  It's also arguably better here to do any
needed pre-processing in the DAP thread.

thanks,
Tom