bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Akira Komamura]

After upgrading to Emacs 29, some packages throw byte-compile errors
due to use of `map' pattern in `pcase':

> org-bookmark-heading.el:176:6: Error: Unknown map pattern: (map
> 'filename 'outline-path 'id 'front-context-string 'indirectp)

The error can be avoided by `require'ing `map' at compile time in the
library depending on pcase:

(eval-when-compile
  ;; Support map pattern in pcase
  (require 'map))

However, this was not necessary in the previous versions.

As a similar example, the `rx' pattern for `pcase' is autoloaded (see
`rx.el'), so shouldn't the `map' pattern support be autoloaded as well?

;;;###autoload
(pcase-defmacro rx (&rest regexps)
  ...

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Eli Zaretskii]

> From: Akira Komamura <akira.komamura@gmail.com>
> Date: Thu, 08 Dec 2022 16:50:25 +0900
> 
> After upgrading to Emacs 29, some packages throw byte-compile errors
> due to use of `map' pattern in `pcase':
> 
> > org-bookmark-heading.el:176:6: Error: Unknown map pattern: (map
> > 'filename 'outline-path 'id 'front-context-string 'indirectp)
> 
> The error can be avoided by `require'ing `map' at compile time in the
> library depending on pcase:
> 
> (eval-when-compile
>   ;; Support map pattern in pcase
>   (require 'map))
> 
> However, this was not necessary in the previous versions.

Could you please point out where does pcase.el uses 'map' in its
source?  The error message you quote doesn't reference pcase.el, only
org-bookmark-heading.el, which is not part of Emacs and is not on
ELPA, AFAICT.  So I wonder where's the place in the core Emacs sources
which causes the problem, and I couldn't find it.  I'm probably
missing something.

Thanks.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Visuwesh]

[வியாழன் டிசம்பர் 08, 2022] Eli Zaretskii wrote:

> Could you please point out where does pcase.el uses 'map' in its
> source?

Pcase matcher that uses the map library is in in map.el.
See the (pcase-defmacro map (&rest args) line.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Eli Zaretskii]

> From: Visuwesh <visuweshm@gmail.com>
> Cc: Akira Komamura <akira.komamura@gmail.com>,  59900@debbugs.gnu.org
> Date: Thu, 08 Dec 2022 18:33:17 +0530
> 
> [வியாழன் டிசம்பர் 08, 2022] Eli Zaretskii wrote:
> 
> > Could you please point out where does pcase.el uses 'map' in its
> > source?
> 
> Pcase matcher that uses the map library is in in map.el.
> See the (pcase-defmacro map (&rest args) line.

Thanks, but if the offending code is in map.el, then there should be
no problem for it to use functions or macros in map.el, right?

Or what am I missing?

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Visuwesh]

[வியாழன் டிசம்பர் 08, 2022] Eli Zaretskii wrote:

>> From: Visuwesh <visuweshm@gmail.com>
>> Cc: Akira Komamura <akira.komamura@gmail.com>,  59900@debbugs.gnu.org
>> Date: Thu, 08 Dec 2022 18:33:17 +0530
>> 
>> [வியாழன் டிசம்பர் 08, 2022] Eli Zaretskii wrote:
>> 
>> > Could you please point out where does pcase.el uses 'map' in its
>> > source?
>> 
>> Pcase matcher that uses the map library is in in map.el.
>> See the (pcase-defmacro map (&rest args) line.
>
> Thanks, but if the offending code is in map.el, then there should be
> no problem for it to use functions or macros in map.el, right?
>
> Or what am I missing?

I do not understand your reply but AFAIU, the error shows up because the
compiler does not know about the pcase map matching pattern during
compilation-time so I think the problem is in org-bookmark-heading
package missing a require statement for map.el and not in core.
[ Akira proposes to autoload the map.el matching pattern to avoid future
  errors as rx's pattern is already autoloaded.  ]

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Akira Komamura]

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

> Could you please point out where does pcase.el uses 'map' in its
> source?  The error message you quote doesn't reference pcase.el, only
> org-bookmark-heading.el, which is not part of Emacs and is not on
> ELPA, AFAICT.


Apology for the misleading title.

`pcase.el' itself doesn't depend on `map.el', nor does it contain `map'
pattern
in its source. `pcase.el' itself successfully compiles.

I encountered the error while trying to byte-compile
`org-bookmark-heading.el',
which contains a `map' pattern at the following location:

https://github.com/alphapapa/org-bookmark-heading/blob/f245c9023df28d6ee545dae4b96a1c237e6965ba/org-bookmark-heading.el#L176

The error vanishes if I add `(eval-when-compile (require 'map))' as in the
following PR:

https://github.com/alphapapa/org-bookmark-heading/pull/19/files

I encountered similar errors in other libraries that contain a `map' pattern
inside a `pcase' form. It didn't happen until recently, when I updated
Emacs and
began to get those errors.


> So I wonder where's the place in the core Emacs sources
> which causes the problem, and I couldn't find it.  I'm probably
> missing something.


 There are some other additional patterns for `pcase'. One of them is `rx'
pattern. There is an autoload cookie right above the definition of
`(pcase-defmacro rx ...' in `rx.el'. On the other hand, there is no autoload
above `(pcase-defmacro map ...' in `map.el'. I think this might be the
cause.
Is there any reason for not autoloading the `pcase-defmacro` form?

Thanks.

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

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Michael Heerdegen]

Akira Komamura <akira.komamura@gmail.com> writes:

> I encountered similar errors in other libraries that contain a `map' pattern
> inside a `pcase' form. It didn't happen until recently, when I updated Emacs
> and
> began to get those errors.
>
>  So I wonder where's the place in the core Emacs sources
>  which causes the problem, and I couldn't find it.  I'm probably
>  missing something.

Maybe it started with

| 85555ad3b79 ; Require map only during compilation
| Philip Kaludercic <philipk@posteo.net> 2022-10-23

Anyway, AFAIU currently compiling pcase forms with `map' patterns
requires map.el to be currently loaded, and since you can't (and could
not) rely on that, it's better to add
(eval-when-compile (require 'map)).

> There are some other additional patterns for `pcase'. One of them is `rx'
> pattern. There is an autoload cookie right above the definition of
> `(pcase-defmacro rx ...' in `rx.el'. On the other hand, there is no autoload
> above `(pcase-defmacro map ...' in `map.el'. I think this might be the cause.
> Is there any reason for not autoloading the `pcase-defmacro` form?

I see no reason, it should be doable.

Michael.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  59900@debbugs.gnu.org
> Date: Fri, 09 Dec 2022 03:11:14 +0100
> 
> Akira Komamura <akira.komamura@gmail.com> writes:
> 
> > I encountered similar errors in other libraries that contain a `map' pattern
> > inside a `pcase' form. It didn't happen until recently, when I updated Emacs
> > and
> > began to get those errors.
> >
> >  So I wonder where's the place in the core Emacs sources
> >  which causes the problem, and I couldn't find it.  I'm probably
> >  missing something.
> 
> Maybe it started with
> 
> | 85555ad3b79 ; Require map only during compilation
> | Philip Kaludercic <philipk@posteo.net> 2022-10-23
> 
> Anyway, AFAIU currently compiling pcase forms with `map' patterns
> requires map.el to be currently loaded, and since you can't (and could
> not) rely on that, it's better to add
> (eval-when-compile (require 'map)).

Thanks, but all this still doesn't answer my questions,
unfortunately.  You-all are describing something that I cannot wrap my
head around, because that basic question was not answered yet.

Would someone please take me through the problem step by step?  ELI5,
OK?

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Akira Komamura]

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

On Fri, Dec 9, 2022 at 4:12 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Thanks, but all this still doesn't answer my questions,
> unfortunately.  You-all are describing something that I cannot wrap my
> head around, because that basic question was not answered yet.
>
> Would someone please take me through the problem step by step?
>

There is a MELPA package depending on `pcase'. It contains a `map' pattern:

(pcase-let* ((`(,_name . ,(map filename outline-path id
front-context-string indirectp))
  ...)

The support for `map' pattern is not built into `pcase.el'. Instead, it is
enabled through `pcase--get-macroexpander', which is implemented as follows:

(defun pcase--get-macroexpander (s)
  "Return the macroexpander for pcase pattern head S, or nil."
  (get s 'pcase-macroexpander))

It returns `pcase-macroexpander' property of the symbol (i.e. `map' in this
case). The property should be available at compile time. If not, the library
will fail to byte-compile.

To set the property of an additional pattern, `rx', `map', `seq', etc. all
use
`pcase-defmacro'. I think autoloading the `pcase-defmacro' form in `map.el'
can
prevent the error as well as possible future errors in other libraries.
This is
already done in `rx.el'.

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

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but all this still doesn't answer my questions,
> unfortunately.  You-all are describing something that I cannot wrap my
> head around, because that basic question was not answered yet.

What specific question?

pcase is extensible: It allows to define new pattern types.  "rx.el"
defines a `rx' pcase pattern, "map.el" a `map' pcase pattern.  The
normal way to be able to use them is to `require' the library in which
they are defined.

Since `pcase' is a macro, the library (rx.el or map.el) must be loaded
when a `pcase' form using such a pattern is compiled.

The `rx' pattern definition in rx.el uses autoload cookies so that
compiling works even when rx.el is not loaded - it gets loaded
automatically when compiling.

The map.el `map' pcase pattern doesn't use autoloading.  The OP found a
case where compiling succeeded until some days ago without requiring
map.el explictily, and now a require is needed.  But that was only by
luck: AFAIU, map.el was already loaded in that scenario, and now it is
any more.  But there is no guarantee that the library is always loaded
when compiling arbitrary files, so one should _always_ explicitly
require map.el when the file contains pcase forms with `map' patterns -
even if this worked without in some cases in the past by luck.

The wish of the OP to make the `map' pattern in map.el `autoload'able
like the `rx' pattern in rx.el is reasonable, I can try to create a
patch.

But strictly speaking here is no bug, just the OP relying on something
that in the past worked by luck.

Hope this answers everything - else please ask specific questions.


Michael.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: akira.komamura@gmail.com,  59900@debbugs.gnu.org
> Date: Fri, 09 Dec 2022 19:02:32 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but all this still doesn't answer my questions,
> > unfortunately.  You-all are describing something that I cannot wrap my
> > head around, because that basic question was not answered yet.
> 
> What specific question?

The one I asked at the very beginning of this discussion:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59900#8

> pcase is extensible: It allows to define new pattern types.  "rx.el"
> defines a `rx' pcase pattern, "map.el" a `map' pcase pattern.  The
> normal way to be able to use them is to `require' the library in which
> they are defined.
> 
> Since `pcase' is a macro, the library (rx.el or map.el) must be loaded
> when a `pcase' form using such a pattern is compiled.
> 
> The `rx' pattern definition in rx.el uses autoload cookies so that
> compiling works even when rx.el is not loaded - it gets loaded
> automatically when compiling.
> 
> The map.el `map' pcase pattern doesn't use autoloading.  The OP found a
> case where compiling succeeded until some days ago without requiring
> map.el explictily, and now a require is needed.  But that was only by
> luck: AFAIU, map.el was already loaded in that scenario, and now it is
> any more.  But there is no guarantee that the library is always loaded
> when compiling arbitrary files, so one should _always_ explicitly
> require map.el when the file contains pcase forms with `map' patterns -
> even if this worked without in some cases in the past by luck.
> 
> The wish of the OP to make the `map' pattern in map.el `autoload'able
> like the `rx' pattern in rx.el is reasonable, I can try to create a
> patch.
> 
> But strictly speaking here is no bug, just the OP relying on something
> that in the past worked by luck.

It sounds like the bug is in the package which uses map.el, and that
package is not part of Emacs.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > What specific question?

> The one I asked at the very beginning of this discussion:
>
>  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59900#8

The same compiler error happens in emacs -Q if you compile a pcase
form that uses the map pattern and miss to require the map library,
e.g. compiling

#+begin_src emacs-lisp
(defun test (plist)
  (pcase-let (((map :one (:two two)) plist))
    (list one two)))
#+end_src

gives you

| Error: Unknown map pattern: (map :one (:two two))

but it's expected.


> It sounds like the bug is in the package which uses map.el, and that
> package is not part of Emacs.

Exactly.

But I still want to create a patch to support autoloading of the `map'
pattern in "map.el", this is a good idea.

Michael.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> But I still want to create a patch to support autoloading of the `map'
> pattern in "map.el", this is a good idea.

I'm not so sure after thinking more about it: _Nothing_ in "map.el" has
an autoload cookie.  I'm not convinced that out of all things `map'
pcase patterns should be the only thing that are autoloaded whereby
everything else in map.el requires an explicit `require'.  That would be
hardly consistent

I don't know why the library has no autoload cookies at all but I guess
there is a reason for it.

Michael.

bug#59900: 30.0.50; map pattern in pcase causes a byte-compile error

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: akira.komamura@gmail.com,  59900@debbugs.gnu.org
> Date: Sat, 10 Dec 2022 02:01:35 +0100
> 
> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> > But I still want to create a patch to support autoloading of the `map'
> > pattern in "map.el", this is a good idea.
> 
> I'm not so sure after thinking more about it: _Nothing_ in "map.el" has
> an autoload cookie.  I'm not convinced that out of all things `map'
> pcase patterns should be the only thing that are autoloaded whereby
> everything else in map.el requires an explicit `require'.  That would be
> hardly consistent

Thanks.  I agree with that conclusion, and so I'm closing this bug
report.  The fix should be in the programs that use pcase-map, not in
core.