bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

I stumbled upon the following issue while checking out the new
dockerfile-ts-mode.

1. emacs -Q
2. C-x C-f /tmp/Dockerfile
3. Insert the following contents (taken from
https://docs.docker.com/build/building/multi-stage/#use-a-previous-stage-as-a-new-stage):

    FROM ubuntu AS base
    RUN echo "base"

    FROM base AS stage1
    RUN echo "stage1"

    FROM base AS stage2
    RUN echo "stage2"

4. Try to use M-x imenu to navigate to the last build stage
4.a. We get the following imenu--index-alist:

    (("Stage"
      ("ubuntu" . #<marker at 1 in Dockerfile>)
      ("base" . #<marker at 42 in Dockerfile>)
      ("base" . #<marker at 81 in Dockerfile>)))

4.b. Consequently, we can't choose and jump to the last stage, because
dockerfile-ts-mode calls both of the last two stages by the same name
("base").

The crux of the issue is that dockerfile-ts-mode uses a stage's base
image as its name, which seems kinda wrong and leads to ambiguity since
many stages can have a common base image.  Instead, it would be
preferable IMO to use the index of the stage or, when available, its
unique name (such as "stage1" in the example above).

Thanks!



In GNU Emacs 30.0.50 (build 9, x86_64-apple-darwin22.1.0, NS
 appkit-2299.00 Version 13.0 (Build 22A380)) of 2022-12-11 built on
 esmac.lan
Repository revision: 85108d541217f0333860c4f86c3b16b4349f85a4
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2299
System Description:  macOS 13.0

Configured using:
 'configure --with-native-compilation --enable-link-time-optimization
 --with-json --with-xwidgets --with-mailutils --with-imagemagick
 --without-dbus'

Configured features:
ACL GIF GLIB GMP GNUTLS IMAGEMAGICK JPEG JSON LCMS2 LIBXML2 MODULES
NATIVE_COMP NOTIFY KQUEUE NS PDUMPER PNG RSVG SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XIM XWIDGETS ZLIB

Important settings:
  value of $LC_CTYPE: UTF-8
  locale-coding-system: utf-8-unix

Major mode: Dockerfile

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils pp cl-print
byte-opt help-fns radix-tree thingatpt imenu comp comp-cstr warnings
icons subr-x rx cl-macs gv cl-extra help-mode bytecomp byte-compile
dockerfile-ts-mode treesit cl-seq cl-loaddefs cl-lib rmc iso-transl
tooltip cconv eldoc paren electric uniquify ediff-hook vc-hooks
lisp-float-type elisp-mode mwheel term/ns-win ns-win ucs-normalize
mule-util term/common-win tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads xwidget-internal kqueue cocoa ns lcms2 multi-tty
make-network-process native-compile emacs)

Memory information:
((conses 16 111634 8544)
 (symbols 48 7501 0)
 (strings 32 31342 3006)
 (string-bytes 1 961129)
 (vectors 16 23207)
 (vector-slots 8 403727 9539)
 (floats 8 60 40)
 (intervals 56 396 0)
 (buffers 984 14))

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Randy Taylor]

On Sunday, December 11th, 2022 at 15:39, Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> wrote:
> 
> I stumbled upon the following issue while checking out the new
> dockerfile-ts-mode.
> 
> 1. emacs -Q
> 2. C-x C-f /tmp/Dockerfile
> 3. Insert the following contents (taken from
> https://docs.docker.com/build/building/multi-stage/#use-a-previous-stage-as-a-new-stage):
> 
> FROM ubuntu AS base
> RUN echo "base"
> 
> FROM base AS stage1
> RUN echo "stage1"
> 
> FROM base AS stage2
> RUN echo "stage2"
> 
> 4. Try to use M-x imenu to navigate to the last build stage
> 4.a. We get the following imenu--index-alist:
> 
> (("Stage"
> ("ubuntu" . #<marker at 1 in Dockerfile>)
> 
> ("base" . #<marker at 42 in Dockerfile>)
> 
> ("base" . #<marker at 81 in Dockerfile>)))
> 
> 
> 4.b. Consequently, we can't choose and jump to the last stage, because
> dockerfile-ts-mode calls both of the last two stages by the same name
> ("base").
> 
> The crux of the issue is that dockerfile-ts-mode uses a stage's base
> image as its name, which seems kinda wrong and leads to ambiguity since
> many stages can have a common base image. Instead, it would be
> preferable IMO to use the index of the stage or, when available, its
> unique name (such as "stage1" in the example above).
> 
> Thanks!
> 

Thanks, I'll take a look and hopefully reply back with a patch in a bit.

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Randy Taylor]

[-- Attachment #1: Type: text/plain, Size: 1937 bytes --]

On Sunday, December 11th, 2022 at 18:57, Randy Taylor <dev@rjt.dev> wrote:
> 
> On Sunday, December 11th, 2022 at 15:39, Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" bug-gnu-emacs@gnu.org wrote:
> 
> > I stumbled upon the following issue while checking out the new
> > dockerfile-ts-mode.
> > 
> > 1. emacs -Q
> > 2. C-x C-f /tmp/Dockerfile
> > 3. Insert the following contents (taken from
> > https://docs.docker.com/build/building/multi-stage/#use-a-previous-stage-as-a-new-stage):
> > 
> > FROM ubuntu AS base
> > RUN echo "base"
> > 
> > FROM base AS stage1
> > RUN echo "stage1"
> > 
> > FROM base AS stage2
> > RUN echo "stage2"
> > 
> > 4. Try to use M-x imenu to navigate to the last build stage
> > 4.a. We get the following imenu--index-alist:
> > 
> > (("Stage"
> > ("ubuntu" . #<marker at 1 in Dockerfile>)
> > 
> > ("base" . #<marker at 42 in Dockerfile>)
> > 
> > ("base" . #<marker at 81 in Dockerfile>)))
> > 
> > 4.b. Consequently, we can't choose and jump to the last stage, because
> > dockerfile-ts-mode calls both of the last two stages by the same name
> > ("base").
> > 
> > The crux of the issue is that dockerfile-ts-mode uses a stage's base
> > image as its name, which seems kinda wrong and leads to ambiguity since
> > many stages can have a common base image. Instead, it would be
> > preferable IMO to use the index of the stage or, when available, its
> > unique name (such as "stage1" in the example above).
> > 
> > Thanks!
> 
> 
> Thanks, I'll take a look and hopefully reply back with a patch in a bit.
> 

I didn't notice this because I use conult-imenu, which will add an index for any duplicates. imenu doesn't do this, and for other modes like c-mode, c++-mode, org-mode, etc., only the first entry gets shown.

I've attached a patch which will use the stage name if it's available, otherwise use the base image.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-dockerfile-ts-mode-imenu-generation-Bug-5997.patch --]
[-- Type: text/x-patch; name=0001-Improve-dockerfile-ts-mode-imenu-generation-Bug-5997.patch, Size: 1708 bytes --]

From 09ffadd9f5db19174dced572f3f0ac98f7d2a3a3 Mon Sep 17 00:00:00 2001
From: Randy Taylor <dev@rjt.dev>
Date: Sun, 11 Dec 2022 20:50:54 -0500
Subject: [PATCH] Improve dockerfile-ts-mode imenu generation (Bug#59979)

* lisp/progmodes/dockerfile-ts-mode.el (treesit-node-child-by-field-name):
Declare.
(dockerfile-ts-mode--imenu-1): Use stage name if available.
---
 lisp/progmodes/dockerfile-ts-mode.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/progmodes/dockerfile-ts-mode.el b/lisp/progmodes/dockerfile-ts-mode.el
index 544e0f82d6..40d90cc2df 100644
--- a/lisp/progmodes/dockerfile-ts-mode.el
+++ b/lisp/progmodes/dockerfile-ts-mode.el
@@ -33,6 +33,7 @@
 (declare-function treesit-parser-create "treesit.c")
 (declare-function treesit-induce-sparse-tree "treesit.c")
 (declare-function treesit-node-child "treesit.c")
+(declare-function treesit-node-child-by-field-name "treesit.c")
 (declare-function treesit-node-start "treesit.c")
 (declare-function treesit-node-type "treesit.c")
 
@@ -117,8 +118,10 @@ dockerfile-ts-mode--imenu-1
                            children))
          (name (when ts-node
                  (pcase (treesit-node-type ts-node)
-                   ("from_instruction" (treesit-node-text
-                                        (treesit-node-child ts-node 1) t)))))
+                   ("from_instruction"
+                    (treesit-node-text
+                     (or (treesit-node-child-by-field-name ts-node "as")
+                         (treesit-node-child ts-node 1)) t)))))
          (marker (when ts-node
                    (set-marker (make-marker)
                                (treesit-node-start ts-node)))))
-- 
2.38.1

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Randy Taylor <dev@rjt.dev> writes:

> On Sunday, December 11th, 2022 at 18:57, Randy Taylor <dev@rjt.dev> wrote:
>
> I didn't notice this because I use conult-imenu, which will add an
> index for any duplicates. imenu doesn't do this, and for other modes
> like c-mode, c++-mode, org-mode, etc., only the first entry gets
> shown.
>
> I've attached a patch which will use the stage name if it's available,
> otherwise use the base image.

Great :) That's certainly an improvement.

I still maintain that using the base image as an identifier of the build
stage is not that useful, it doesn't really identify the stage in any
way, like identifying functions their arguments doesn't make a lot of
sense.  I do see how in practice that'll probably be fine in most cases.

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Randy Taylor]

On Monday, December 12th, 2022 at 03:28, Eshel Yaron via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> wrote:
> 
> Randy Taylor dev@rjt.dev writes:
> 
> > On Sunday, December 11th, 2022 at 18:57, Randy Taylor dev@rjt.dev wrote:
> > 
> > I didn't notice this because I use conult-imenu, which will add an
> > index for any duplicates. imenu doesn't do this, and for other modes
> > like c-mode, c++-mode, org-mode, etc., only the first entry gets
> > shown.
> > 
> > I've attached a patch which will use the stage name if it's available,
> > otherwise use the base image.
> 
> 
> Great :) That's certainly an improvement.
> 
> I still maintain that using the base image as an identifier of the build
> stage is not that useful, it doesn't really identify the stage in any
> way, like identifying functions their arguments doesn't make a lot of
> sense. I do see how in practice that'll probably be fine in most cases.
> 

We don't get any information about which stage is which beyond the base image and potential alias (which is all Dockerfiles themselves have anyway), so we would need to track that stuff ourselves, and I don't think it's worth the complexity. Dockerfiles give us a solution to this anyway: use aliases.

Perhaps consider trying a different imenu implementation? I use consult-imenu which gives indexes for any duplicates (it also flattens imenu which I really like). Maybe someone could add something like that to imenu.

Can someone install the patch? Thanks.

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Yuan Fu]

Randy Taylor <dev@rjt.dev> writes:

> On Monday, December 12th, 2022 at 03:28, Eshel Yaron via "Bug reports
> for GNU Emacs, the Swiss army knife of text editors"
> <bug-gnu-emacs@gnu.org> wrote:
>> 
>> Randy Taylor dev@rjt.dev writes:
>> 
>> > On Sunday, December 11th, 2022 at 18:57, Randy Taylor dev@rjt.dev wrote:
>> > 
>> > I didn't notice this because I use conult-imenu, which will add an
>> > index for any duplicates. imenu doesn't do this, and for other modes
>> > like c-mode, c++-mode, org-mode, etc., only the first entry gets
>> > shown.
>> > 
>> > I've attached a patch which will use the stage name if it's available,
>> > otherwise use the base image.
>> 
>> 
>> Great :) That's certainly an improvement.
>> 
>> I still maintain that using the base image as an identifier of the build
>> stage is not that useful, it doesn't really identify the stage in any
>> way, like identifying functions their arguments doesn't make a lot of
>> sense. I do see how in practice that'll probably be fine in most cases.
>> 
>
> We don't get any information about which stage is which beyond the
> base image and potential alias (which is all Dockerfiles themselves
> have anyway), so we would need to track that stuff ourselves, and I
> don't think it's worth the complexity. Dockerfiles give us a solution
> to this anyway: use aliases.
>
> Perhaps consider trying a different imenu implementation? I use
> consult-imenu which gives indexes for any duplicates (it also flattens
> imenu which I really like). Maybe someone could add something like
> that to imenu.
>
> Can someone install the patch? Thanks.

Applied, thanks! I’ll leave closing the report to one of you guys :-)

Yuan

bug#59979: 30.0.50; dockerfile-ts-mode imenu issue

[Randy Taylor]

On Monday, December 12th, 2022 at 17:59, Yuan Fu <casouri@gmail.com> wrote:
>
> Applied, thanks! I’ll leave closing the report to one of you guys :-)
> 
> Yuan

Thanks! Closing.