bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Michał Kondraciuk]

1) Start emacs with `emacs --no-init-file`

2) Type into buffer named `*test*`:

    1234   23347   35466
       155555      267       31
    1444444 2444 32777


3) Type into `*scratch*` buffer:

   (let ((rule
          (list (list nil (cons 'regexp "\\(\\s-*[0-9]+\\)")
                      (cons 'group 1)
                      (cons 'justify t)
                      (cons 'spacing 1)
                      (cons 'repeat t)))))
     (with-current-buffer "*test*"
       (align-region (point-min) (point-max) 'entire rule nil
                     (lambda (&rest r) t))))

4) Evaluate scratch buffer


Expected behavior: Form above evaluates to nil.

Actual behavior - error is caught:
   Debugger entered--Lisp error: (wrong-type-argument markerp (#<marker 
at 53 in *test*> . #<marker (moves after insertion) at 60 in *test*>))
     marker-position((#<marker at 53 in *test*> . #<marker (moves after 
insertion) at 60 in *test*>))
     align-areas(((#<marker (moves after insertion) at 51 in *test*> 
#<marker at 53 in *test*> . #<marker (moves after insertion) at 60 in 
*test*>) (#<marker (moves after insertion) at 22 in *test*> #<marker at 
27 in *test*> . #<marker (moves after insertion) at 33 in *test*>) 
(#<marker (moves after insertion) at 1 in *test*> #<marker at 2 in 
*test*> . #<marker (moves after insertion) at 6 in *test*>)) (1 . t) 
(nil (regexp . "\\(\\s-*[0-9]+\\)") (group . 1) (justify . t) (spacing . 
1) (repeat . t)) (lambda (&rest r) t))
     align-region(1 71 entire ((nil (regexp . "\\(\\s-*[0-9]+\\)") 
(group . 1) (justify . t) (spacing . 1) (repeat . t))) nil (lambda 
(&rest r) t))
     (save-current-buffer (set-buffer "*test*") (align-region 
(point-min) (point-max) (quote entire) rule nil (function (lambda (&rest 
r) t))))
     (let ((rule (list (list nil (cons (quote regexp) 
"\\(\\s-*[0-9]+\\)") (cons (quote group) 1) (cons (quote justify) t) 
(cons (quote spacing) 1) (cons (quote repeat) t))))) 
(save-current-buffer (set-buffer "*test*") (align-region (point-min) 
(point-max) (quote entire) rule nil (function (lambda (&rest r) t)))))
     eval((let ((rule (list (list nil (cons (quote regexp) 
"\\(\\s-*[0-9]+\\)") (cons (quote group) 1) (cons (quote justify) t) 
(cons (quote spacing) 1) (cons (quote repeat) t))))) 
(save-current-buffer (set-buffer "*test*") (align-region (point-min) 
(point-max) (quote entire) rule nil (function (lambda (&rest r) t))))) nil)
     elisp--eval-last-sexp(nil)
     eval-last-sexp(nil)
     funcall-interactively(eval-last-sexp nil)
     call-interactively(eval-last-sexp nil nil)
     command-execute(eval-last-sexp)

This error doesn't happen if I don't use 'justify rule.


In GNU Emacs 25.3.2 (x86_64-pc-linux-gnu, GTK+ Version 3.18.9)
  of 2017-09-12 built on lcy01-32
Windowing system distributor 'The X.Org Foundation', version 11.0.11804000
System Description:    Linux Mint 18.2 Sonya

Configured using:
  'configure --build=x86_64-linux-gnu --prefix=/usr
  '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
  '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
  --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
  '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
  --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
  --program-suffix=25 --with-modules --with-x=yes --with-x-toolkit=gtk3
  'CFLAGS=-g -O2 -fstack-protector-strong -Wformat
  -Werror=format-security' 'CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2'
  'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
   value of $LC_CTYPE: en_US.UTF-8
   value of $LC_MONETARY: pl_PL.UTF-8
   value of $LC_NUMERIC: pl_PL.UTF-8
   value of $LANG: en_US.UTF-8
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-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
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t
   line-number-mode: t
   transient-mark-mode: t

Recent messages:
Checking 60 files in /usr/share/emacs/25.3/lisp/obsolete...
Checking for load-path shadows...done
Mark set [3 times]
Quit
Undo! [4 times]
C-x 4 k is undefined
C-x n C-g is undefined
Quit
Type "q" in help window to restore its previous buffer, C-M-v to scroll 
help.
switch-to-prev-buffer is not on any key
switch-to-next-buffer is not on any key

Load-path shadows:
/usr/share/emacs/site-lisp/dictionaries-common/flyspell hides 
/usr/share/emacs/25.3/lisp/textmodes/flyspell
/usr/share/emacs/site-lisp/dictionaries-common/ispell hides 
/usr/share/emacs/25.3/lisp/textmodes/ispell

Features:
(thingatpt kmacro two-column iso-transl pp shadow sort mail-extr
emacsbug message dired format-spec rfc822 mml mml-sec password-cache epg
epg-config gnus-util mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util help-fns mail-prsvr mail-utils help-mode easymenu cl-loaddefs
pcase cl-lib debug align time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cl-generic 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 charscript case-table epa-hook jka-cmpr-hook help
simple abbrev minibuffer cl-preloaded nadvice loaddefs button faces
cus-face macroexp files text-properties overlay sha1 md5 base64 format
env code-pages mule custom widget hashtable-print-readable backquote
dbusbind inotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 102095 11612)
  (symbols 48 20091 1)
  (miscs 40 140 304)
  (strings 32 15967 3594)
  (string-bytes 1 461217)
  (vectors 16 13069)
  (vector-slots 8 442283 10883)
  (floats 8 180 200)
  (intervals 56 1866 2)
  (buffers 976 24)
  (heap 1024 37952 2989))

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Michał Kondraciuk]

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

I looked at source of `align-areas` in lisp/align.el and it looks like 
the changes made to `areas` on line 1196 are ignored in funcall on line 
1218.
Attached patch contains my proposed change.

[-- Attachment #2: align.patch --]
[-- Type: text/x-patch, Size: 851 bytes --]

diff --git a/lisp/align.el b/lisp/align.el
index 941fa3a..10f3fcb 100644
--- a/lisp/align.el
+++ b/lisp/align.el
@@ -1215,10 +1215,16 @@ align-areas
 	      (gocol col) cur)
 	  (when area
 	    (if func
-		(funcall func
-			 (marker-position (car area))
-			 (marker-position (cdr area))
-			 change)
+                (if (and justify
+                         (consp (cdr area)))
+                    (funcall func
+                             (marker-position (car area))
+                             (marker-position (cadr area))
+                             change)
+                  (funcall func
+                             (marker-position (car area))
+                             (marker-position (cdr area))
+                             change))
 	      (if (not (and justify
 			    (consp (cdr area))))
 		  (goto-char (cdr area))

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Michał Kondraciuk]

Hello,

is there any news on the status of this bug? Should I just send the 
patch to emacs-devel?

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Eli Zaretskii]

> From: Michał Kondraciuk <k.michal@zoho.com>
> Date: Tue, 20 Feb 2018 22:07:10 +0100
> 
> is there any news on the status of this bug? Should I just send the 
> patch to emacs-devel?

Sorry for the delay, I will look at the patch soon.

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Eli Zaretskii]

> Date: Wed, 21 Feb 2018 20:30:00 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 30139@debbugs.gnu.org
> 
> > From: Michał Kondraciuk <k.michal@zoho.com>
> > Date: Tue, 20 Feb 2018 22:07:10 +0100
> > 
> > is there any news on the status of this bug? Should I just send the 
> > patch to emacs-devel?
> 
> Sorry for the delay, I will look at the patch soon.

John, could you please have a look at this?

Btw, I find the documentation in align.el hard to understand.  It
sounds like a very powerful feature, but I would have difficulties
using it, with the doc strings as my only guidance.  Some examples of
unclear or confusing documentation:

 . align-region-separate has this to describe the 'group' method:

     Each contiguous set of lines where a specific alignment occurs is
     considered a section for that alignment rule.

   What is a "contiguous set of lines"?  Does it mean non-empty lines,
   i.e. that groups are separated by empty lines (that's what the
   example seems to imply)?

 . align-rules-list has this to describe the 'group' attribute:

     [...]  For alignment rules, these are the characters that will be
     deleted/expanded for the purposes of alignment.  The "alignment
     character" is always the first character immediately following
     this parenthesis group.  This attribute may also be a list of
     integers, in which case multiple alignment characters will be
     aligned, with the list of integers identifying the whitespace
     groups which precede them.  The default for this attribute is 1.

   Which "this parenthesis group" is being alluded to here?  Also, it
   leaves unexplained how characters are specified by integers, and
   the meaning of the default value of 1 is thus unclear.

So patches to clarify the documentation in align.el are most welcome.

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Eli Zaretskii]

Ping!

> Date: Sat, 24 Feb 2018 12:21:04 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 30139@debbugs.gnu.org, k.michal@zoho.com
> 
> > Date: Wed, 21 Feb 2018 20:30:00 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 30139@debbugs.gnu.org
> > 
> > > From: Michał Kondraciuk <k.michal@zoho.com>
> > > Date: Tue, 20 Feb 2018 22:07:10 +0100
> > > 
> > > is there any news on the status of this bug? Should I just send the 
> > > patch to emacs-devel?
> > 
> > Sorry for the delay, I will look at the patch soon.
> 
> John, could you please have a look at this?
> 
> Btw, I find the documentation in align.el hard to understand.  It
> sounds like a very powerful feature, but I would have difficulties
> using it, with the doc strings as my only guidance.  Some examples of
> unclear or confusing documentation:
> 
>  . align-region-separate has this to describe the 'group' method:
> 
>      Each contiguous set of lines where a specific alignment occurs is
>      considered a section for that alignment rule.
> 
>    What is a "contiguous set of lines"?  Does it mean non-empty lines,
>    i.e. that groups are separated by empty lines (that's what the
>    example seems to imply)?
> 
>  . align-rules-list has this to describe the 'group' attribute:
> 
>      [...]  For alignment rules, these are the characters that will be
>      deleted/expanded for the purposes of alignment.  The "alignment
>      character" is always the first character immediately following
>      this parenthesis group.  This attribute may also be a list of
>      integers, in which case multiple alignment characters will be
>      aligned, with the list of integers identifying the whitespace
>      groups which precede them.  The default for this attribute is 1.
> 
>    Which "this parenthesis group" is being alluded to here?  Also, it
>    leaves unexplained how characters are specified by integers, and
>    the meaning of the default value of 1 is thus unclear.
> 
> So patches to clarify the documentation in align.el are most welcome.
> 
> 
> 
> 

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Eli Zaretskii]

Ping!  Ping!

> Date: Sat, 03 Mar 2018 13:11:33 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 30139@debbugs.gnu.org, k.michal@zoho.com
> 
> Ping!
> 
> > Date: Sat, 24 Feb 2018 12:21:04 +0200
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: 30139@debbugs.gnu.org, k.michal@zoho.com
> > 
> > > Date: Wed, 21 Feb 2018 20:30:00 +0200
> > > From: Eli Zaretskii <eliz@gnu.org>
> > > Cc: 30139@debbugs.gnu.org
> > > 
> > > > From: Michał Kondraciuk <k.michal@zoho.com>
> > > > Date: Tue, 20 Feb 2018 22:07:10 +0100
> > > > 
> > > > is there any news on the status of this bug? Should I just send the 
> > > > patch to emacs-devel?
> > > 
> > > Sorry for the delay, I will look at the patch soon.
> > 
> > John, could you please have a look at this?
> > 
> > Btw, I find the documentation in align.el hard to understand.  It
> > sounds like a very powerful feature, but I would have difficulties
> > using it, with the doc strings as my only guidance.  Some examples of
> > unclear or confusing documentation:
> > 
> >  . align-region-separate has this to describe the 'group' method:
> > 
> >      Each contiguous set of lines where a specific alignment occurs is
> >      considered a section for that alignment rule.
> > 
> >    What is a "contiguous set of lines"?  Does it mean non-empty lines,
> >    i.e. that groups are separated by empty lines (that's what the
> >    example seems to imply)?
> > 
> >  . align-rules-list has this to describe the 'group' attribute:
> > 
> >      [...]  For alignment rules, these are the characters that will be
> >      deleted/expanded for the purposes of alignment.  The "alignment
> >      character" is always the first character immediately following
> >      this parenthesis group.  This attribute may also be a list of
> >      integers, in which case multiple alignment characters will be
> >      aligned, with the list of integers identifying the whitespace
> >      groups which precede them.  The default for this attribute is 1.
> > 
> >    Which "this parenthesis group" is being alluded to here?  Also, it
> >    leaves unexplained how characters are specified by integers, and
> >    the meaning of the default value of 1 is thus unclear.
> > 
> > So patches to clarify the documentation in align.el are most welcome.
> > 
> > 
> > 
> > 
> 
> 
> 
> 

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[John Wiegley]

>>>>> "MK" == Michał Kondraciuk <k.michal@zoho.com> writes:

MK> is there any news on the status of this bug? Should I just send the patch to
MK> emacs-devel?

Hi Michał,

I would suggest the following code:

    (funcall func
             (marker-position (car area))
             (marker-position (if (and justify
                                       (consp (cdr area)))
                                  (cadr area)
                                (cdr area)))
             change)

I can't really tell you at this point if this is the correct change, but if it
solves the bug, it surely is an improvement.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[John Wiegley]

>>>>> "EZ" == Eli Zaretskii <eliz@gnu.org> writes:

> Btw, I find the documentation in align.el hard to understand. It sounds like
> a very powerful feature, but I would have difficulties using it, with the
> doc strings as my only guidance. Some examples of unclear or confusing
> documentation:

>  . align-region-separate has this to describe the 'group' method:

>      Each contiguous set of lines where a specific alignment occurs is
>      considered a section for that alignment rule.

>    What is a "contiguous set of lines"?  Does it mean non-empty lines,
>    i.e. that groups are separated by empty lines (that's what the
>    example seems to imply)?

It refers to this:

group one
group one
group one

group two
group two

That is, groups of lines separated by at least one blank line.

>  . align-rules-list has this to describe the 'group' attribute:

>      [...]  For alignment rules, these are the characters that will be
>      deleted/expanded for the purposes of alignment.  The "alignment
>      character" is always the first character immediately following
>      this parenthesis group.  This attribute may also be a list of
>      integers, in which case multiple alignment characters will be
>      aligned, with the list of integers identifying the whitespace
>      groups which precede them.  The default for this attribute is 1.

This refers to regexp groups. So if the align regexp were:

    \( +\)=

Then regexp group 1 is the text that align should feel free to delete, or fill
with spaces, in order to line up all the equal signs.

>    Which "this parenthesis group" is being alluded to here?  Also, it
>    leaves unexplained how characters are specified by integers, and
>    the meaning of the default value of 1 is thus unclear.

It just means that the default is to use the first regexp quote in the
matching string.

> So patches to clarify the documentation in align.el are most welcome.

Not sure when I'll have time to do this, but I'll add it to my list.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Eli Zaretskii]

Ping!  What is the road ahead towards solving this issue?

> From: "John Wiegley" <johnw@gnu.org>
> Date: Mon, 19 Mar 2018 17:39:43 -0700
> Cc: 30139@debbugs.gnu.org
> 
> >>>>> "MK" == Michał Kondraciuk <k.michal@zoho.com> writes:
> 
> MK> is there any news on the status of this bug? Should I just send the patch to
> MK> emacs-devel?
> 
> Hi Michał,
> 
> I would suggest the following code:
> 
>     (funcall func
>              (marker-position (car area))
>              (marker-position (if (and justify
>                                        (consp (cdr area)))
>                                   (cadr area)
>                                 (cdr area)))
>              change)
> 
> I can't really tell you at this point if this is the correct change, but if it
> solves the bug, it surely is an improvement.
> 
> -- 
> John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
> http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2
> 
> 
> 
> 

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[John Wiegley]

>>>>> Eli Zaretskii <eliz@gnu.org> writes:

> Ping!  What is the road ahead towards solving this issue?

I'm fine with the amended version of the function as it stands now. Unless
I've misunderstood, it solves the bug, yes?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Lars Ingebrigtsen]

"John Wiegley" <johnw@gnu.org> writes:

>>>>>> "MK" == MichaB Kondraciuk <k.michal@zoho.com> writes:
>
> MK> is there any news on the status of this bug? Should I just send the patch to
> MK> emacs-devel?
>
> Hi MichaB,
>
> I would suggest the following code:
>
>     (funcall func
>              (marker-position (car area))
>              (marker-position (if (and justify
>                                        (consp (cdr area)))
>                                   (cadr area)
>                                 (cdr area)))
>              change)
>
> I can't really tell you at this point if this is the correct change, but if it
> solves the bug, it surely is an improvement.

I am guessing that you meant the following change:

diff --git a/lisp/align.el b/lisp/align.el
index 443237b451..cd72d52df4 100644
--- a/lisp/align.el
+++ b/lisp/align.el
@@ -1216,9 +1216,12 @@ align-areas
 	  (when area
 	    (if func
 		(funcall func
-			 (marker-position (car area))
-			 (marker-position (cdr area))
-			 change)
+                         (marker-position (car area))
+                         (marker-position (if (and justify
+                                                   (consp (cdr area)))
+                                              (cadr area)
+                                            (cdr area)))
+                         change)
 	      (if (not (and justify
 			    (consp (cdr area))))
 		  (goto-char (cdr area))

And with it in place, I don't get any errors.

But nothing happens in the *test* buffer at all, so that's a bit
underwhelming...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#30139: 25.3; Passing callback to align-region raises an error when using `justify` rule

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> And with it in place, I don't get any errors.
>
> But nothing happens in the *test* buffer at all, so that's a bit
> underwhelming...

I missed this from the doc string:

If FUNC is specified, no text will be modified. 

So I guess this is the correct change, and I'm applying it to the trunk.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no