bug#67158: [PATCH] Repair tab-always-indent

bug#67158: [PATCH] Repair tab-always-indent

[Aymeric Agon-Rambosson]

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

Tags: patch

Tags: patch


Hello,

tab-first-completion does not work correctly at the moment, and 
indent-for-tab-command must be modified in several ways to take 
its meaning into account correctly :

Since syntax-after returns a list and not an integer, the forms 
like (eql 2 syn) will always return nil. This was introduced by 
commit c7234011518. We partially revert that commit, although it 
would have been possible to solve this issue by wrapping 
(syntax-after (point)) with syntax-class like so :


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: class.patch --]
[-- Type: text/x-patch, Size: 500 bytes --]

diff --git a/lisp/indent.el b/lisp/indent.el
index 89de0a1d7d1..e5f2acdd33b 100644
--- a/lisp/indent.el
+++ b/lisp/indent.el
@@ -171,7 +171,7 @@ prefix argument is ignored."
     (let ((old-tick (buffer-chars-modified-tick))
           (old-point (point))
 	  (old-indent (current-indentation))
-          (syn (syntax-after (point))))
+          (syn (syntax-class (syntax-after (point)))))
 
       ;; Indent the line.
       (or (not (eq (indent--funcall-widened indent-line-function) 'noindent))

[-- Attachment #3: Type: text/plain, Size: 2847 bytes --]


Feel free to change the commit if you prefer this way.

Then, the semantic of word-or-paren and word-or-paren-or-punct is 
not correctly implemented : in the current state, if 
tab-first-completion is set to word-or-paren, and if 
tab-always-indent is set to complete, and we press tab in the 
middle of a word, the word will get autocompleted despite the 
docstring promising the contrary because :

The following form will correctly return nil :

(and (memq tab-first-completion
           '(word word-or-paren word-or-paren-or-punct))
     (not (memq 2 syn)))

But this one will return non-nil :

(and (memq tab-first-completion
           '(word-or-paren word-or-paren-or-punct))
     (not (or (eql 4 syn)
              (eql 5 syn))))

Since syn is equal to (2) (we are within a word).

The constraints need to be cumulative, since they are evaluated 
until one succeeds. So we simply cumulate them so that 
word-or-paren cannot succeed where word could not, and 
word-or-paren-or-punct cannot succeed when word-or-paren could 
not.

This is my first contribution with email. I have tried to follow 
the guidelines specified in CONTRIBUTE and Sending-Patches. Feel 
free to change the commit message or ask me to do it. I have 
already attributed the Copyright to the FSF because of a previous 
contribution.

Best,

Aymeric Agon-Rambosson


In GNU Emacs 29.1 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.16.0, Xaw3d scroll bars) of 2023-09-20, modified by 
 Debian
 built on X570GP
Windowing system distributor 'The X.Org Foundation', version 
11.0.12101007
System Description: Debian GNU/Linux 12 (bookworm)

Configured using:
 'configure --build x86_64-linux-gnu --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/libexec
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils
 --with-native-compilation=aot --build x86_64-linux-gnu 
 --prefix=/usr
 --sharedstatedir=/var/lib --libexecdir=/usr/libexec
 --localstatedir=/var/lib --infodir=/usr/share/info
 --mandir=/usr/share/man --with-libsystemd --with-pop=yes
 --enable-locallisppath=/etc/emacs:/usr/local/share/emacs/29.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/29.1/site-lisp:/usr/share/emacs/site-lisp
 --with-sound=alsa --without-gconf --with-mailutils
 --with-native-compilation=aot --with-x=yes --with-x-toolkit=lucid
 --with-toolkit-scroll-bars --without-gsettings 'CFLAGS=-g -O2
 -ffile-prefix-map=/build/emacs-G3TJOq/emacs-29.1+1=. -fstack-protector-strong
 -Wformat -Werror=format-security -Wall' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' LDFLAGS=-Wl,-z,relro'

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0001-Repair-tab-always-indent.patch --]
[-- Type: text/patch, Size: 1974 bytes --]

From 8afb0f0ec8e645b41c8da2a5d5156e63fc04bdbd Mon Sep 17 00:00:00 2001
From: Aymeric Agon-Rambosson <aymeric.agon@yandex.com>
Date: Tue, 14 Nov 2023 00:03:46 +0100
Subject: [PATCH] Repair tab-always-indent

Take the values word, word-or-paren, word-or-paren-or-punct correctly
into account in the function indent-for-tab-command :
* syntax-after returns a list, not an integer, either memq or member
must be used (partial revert of c7234011518).
* the constraints on completion-at-point must be cumulative when we go
from word to word-or-paren to word-or-paren-or-punct. Otherwise, tab
always complete with values word-or-paren or word-or-paren-or-punct,
which is not what the docstring seems to say
---
 lisp/indent.el | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lisp/indent.el b/lisp/indent.el
index 89de0a1d7d1..de8101dc76e 100644
--- a/lisp/indent.el
+++ b/lisp/indent.el
@@ -191,13 +191,17 @@ prefix argument is ignored."
                       (eolp))
                  (and (memq tab-first-completion
                             '(word word-or-paren word-or-paren-or-punct))
-                      (not (eql 2 syn)))
+                      (not (memq 2 syn)))
                  (and (memq tab-first-completion
                             '(word-or-paren word-or-paren-or-punct))
-                      (not (or (eql 4 syn)
-                               (eql 5 syn))))
+                      (not (or (memq 2 syn)
+                               (memq 4 syn)
+                               (memq 5 syn))))
                  (and (eq tab-first-completion 'word-or-paren-or-punct)
-                      (not (eql 1 syn)))))
+                      (not (or (memq 2 syn)
+                               (memq 4 syn)
+                               (memq 5 syn)
+                               (memq 1 syn))))))
         (completion-at-point))
 
        ;; If a prefix argument was given, rigidly indent the following
-- 
2.39.2

bug#67158: [PATCH] Repair tab-always-indent

[Eli Zaretskii]

> From: Aymeric Agon-Rambosson <aymeric.agon@yandex.com>
> Date: Tue, 14 Nov 2023 00:43:17 +0100
> 
> tab-first-completion does not work correctly at the moment, and 
> indent-for-tab-command must be modified in several ways to take 
> its meaning into account correctly :

Could you please post a recipe which reproduces the problems you
describe?

> Since syntax-after returns a list and not an integer, the forms 
> like (eql 2 syn) will always return nil. This was introduced by 
> commit c7234011518. We partially revert that commit, although it 
> would have been possible to solve this issue by wrapping 
> (syntax-after (point)) with syntax-class like so :
> 
> diff --git a/lisp/indent.el b/lisp/indent.el
> index 89de0a1d7d1..e5f2acdd33b 100644
> --- a/lisp/indent.el
> +++ b/lisp/indent.el
> @@ -171,7 +171,7 @@ prefix argument is ignored."
>      (let ((old-tick (buffer-chars-modified-tick))
>            (old-point (point))
>  	  (old-indent (current-indentation))
> -          (syn (syntax-after (point))))
> +          (syn (syntax-class (syntax-after (point)))))
>  
>        ;; Indent the line.
>        (or (not (eq (indent--funcall-widened indent-line-function) 'noindent))
> 
> 
> Feel free to change the commit if you prefer this way.
> 
> Then, the semantic of word-or-paren and word-or-paren-or-punct is 
> not correctly implemented : in the current state, if 
> tab-first-completion is set to word-or-paren, and if 
> tab-always-indent is set to complete, and we press tab in the 
> middle of a word, the word will get autocompleted despite the 
> docstring promising the contrary because :
> 
> The following form will correctly return nil :
> 
> (and (memq tab-first-completion
>            '(word word-or-paren word-or-paren-or-punct))
>      (not (memq 2 syn)))
> 
> But this one will return non-nil :
> 
> (and (memq tab-first-completion
>            '(word-or-paren word-or-paren-or-punct))
>      (not (or (eql 4 syn)
>               (eql 5 syn))))
> 
> Since syn is equal to (2) (we are within a word).
> 
> The constraints need to be cumulative, since they are evaluated 
> until one succeeds. So we simply cumulate them so that 
> word-or-paren cannot succeed where word could not, and 
> word-or-paren-or-punct cannot succeed when word-or-paren could 
> not.
> 
> This is my first contribution with email. I have tried to follow 
> the guidelines specified in CONTRIBUTE and Sending-Patches. Feel 
> free to change the commit message or ask me to do it. I have 
> already attributed the Copyright to the FSF because of a previous 
> contribution.

Thanks.

I added Stefan to this discussion.

bug#67158: [PATCH] Repair tab-always-indent

[Aymeric Agon-Rambosson]

Le mardi 14 novembre 2023 à 14:39, Eli Zaretskii <eliz@gnu.org> a 
écrit :

> Could you please post a recipe which reproduces the problems you
> describe?

With emacs -Q.

On the scratch buffer (lisp-interaction-mode), eval the following 
lines :

(setq tab-always-indent 'complete)
(setq tab-first-completion 'word)

Then type "tty" (without the quotes) on an empty line, place point 
on the "y", and evaluate '(syntax-after (point))'.

You get "(2)" in the minibuffer, which is proof you are in the 
middle of a word according to the syntax table.

Then, with the point still on the "y", press TAB (or evaluate 
'(indent-for-tab-command)').

"tty" gets autocompleted to "tty-" and the point is placed after 
the "-".

This is in contradiction with what the docstring says about the 
variable tab-first-completion.

This is also reproducible if you set tab-first-completion to 
either 'word-or-paren or 'word-or-paren-or-punct.

A detailed explanation of what causes this can be found in my 
previous mail. Feel free to ask any questions.

The problem is not reproducible anymore with the patch applied.

Best,

Aymeric

bug#67158: [PATCH] Repair tab-always-indent

[Eli Zaretskii]

> From: Aymeric Agon-Rambosson <aymeric.agon@yandex.com>
> Date: Tue, 14 Nov 2023 00:43:17 +0100
> 
> tab-first-completion does not work correctly at the moment, and 
> indent-for-tab-command must be modified in several ways to take 
> its meaning into account correctly :
> 
> Since syntax-after returns a list and not an integer, the forms 
> like (eql 2 syn) will always return nil. This was introduced by 
> commit c7234011518. We partially revert that commit, although it 
> would have been possible to solve this issue by wrapping 
> (syntax-after (point)) with syntax-class like so :

Stefan, any comments on these two patches?

> diff --git a/lisp/indent.el b/lisp/indent.el
> index 89de0a1d7d1..e5f2acdd33b 100644
> --- a/lisp/indent.el
> +++ b/lisp/indent.el
> @@ -171,7 +171,7 @@ prefix argument is ignored."
>      (let ((old-tick (buffer-chars-modified-tick))
>            (old-point (point))
>  	  (old-indent (current-indentation))
> -          (syn (syntax-after (point))))
> +          (syn (syntax-class (syntax-after (point)))))
>  
>        ;; Indent the line.
>        (or (not (eq (indent--funcall-widened indent-line-function) 'noindent))
> 
> Feel free to change the commit if you prefer this way.
> 
> Then, the semantic of word-or-paren and word-or-paren-or-punct is 
> not correctly implemented : in the current state, if 
> tab-first-completion is set to word-or-paren, and if 
> tab-always-indent is set to complete, and we press tab in the 
> middle of a word, the word will get autocompleted despite the 
> docstring promising the contrary because :
> 
> The following form will correctly return nil :
> 
> (and (memq tab-first-completion
>            '(word word-or-paren word-or-paren-or-punct))
>      (not (memq 2 syn)))
> 
> But this one will return non-nil :
> 
> (and (memq tab-first-completion
>            '(word-or-paren word-or-paren-or-punct))
>      (not (or (eql 4 syn)
>               (eql 5 syn))))
> 
> Since syn is equal to (2) (we are within a word).
> 
> The constraints need to be cumulative, since they are evaluated 
> until one succeeds. So we simply cumulate them so that 
> word-or-paren cannot succeed where word could not, and 
> word-or-paren-or-punct cannot succeed when word-or-paren could 
> not.
> 
> This is my first contribution with email. I have tried to follow 
> the guidelines specified in CONTRIBUTE and Sending-Patches. Feel 
> free to change the commit message or ask me to do it. I have 
> already attributed the Copyright to the FSF because of a previous 
> contribution.
>
> >From 8afb0f0ec8e645b41c8da2a5d5156e63fc04bdbd Mon Sep 17 00:00:00 2001
> From: Aymeric Agon-Rambosson <aymeric.agon@yandex.com>
> Date: Tue, 14 Nov 2023 00:03:46 +0100
> 
> Take the values word, word-or-paren, word-or-paren-or-punct correctly
> into account in the function indent-for-tab-command :
> * syntax-after returns a list, not an integer, either memq or member
> must be used (partial revert of c7234011518).
> * the constraints on completion-at-point must be cumulative when we go
> from word to word-or-paren to word-or-paren-or-punct. Otherwise, tab
> always complete with values word-or-paren or word-or-paren-or-punct,
> which is not what the docstring seems to say
> ---
>  lisp/indent.el | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/lisp/indent.el b/lisp/indent.el
> index 89de0a1d7d1..de8101dc76e 100644
> --- a/lisp/indent.el
> +++ b/lisp/indent.el
> @@ -191,13 +191,17 @@ prefix argument is ignored."
>                        (eolp))
>                   (and (memq tab-first-completion
>                              '(word word-or-paren word-or-paren-or-punct))
> -                      (not (eql 2 syn)))
> +                      (not (memq 2 syn)))
>                   (and (memq tab-first-completion
>                              '(word-or-paren word-or-paren-or-punct))
> -                      (not (or (eql 4 syn)
> -                               (eql 5 syn))))
> +                      (not (or (memq 2 syn)
> +                               (memq 4 syn)
> +                               (memq 5 syn))))
>                   (and (eq tab-first-completion 'word-or-paren-or-punct)
> -                      (not (eql 1 syn)))))
> +                      (not (or (memq 2 syn)
> +                               (memq 4 syn)
> +                               (memq 5 syn)
> +                               (memq 1 syn))))))
>          (completion-at-point))
>  
>         ;; If a prefix argument was given, rigidly indent the following
> -- 
> 2.39.2

bug#67158: [PATCH] Repair tab-always-indent

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

> diff --git a/lisp/indent.el b/lisp/indent.el
> index 89de0a1d7d1..e5f2acdd33b 100644
> --- a/lisp/indent.el
> +++ b/lisp/indent.el
> @@ -171,7 +171,7 @@ prefix argument is ignored."
>      (let ((old-tick (buffer-chars-modified-tick))
>            (old-point (point))
>  	  (old-indent (current-indentation))
> -          (syn (syntax-after (point))))
> +          (syn (syntax-class (syntax-after (point)))))
>  
>        ;; Indent the line.
>        (or (not (eq (indent--funcall-widened indent-line-function) 'noindent))

Duh!  Yes, of course.  And I think this patch is better than the other
since `syntax-after` returns a cons-cell but not a list, so using `memq`
on it is weird (and in addition to that, its `car` is a funny integer
which we usually don't want to compare directly to things like 2).

> The following form will correctly return nil :
>
> (and (memq tab-first-completion
>           '(word word-or-paren word-or-paren-or-punct))
>     (not (memq 2 syn)))
>
> But this one will return non-nil :
>
> (and (memq tab-first-completion
>           '(word-or-paren word-or-paren-or-punct))
>     (not (or (eql 4 syn)
>              (eql 5 syn))))
>
> Since syn is equal to (2) (we are within a word).

Indeed.
I reworked the code based on your patch and pushed it to `master`.
Thank you, and sorry for the delay (and thanks Eli again for (re)pinging me).


        Stefan

bug#67158: [PATCH] Repair tab-always-indent

[Aymeric Agon-Rambosson]

Hi Stefan,

Le samedi 25 novembre 2023 à 10:26, Stefan Monnier 
<monnier@iro.umontreal.ca> a écrit :

> I reworked the code based on your patch and pushed it to 
> `master`.

Nice !

Two follow-up questions, if you have the time :
- In commit c20226a1ef5, you used memql to compare syn to '(2 4 
  5), and memq to compare syn to '(2 4 5 1). I'd say both are 
  equivalent in this case, so why having used each of them once 
  rather than the same twice ?
- Any chance this commit gets cherry-picked for the 29.2 bugfix 
  release ?

Thank you in advance for your time.

Best,

Aymeric

bug#67158: [PATCH] Repair tab-always-indent

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

> - In commit c20226a1ef5, you used memql to compare syn to '(2 4   5), and
>   memq to compare syn to '(2 4 5 1). I'd say both are   equivalent in this
>   case, so why having used each of them once   rather than the same twice ?

Oops, not sure how that happened.  They should both use the same (tho
either `memq` or `memql` does the trick).

> - Any chance this commit gets cherry-picked for the 29.2 bugfix release ?

I'll let Eli or Stefan make the decision.


        Stefan

bug#67158: [PATCH] Repair tab-always-indent

[Eli Zaretskii]

> Cc: 67158@debbugs.gnu.org
> Date: Sun, 26 Nov 2023 08:56:04 -0500
> From:  Stefan Monnier via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > - In commit c20226a1ef5, you used memql to compare syn to '(2 4   5), and
> >   memq to compare syn to '(2 4 5 1). I'd say both are   equivalent in this
> >   case, so why having used each of them once   rather than the same twice ?
> 
> Oops, not sure how that happened.  They should both use the same (tho
> either `memq` or `memql` does the trick).

I fixed that on master.

> > - Any chance this commit gets cherry-picked for the 29.2 bugfix release ?
> 
> I'll let Eli or Stefan make the decision.

I've cherry-picked it.