bug#24901: Fwd: Caught mistake in elec-pair.el patch

bug#24901: Fwd: Caught mistake in elec-pair.el patch

[João Távora]

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

After unarchiving this bug just to add this commnet.

---------- Forwarded message ----------
From: João Távora <joaotavora@gmail.com>
Date: Sun, Aug 13, 2017 at 11:57 AM
Subject: Caught mistake in elec-pair.el patch
To: 24901@debbugs.gnu.org, rrt@sc3d.org, Eli Zaretskii <eliz@gnu.org>


Hi Reuben,

Casually glancing at the source for lisp/elec-pair.el I have found an
error.

You cannot use lists like (nth 0 electric-quote-chars) inside a quoted list
as
you did in your patch of Nov. 8 2016. The list will not be evaluated.
Perhaps
you were distracted by the grim events taking place that day :-)

Anyway, I think you meant backquote-and-comma so I'll install this fix soon,
unless anyone objects.

João


diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index 87e82e2..4ede4f1 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -28,9 +28,9 @@
 ;;; Electric pairing.

 (defcustom electric-pair-pairs
-  '((?\" . ?\")
-    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
-    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
+  `((?\" . ?\")
+    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
+    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
   "Alist of pairs that should be used regardless of major mode.

 Pairs of delimiters in this list are a fallback in case they have
@@ -44,9 +44,9 @@ electric-pair-pairs

 ;;;###autoload
 (defcustom electric-pair-text-pairs
-  '((?\" . ?\" )
-    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
-    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
+  `((?\" . ?\" )
+    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
+    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
   "Alist of pairs that should always be used in comments and strings.

 Pairs of delimiters in this list are a fallback in case they have




-- 
João Távora

[-- Attachment #2: Type: text/html, Size: 3038 bytes --]

bug#24901: Caught mistake in elec-pair.el patch

[João Távora]

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

Turns out it's not so simple, because electric-pair-text-pairs is autoloaded
and thus relying on electric-quote-chars is a problem.

Loading loaddefs.el (source)...
Symbol's value as variable is void: electric-quote-chars
Makefile:535: recipe for target 'emacs' failed

I could:

1. Add an autoload to electric-quote-chars. (Unfortunately I seem to fail
at that,
just adding the cookie isn't enough, it has to go into ldefs-boot.el
somehow).

2. Replace those (nth...) expressions with the actual quote characters.
It's no
more disconnected from a possible customized value in electric-quote-chars
than it is now.

3. Rework elec-pair.el so that lisp expressions are accepted in
electric-pair-text-pairs
and evaluated just-in-time.

Anyway, It's been more than 1 year since my last contribution to Emacs, and
I shouldn't make this decision alone. Eli, could you please weigh in or
point me
to someone who can? Emacs-devel?

Thanks,
João


On Sun, Aug 13, 2017 at 11:59 AM, Reuben Thomas <rrt@sc3d.org> wrote:

> Thanks very much, I fear that was just ineptitude on my part.
>
> --
> https://rrt.sc3d.org
>
> On 13 Aug 2017 11:57 am, "João Távora" <joaotavora@gmail.com> wrote:
>
>> Hi Reuben,
>>
>> Casually glancing at the source for lisp/elec-pair.el I have found an
>> error.
>>
>> You cannot use lists like (nth 0 electric-quote-chars) inside a quoted
>> list as
>> you did in your patch of Nov. 8 2016. The list will not be evaluated.
>> Perhaps
>> you were distracted by the grim events taking place that day :-)
>>
>> Anyway, I think you meant backquote-and-comma so I'll install this fix
>> soon,
>> unless anyone objects.
>>
>> João
>>
>>
>> diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
>> index 87e82e2..4ede4f1 100644
>> --- a/lisp/elec-pair.el
>> +++ b/lisp/elec-pair.el
>> @@ -28,9 +28,9 @@
>>  ;;; Electric pairing.
>>
>>  (defcustom electric-pair-pairs
>> -  '((?\" . ?\")
>> -    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
>> -    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
>> +  `((?\" . ?\")
>> +    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
>> +    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
>>    "Alist of pairs that should be used regardless of major mode.
>>
>>  Pairs of delimiters in this list are a fallback in case they have
>> @@ -44,9 +44,9 @@ electric-pair-pairs
>>
>>  ;;;###autoload
>>  (defcustom electric-pair-text-pairs
>> -  '((?\" . ?\" )
>> -    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
>> -    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
>> +  `((?\" . ?\" )
>> +    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
>> +    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
>>    "Alist of pairs that should always be used in comments and strings.
>>
>>  Pairs of delimiters in this list are a fallback in case they have
>>
>>

[-- Attachment #2: Type: text/html, Size: 4554 bytes --]

bug#24901: Caught mistake in elec-pair.el patch

[npostavs]

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

João Távora <joaotavora@gmail.com> writes:

> Turns out it's not so simple, because electric-pair-text-pairs is autoloaded
> and thus relying on electric-quote-chars is a problem.
>
> Loading loaddefs.el (source)...
> Symbol's value as variable is void: electric-quote-chars
> Makefile:535: recipe for target 'emacs' failed
>
> I could:
>
> 1. Add an autoload to electric-quote-chars. (Unfortunately I seem to fail at that,
> just adding the cookie isn't enough, it has to go into ldefs-boot.el somehow).

Or remove the autoload from electric-pair-text-pairs?


[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1885 bytes --]

From 57a68d001c0851ce10b1b8be47bc8db12b7789f1 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 17 Aug 2017 07:06:47 -0400
Subject: [PATCH] * lisp/elec-pair.el (electric-pair-text-pairs): Don't
 autoload (Bug#24901).

* lisp/progmodes/elisp-mode.el (emacs-lisp-mode): Require `elec-pair'
explicitly in the interactive case.
---
 lisp/elec-pair.el            | 1 -
 lisp/progmodes/elisp-mode.el | 9 ++++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index 87e82e24fb..f990851185 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -42,7 +42,6 @@ electric-pair-pairs
   :group 'electricity
   :type '(repeat (cons character character)))
 
-;;;###autoload
 (defcustom electric-pair-text-pairs
   '((?\" . ?\" )
     ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 47739f5957..0bf8857960 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -231,9 +231,12 @@ emacs-lisp-mode
   (defvar project-vc-external-roots-function)
   (lisp-mode-variables nil nil 'elisp)
   (add-hook 'after-load-functions #'elisp--font-lock-flush-elisp-buffers)
-  (setq-local electric-pair-text-pairs
-              (append '((?\` . ?\') (?‘ . ?’)) electric-pair-text-pairs))
-  (setq-local electric-quote-string t)
+  (unless noninteractive
+    (require 'elec-pair)
+    (defvar electric-pair-text-pairs)
+    (setq-local electric-pair-text-pairs
+                (append '((?\` . ?\') (?‘ . ?’)) electric-pair-text-pairs))
+    (setq-local electric-quote-string t))
   (setq imenu-case-fold-search nil)
   (add-function :before-until (local 'eldoc-documentation-function)
                 #'elisp-eldoc-documentation-function)
-- 
2.14.1

bug#24901: Caught mistake in elec-pair.el patch

[João Távora]

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

On Thu, Aug 17, 2017 at 1:35 PM, <npostavs@users.sourceforge.net> wrote:

> Or remove the autoload from electric-pair-text-pairs?

Seems to be there for a reason, unfortunately. It's forward
referenced in electric.el if I'm not mistaken.

J


--
João Távora

[-- Attachment #2: Type: text/html, Size: 481 bytes --]

bug#24901: Caught mistake in elec-pair.el patch

[Noam Postavsky]

On Thu, Aug 17, 2017 at 9:32 AM, João Távora <joaotavora@gmail.com> wrote:
> On Thu, Aug 17, 2017 at 1:35 PM, <npostavs@users.sourceforge.net> wrote:
>
>> Or remove the autoload from electric-pair-text-pairs?
>
> Seems to be there for a reason, unfortunately. It's forward
> referenced in electric.el if I'm not mistaken.

The reference in electric.el is inside (and electric-pair-mode ...)
which is already autoloaded so it should be fine. There is another
reference in elisp-mode.el, for which, AFAICT, we can just use
explicit require instead (see my patch in
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=24901#53; sorry I didn't
mention it before, I forgot some mail clients don't show attachments
clearly).

bug#24901: Caught mistake in elec-pair.el patch

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Thu, 17 Aug 2017 14:32:08 +0100
> Cc: Reuben Thomas <rrt@sc3d.org>, 24901@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> 
> On Thu, Aug 17, 2017 at 1:35 PM, <npostavs@users.sourceforge.net> wrote:
> 
> > Or remove the autoload from electric-pair-text-pairs?
> 
> Seems to be there for a reason, unfortunately. It's forward 
> referenced in electric.el if I'm not mistaken.

I don't understand this: electric.el is preloaded, so why would it
need the autoload?

But in any case, if you do need the autoload, you can add an explicit
autoload form in electric.el.

bug#24901: Caught mistake in elec-pair.el patch

[João Távora]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Thu, 17 Aug 2017 14:32:08 +0100
>> Cc: Reuben Thomas <rrt@sc3d.org>, 24901@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
>> 
>> On Thu, Aug 17, 2017 at 1:35 PM, <npostavs@users.sourceforge.net> wrote:
>> 
>> > Or remove the autoload from electric-pair-text-pairs?
>> 
>> Seems to be there for a reason, unfortunately. It's forward 
>> referenced in electric.el if I'm not mistaken.
>
> I don't understand this: electric.el is preloaded, so why would it
> need the autoload?

I don't know, I admit I'm very confused by the autoload logic, so you
lost me at preloaded.  Also have been away from Elisp for almost a year.

If I remove the autoload I get compilation errors in electric.elc or
align.elc (from make bootstrap). I might be doing something wrong. How
can I test the autoload dependecies without a full make bootstrap?

And then there's the problem Noam mentioned in elisp-mode.el.

But Noam's patch apparently fixes that, so here's his patch and mine for
a quick review from either of you if you don't mind. Tested with 'make
bootstrap'.

Thanks,
João


[-- Attachment #2: 0001-lisp-elec-pair.el-electric-pair-text-pairs-Don-t-aut.patch --]
[-- Type: text/x-diff, Size: 1889 bytes --]

From 3e5a166cb7e5b575252c5c69c2678d2074d58c12 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 17 Aug 2017 07:06:47 -0400
Subject: [PATCH 1/2] * lisp/elec-pair.el (electric-pair-text-pairs): Don't
 autoload (Bug#24901).

* lisp/progmodes/elisp-mode.el (emacs-lisp-mode): Require `elec-pair'
explicitly in the interactive case.
---
 lisp/elec-pair.el            | 1 -
 lisp/progmodes/elisp-mode.el | 9 ++++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index 87e82e24fb..f990851185 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -42,7 +42,6 @@ electric-pair-pairs
   :group 'electricity
   :type '(repeat (cons character character)))
 
-;;;###autoload
 (defcustom electric-pair-text-pairs
   '((?\" . ?\" )
     ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index 47739f5957..0bf8857960 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -231,9 +231,12 @@ emacs-lisp-mode
   (defvar project-vc-external-roots-function)
   (lisp-mode-variables nil nil 'elisp)
   (add-hook 'after-load-functions #'elisp--font-lock-flush-elisp-buffers)
-  (setq-local electric-pair-text-pairs
-              (append '((?\` . ?\') (?‘ . ?’)) electric-pair-text-pairs))
-  (setq-local electric-quote-string t)
+  (unless noninteractive
+    (require 'elec-pair)
+    (defvar electric-pair-text-pairs)
+    (setq-local electric-pair-text-pairs
+                (append '((?\` . ?\') (?‘ . ?’)) electric-pair-text-pairs))
+    (setq-local electric-quote-string t))
   (setq imenu-case-fold-search nil)
   (add-function :before-until (local 'eldoc-documentation-function)
                 #'elisp-eldoc-documentation-function)
-- 
2.11.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Fix-default-value-of-electric-pair-pairs-and-electri.patch --]
[-- Type: text/x-diff, Size: 2023 bytes --]

From a96f9ed05edfe619f7bc4edf5485c937e3df4d1b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Thu, 17 Aug 2017 10:44:38 +0100
Subject: [PATCH 2/2] Fix default value of electric-pair-pairs and
 electric-pair-text-pairs

Fixes: debbugs:24901

A previous change, titled "Add support for curly quotation marks to
electric-pair-mode", attempted to add these characters to the default
value of these variables.  But it did so in a quoted list, preventing
evaluation of the relevant expressions and resulting in an invalid
format.

* lisp/elec-pair.el (electric-pair-pairs): Use backquote and comma.
(electric-pair-text-pairs): Use backquote and comma.
---
 lisp/elec-pair.el | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el
index f990851185..236f5f1283 100644
--- a/lisp/elec-pair.el
+++ b/lisp/elec-pair.el
@@ -28,9 +28,9 @@
 ;;; Electric pairing.
 
 (defcustom electric-pair-pairs
-  '((?\" . ?\")
-    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
-    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
+  `((?\" . ?\")
+    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
+    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
   "Alist of pairs that should be used regardless of major mode.
 
 Pairs of delimiters in this list are a fallback in case they have
@@ -43,9 +43,9 @@ electric-pair-pairs
   :type '(repeat (cons character character)))
 
 (defcustom electric-pair-text-pairs
-  '((?\" . ?\" )
-    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
-    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
+  `((?\" . ?\" )
+    (,(nth 0 electric-quote-chars) . ,(nth 1 electric-quote-chars))
+    (,(nth 2 electric-quote-chars) . ,(nth 3 electric-quote-chars)))
   "Alist of pairs that should always be used in comments and strings.
 
 Pairs of delimiters in this list are a fallback in case they have
-- 
2.11.0

bug#24901: Caught mistake in elec-pair.el patch

[Noam Postavsky]

On Thu, Aug 17, 2017 at 11:36 AM, João Távora <joaotavora@gmail.com> wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Seems to be there for a reason, unfortunately. It's forward
>>> referenced in electric.el if I'm not mistaken.
>>
>> I don't understand this: electric.el is preloaded, so why would it
>> need the autoload?

I think you have the direction backwards: electric.el uses the
autoloaded variable *from* elec-pair.el.

> I don't know, I admit I'm very confused by the autoload logic, so you
> lost me at preloaded.  Also have been away from Elisp for almost a year.
>
> If I remove the autoload I get compilation errors in electric.elc or
> align.elc (from make bootstrap). I might be doing something wrong. How
> can I test the autoload dependecies without a full make bootstrap?

As far as I know, the only way to be sure of testing all dependencies
is deleting all elc files, i.e., a full bootstrap minus the C
compilation (using ccache can help automate the "minus the C
compilation" part).

> And then there's the problem Noam mentioned in elisp-mode.el.

The elisp-mode.el problem is what causes the align.elc failure: the
byte compiler uses elisp-mode.

bug#24901: Caught mistake in elec-pair.el patch

[João Távora]

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> On Thu, Aug 17, 2017 at 11:36 AM, João Távora <joaotavora@gmail.com> wrote:
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>>>> Seems to be there for a reason, unfortunately. It's forward
>>>> referenced in electric.el if I'm not mistaken.
>>>
>>> I don't understand this: electric.el is preloaded, so why would it
>>> need the autoload?
>
> I think you have the direction backwards: electric.el uses the
> autoloaded variable *from* elec-pair.el.

You mean Eli has the direction backwards. I don't have a clue :-)

> As far as I know, the only way to be sure of testing all dependencies
> is deleting all elc files, i.e., a full bootstrap minus the C
> compilation (using ccache can help automate the "minus the C
> compilation" part).

So is 'find . -iname "*.elc" | xargs rm -f' enough? What's ccache?

>> And then there's the problem Noam mentioned in elisp-mode.el.
>
> The elisp-mode.el problem is what causes the align.elc failure: the
> byte compiler uses elisp-mode.

Does this mean you sign off on both patches?

João

bug#24901: Caught mistake in elec-pair.el patch

[npostavs]

joaotavora@gmail.com (João Távora) writes:

>> As far as I know, the only way to be sure of testing all dependencies
>> is deleting all elc files, i.e., a full bootstrap minus the C
>> compilation (using ccache can help automate the "minus the C
>> compilation" part).
>
> So is 'find . -iname "*.elc" | xargs rm -f' enough?

Actually, now that I think of it, 'rm lisp/loaddefs.el' might be needed
too.

> What's ccache?

https://ccache.samba.org/

    ccache is a compiler cache. It speeds up recompilation by caching
    previous compilations and detecting when the same compilation is being
    done again. Supported languages are C, C++, Objective-C and
    Objective-C++.

So you can ./configure CC='ccache gcc' and then if you do 'make
bootstrap' the C compilation goes by very quickly after the first time.
The lisp compilation still takes a long time, but caching the C part at
least makes it less painful to use an -O2 optimized build which compiles
lisp a bit faster.

>> The elisp-mode.el problem is what causes the align.elc failure: the
>> byte compiler uses elisp-mode.
>
> Does this mean you sign off on both patches?

Yes, I think they're good (I have just a couple of format trivia
nitpicks, below).  I'm under the impression that I'm more tolerant of
explicit `require' than the average Emacser though.

> * lisp/elec-pair.el (electric-pair-pairs): Use backquote and comma.
> (electric-pair-text-pairs): Use backquote and comma.

This could be

    * lisp/elec-pair.el (electric-pair-pairs, electric-pair-text-pairs): Use
    backquote and comma.

>  (defcustom electric-pair-text-pairs
> -  '((?\" . ?\" )
> -    ((nth 0 electric-quote-chars) . (nth 1 electric-quote-chars))
> -    ((nth 2 electric-quote-chars) . (nth 3 electric-quote-chars)))
> +  `((?\" . ?\" )

Can we get rid of that space before the closing paren while we're here?

bug#24901: Caught mistake in elec-pair.el patch

[Reuben Thomas]

(Thanks, everyone, for dealing with this!)

bug#24901: Caught mistake in elec-pair.el patch

[João Távora]

npostavs@users.sourceforge.net writes:

> Yes, I think they're good (I have just a couple of format trivia
> nitpicks, below).

Thanks. Done, with nitpicks fixed.

Shall I archive the bug again or wait until someone does that
automatically?

João

bug#24901: Caught mistake in elec-pair.el patch

[Noam Postavsky]

On Fri, Aug 18, 2017 at 6:46 PM, João Távora <joaotavora@gmail.com> wrote:

> Shall I archive the bug again or wait until someone does that
> automatically?

The bug is still closed, so it will archive automatically 30 days
after the last response, no need for anything else I think.