Fix: prevent preamble evaluation breaking completion framework #285

chasecaleb · 2022-06-23T21:46:32Z

I encountered an issue using the heading predicate with org-ql-find interactively. Typing h: throws an error which isn't caught, so in my case as a vertico user it prevents any more completion updates from displaying (i.e. typing into the completion minibuffer no longer updates the completions results list). I think this is an issue with org-ql, not vertico, because org-ql should either prevent the error or catch it.

Reproduction

M-x org-ql-find
Type h:
Trigger completion (automatic in my case with vertico, TAB for default Emacs completion)

Result: backtrace that ends with

  error("Unknown rx symbol `%s'" nil)
  rx--translate-symbol(nil)
  rx--translate(nil)
  rx--translate-seq((bol (1+ "*") (1+ blank) (0+ nonl) nil))
  rx--translate-form((seq bol (1+ "*") (1+ blank) (0+ nonl) nil))
  rx--translate((seq bol (1+ "*") (1+ blank) (0+ nonl) nil))
  rx-to-string((seq bol (1+ "*") (1+ blank) (0+ nonl) nil) no-group)
  #f(compiled-function (strings) #<bytecode -0x5838b1a9927114e>)(nil)
  #f(compiled-function (element) #<bytecode -0x1eae36f5c2ca9854>)((heading))
  mapcar(#f(compiled-function (element) #<bytecode -0x1eae36f5c2ca9854>) ((heading)))
  org-ql--query-preamble((heading))

Alternative reproduction method: run the following elisp snippet and observe the backtrace.

(let* ((query '(heading))
       (normalized (org-ql--normalize-query query))
       (preamble (org-ql--query-preamble normalized)))
  (list :query query
        :normalized normalized
        :preamble preamble))

Fix

I made two different commits with different solutions:

83cc00a fixes the heading preamble specifically.
62f4e0b adds error handling to prevent any preamble errors from propagating up.

You could use both commits or just one of them depending on what solution seems preferable to you. Let me know if you would prefer a different solution too.

Use empty string instead of nil as fallback for "h:".

alphapapa · 2022-06-25T01:07:59Z

Hi Caleb,

Thanks for your thorough investigation. The problem is that the predicate has no argument when the query is given for preamble expansion.

This isn't unique to the heading predicate, of course, so what's probably needed is to check in org-ql-find for any predicates that have no argument, and avoid executing the query if that is so. But, of course, some predicates may require no argument (e.g. the todo: predicate), so those should be excepted from the check.

So what's probably really needed is for the org-ql-defpred macro to set an additional property for each predicate specifying whether it requires arguments, and then org-ql-find could determine whether any given predicates require arguments, and avoid running the query if any that do require them have none. Ideally there would be a predicate function, like org-ql--query-arguments-satisfied-p, that would take a sexp query as its argument and return non-nil if all of the predicates in the query that require arguments have some (it wouldn't need to validate them any further than that they exist).

That's a bit more involved, but it's probably The Right Way(TM) to fix it.

By the way, I'm considering publishing org-ql on GNU ELPA, in which case major code contributions would require FSF copyright assignment, like Emacs itself. So before you write any more code, you should keep that in mind.

Thanks.

alphapapa · 2022-06-25T13:44:10Z

By the way, while I see the same error you see when I just type h: as the query in org-ql-find, and I also use Vertico, it does not seem to prevent Vertico from working and presenting query results, e.g. if I continue typing an argument like h:emacs.

chasecaleb · 2022-06-28T20:52:52Z

That's a bit more involved, but it's probably The Right Way(TM) to fix it.

Yeah, that sounds like a reasonable solution to me.

By the way, I'm considering publishing org-ql on GNU ELPA, in which case major code contributions would require FSF copyright assignment, like Emacs itself. So before you write any more code, you should keep that in mind.

Thanks for the heads up. Unfortunately I'll have to pass on implementing your Right Way solution then, because I don't want to go through the rigamarole involved. I think you should still be able to merge whichever parts of this PR that you're interested in, if any, since it's short enough to meet FSF's trivial change exception.

If you get a chance you might want to call this out in your readme or in a CONTRIBUTING.org file too.

By the way, while I see the same error you see when I just type h: as the query in org-ql-find, and I also use Vertico, it does not seem to prevent Vertico from working and presenting query results, e.g. if I continue typing an argument like h:emacs.

Now that's interesting... Do you know what commit or version of Vertico you're using? I'd like to try with the same one t 8000 o see what's going on. I'm on minad/vertico@afd1e1b, which is in between tags 0.24 and 0.25 (the main branch is currently equivalent to 0.25, for context).

alphapapa · 2022-06-30T09:04:22Z

That's a bit more involved, but it's probably The Right Way(TM) to fix it.

Yeah, that sounds like a reasonable solution to me.

I thought about it some more a few days ago (when I wasn't in front of a keyboard), and IIRC even that wasn't the best solution. So we'll see.

By the way, I'm considering publishing org-ql on GNU ELPA, in which case major code contributions would require FSF copyright assignment, like Emacs itself. So before you write any more code, you should keep that in mind.

Thanks for the heads up. Unfortunately I'll have to pass on implementing your Right Way solution then, because I don't want to go through the rigamarole involved. I think you should still be able to merge whichever parts of this PR that you're interested in, if any, since it's short enough to meet FSF's trivial change exception.

The eventual solution (if I write one) probably won't involve any of that code, but thanks for the offer. (This is why I generally recommend filing an issue before sending a PR, to avoid writing code that doesn't get used--unless you want the coding exercise, that is.)

If you get a chance you might want to call this out in your readme or in a CONTRIBUTING.org file too.

This is a very recent idea, brought to my attention by another user and contributor, so I haven't made it official yet. If and when I do, it will be documented.

By the way, while I see the same error you see when I just type h: as the query in org-ql-find, and I also use Vertico, it does not seem to prevent Vertico from working and presenting query results, e.g. if I continue typing an argument like h:emacs.

Now that's interesting... Do you know what commit or version of Vertico you're using? I'd like to try with the same one to see what's going on. I'm on minad/vertico@afd1e1b, which is in between tags 0.24 and 0.25 (the main branch is currently equivalent to 0.25, for context).

According to Emacs, I'm currently using Vertico 0.14, originally installed from GNU ELPA. I didn't realize it was so far out-of-date, but of course, it works fine, so why upgrade... :)

alphapapa · 2022-12-10T02:30:49Z

Hi Caleb,

I've pushed a fix for this problem. I decided to go over all of the predicates and make sure each one that needed to handle nil arguments worked correctly. In my tests with org-ql-find, this problem seems to be resolved. Please let me know if it is for you as well.

Thanks for your help with this issue.

chasecaleb · 2022-12-11T21:10:23Z

Tested and verified this fix works, no need for my workaround anymore. Thank you!

chasecaleb added 2 commits June 23, 2022 15:51

Fix: rx nil symbol in heading preamble

83cc00a

Use empty string instead of nil as fallback for "h:".

Fix: Catch errors in preamble evaluation

62f4e0b

alphapapa self-assigned this Jun 25, 2022

alphapapa added the bug label Jun 25, 2022

alphapapa added this to the 0.7 milestone Jun 25, 2022

alphapapa closed this in 9190818 Dec 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: prevent preamble evaluation breaking completion framework #285

Fix: prevent preamble evaluation breaking completion framework #285

Fix: prevent preamble evaluation breaking completion framework #285

Fix: prevent preamble evaluation breaking completion framework #285

Conversation

Reproduction

Fix