Pcase change in Emacs master (30) branch causes "Please avoid it" error

[alphapapa]

[Apologies for earlier pinging the wrong "Monnier" account on GitHub. :) Oops.]

@monnier As reported starting at this comment, the Pcase-related change in emacs.git commit 16fc5b6c0c72464a75d9a84b754375662b3acec6 has caused a breakage in Org QL: now trying to run some queries (which are parsed and optimized using Pcase expressions) signals a "Please avoid it" error.

The code in question works fine on master before that commit, and on earlier versions of Emacs.

Is this a bug in that commit, or is it now exposing a bug in downstream software?

Thanks.

(Backtrace is in an attachment because GitHub won't accept it in this form.)
backtrace.txt

[whudwl]

Could you have at-ed the wrong monnier?

[alphapapa]

Could you have at-ed the wrong monnier?

Oops, yes, thanks. I forgot that Stefan is simply @monnier.

[monnier]

I haven't investigated very far, but IIRC this error is signaled when Pcase encounters a "silly or" with a single branch.
IOW, my crystal ball suggests that you can avoid that error by changing org-ql-defpred so as to avoid wrapping (quote ,it) inside an or when predicate-names has a single element (when computing normalizers and preambles).

[akirak]

Now org-ql fails to byte-compile on Emacs master and throws the "Please avoid it" error.

Commenting out the following expression inside org-ql--define-query-preamble-fn seems to prevent the issue

org-ql/org-ql.el

Line 1144 in d5269bb

(byte-compile 'org-ql--query-preamble)))

which is evaluated during the following top-level expression:

org-ql/org-ql.el

Line 2398 in d5269bb

(org-ql--define-query-preamble-fn (reverse org-ql-predicates))

I presume this byte-compilation is intended for improving the performance, but the Emacs byte compiler has been under significant changes to make it more strict, so it's understandable that some existing code may not work. Perhaps nested byte-compilation is no longer allowed because it hinders further optimization by the byte compiler.

[akirak]

I followed the comment by @monnier

Pcase encounters a "silly or" with a single branch

and this change seems to fix the error. At least, it vanishes the "Please avoid it" error both at compile time and runtime.

I am not sure which party should handle this normalization, but we now seem to know the cause.

[alphapapa]

@akirak Thanks, I pushed a slightly different patch, which seems to be more in line with the previous, equivalent behavior (i.e. not recursing on that element).

@monnier Thank you for digging into that. May I ask, would it be reasonable for Pcase to optimize this case away instead of signaling an error about it? I think it's not uncommon to use Pcase as a sort of simple compiler/optimizer, and if an (or FOO) could just be optimized to FOO by Pcase...why not? :) (Or maybe there's a good reason that I don't know of.)

[monnier]

@monnier Thank you for digging into that. May I ask, would it be reasonable for Pcase to optimize this case away instead of signaling an error about it? I think it's not uncommon to use Pcase as a sort of simple compiler/optimizer, and if an `(or FOO)` could just be optimized to `FOO` by Pcase...why not? :) (Or maybe there's a good reason that I don't know of.)

It does sound quite reasonable, indeed. I can't remember why it signals an error, and looking at the code doesn't give me an immediate enlightenment. I suggest you `M-x report-emacs-bug`. My best guess is that it was inconvenient to handle it, and I assumed the patterns would be written by humans so I decided it was OK to tell them to avoid such silly patterns.

[alphapapa]

I think this can, and probably should be, applied in a v0.8.9 release, so reopening and assigning to that milestone.

• Move changelog entry to 0.8.9 section.