Property :inherit keyword argument not working

Stewmath commented 1 week ago

OS/platform

Arch Linux

Emacs version and provenance

Emacs-wayland v29.3 from Arch repositories

Emacs command

COLORTERM=truecolor emacs -nw

Org version and provenance

Org commit: f398724bd53eb6af3cf4187c864ec6f89a22ef59

From doom emacs

org-ql package version and provenance

fcb4e3ee62d2c6f9d2a9a8a510a50dd1a6eceb5e

Actions taken

I am probably missing something obvious here. But

I created a heading with the property "QL_DEPTH: 3"
I ran (org-ql-search (current-buffer) '(property "QL_DEPTH")), which worked correctly
I tried (org-ql-search (current-buffer) '(property "QL_DEPTH" :inherit t)), which returned no results
I tried (org-ql--predicate-property "QL_DEPTH" :inherit t) for testing purposes which produced an error (see backtrace)

Observed results

The :inherit keyword argument either caused errors or produced no results, even when there were results when :inherit wasn`t specified.

Expected results

The :inherit keyword argument should produce results

Backtrace

Results of (org-ql--predicate-property "QL_DEPTH" :inherit t):

Debugger entered--Lisp error: (error "Keyword argument t not one of (:inherit)")
  error("Keyword argument %s not one of (:inherit)" t)
  org-ql--predicate-property("QL_DEPTH" :inherit t)
  eval-expression((org-ql--predicate-property "QL_DEPTH" :inherit t) nil nil 127)
  funcall-interactively(eval-expression (org-ql--predicate-property "QL_DEPTH" :inherit t) nil nil 127)
  command-execute(eval-expression)

Removing t avoided the error but did not produce the correct result.

Etc.

Setting org-use-property-inheritance to t also had no effect on the queries.

alphapapa commented 1 week ago

Thanks for reporting this. Please try this branch and let me know if it works for you: https://github.com/alphapapa/org-ql/compare/fix/460-property-inherit

Stewmath commented 1 week ago

So, the good news is that the org-ql-search examples in my initial post are working now. There is still something a bit funky going on though.

I didn't really want to post this because of how horrifying it is, but this is the code I'm using:

  (defun hack-org-ql-completing-read (ql-completing-read &rest args)
    "Hack org-ql's completing-read to apply my custom sexp on top of the input"
    (cl-letf* ((str-to-sexp (symbol-function 'org-ql--query-string-to-sexp))
               ((symbol-function 'org-ql--query-string-to-sexp)
                (lambda (input)
                  `(and (or
                            (level '<= (string-to-number (or (property "QL_DEPTH" nil :inherit t) "1")))
                            (tags-local "ql"))
                        ,(funcall str-to-sexp input)))
                ))
      (apply ql-completing-read args)
      ))

  (advice-add #'org-ql-completing-read :around #'hack-org-ql-completing-read)

I made this so that org-ql-find would automatically be filtered with a complex s-expression on top of whatever I input. This works, but only when the nil parameter is included in the property predicate. If I omit nil then the same error as before occurs.

I wasn't able to reduce this to a simpler example I'm afraid. I don't even totally understand everything I did here, like why I had to quote "<=" within a quote. Amazingly though it does work now, but only when nil is added to the property query.

alphapapa commented 1 week ago

@Stewmath That is quite a hack. I think you should be able to use the :query-filter and/or :query-prefix arguments to org-ql-find to do what you need; that's what they're intended for.

Anyway, now that I see that, I think this would be a good time to talk about what you're actually trying to do with that (level '<= (string-to-number (or (property "QL_DEPTH" nil :inherit t) "1"))) clause in the query. There might be a better way to accomplish the same result.

alphapapa commented 1 week ago

@Stewmath Regarding:

This works, but only when the nil parameter is included in the property predicate. If I omit nil then the same error as before occurs.

That shouldn't be giving the same error, because I've added a test for that argument form, and it passes: https://github.com/alphapapa/org-ql/commit/a00be91ca17962401a84cfb9c6557170ca8a7670#diff-aaf61b821b82dbfc5f6679559b92652d45c66273959968e67b9e41d2afcb4ff2R1347

But I'm afraid the problem here is that, although the property predicate does return a value which can be used in simple expressions, the query pre-processor does not look into the (string-to-number ...) expression that contains it, so the argument form is not normalized to include the nil value argument.

Basically, as the documentation shows, the value argument is not really optional when passing :inherit t. The expected signature is (property &optional value &key inherit). This is because it uses cl-defun argument parsing. You can see the same behavior like so:

(cl-defun foo (property &optional value &key inherit)
  (list property value :inherit inherit))

(foo "PROPERTY" :inherit t)             ; Errors.
(foo "PROPERTY" nil :inherit t) ;; ("PROPERTY" nil :inherit t)

So when writing your code, if you want to specify the inherit keyword argument, you need to pass the value argument also.

Stewmath commented 1 week ago

@Stewmath That is quite a hack. I think you should be able to use the :query-filter and/or :query-prefix arguments to org-ql-find to do what you need; that's what they're intended for.

I didn't think that query-filter or query-prefix would work for my needs, as all they do is modify the input string that gets passed in - a non-sexp query, which doesn't seem very powerful. Certainly I couldn't use string-to-number in them.

Speaking of which, do you know why the following use of string-to-number doesn't work, when this kind of query would work within the context of my hack?

(org-ql-search (current-buffer) '(level <= (string-to-number (property "QL_DEPTH"))))

It gives a backtrace like this:

Debugger entered--Lisp error: (error "rx ‘repeat’ range error")
  signal(error ("rx ‘repeat’ range error"))
  error("rx `%s' range error" repeat)
  rx--translate-bounded-repetition(repeat (1 (string-to-number (property "QL_DEPTH")) "*"))
  rx--translate-repeat((1 (string-to-number (property "QL_DEPTH")) "*"))
  rx--translate-form((repeat 1 (string-to-number (property "QL_DEPTH")) "*"))
  rx--translate((repeat 1 (string-to-number (property "QL_DEPTH")) "*"))
  mapcar(rx--translate (bol (repeat 1 (string-to-number (property "QL_DEPTH")) "*") " "))
  rx--translate-seq((bol (repeat 1 (string-to-number (property "QL_DEPTH")) "*") " "))
  rx--translate-form((seq bol (repeat 1 (string-to-number (property "QL_DEPTH")) "*") " "))
  rx--translate((seq bol (repeat 1 (string-to-number (property "QL_DEPTH")) "*") " "))
  rx-to-string((seq bol (repeat 1 (string-to-number (property "QL_DEPTH")) "*") " ") t)
  #f(compiled-function (element) #<bytecode -0x1bc2f72d32597bc1>)((level <= (string-to-number (property "QL_DEPTH"))))
  mapcar(#f(compiled-function (element) #<bytecode -0x1bc2f72d32597bc1>) ((level <= (string-to-number (property "QL_DEPTH")))))
  org-ql--query-preamble((level <= (string-to-number (property "QL_DEPTH"))))
  org-ql-select(#<buffer other.org> (level <= (string-to-number (property "QL_DEPTH"))) :action element-with-markers :narrow nil :sort nil)
  org-ql-search(#<buffer other.org> (level <= (string-to-number (property "QL_DEPTH"))))
  eval((org-ql-search (current-buffer) '(level <= (string-to-number (property "QL_DEPTH")))) t)
  eval-expression((org-ql-search (current-buffer) '(level <= (string-to-number (property "QL_DEPTH")))) nil nil 127)
  funcall-interactively(eval-expression (org-ql-search (current-buffer) '(level <= (string-to-number (property "QL_DEPTH")))) nil nil 127)
  call-interactively(eval-expression nil nil)
  command-execute(eval-expression)

Anyway, now that I see that, I think this would be a good time to talk about what you're actually trying to do with that (level '<= (string-to-number (or (property "QL_DEPTH" nil :inherit t) "1"))) clause in the query. There might be a better way to accomplish the same result.

The purpose of the QL_DEPTH property is to specify how many levels deep an org-ql query should look. So if on a heading I give it the property QL_DEPTH:3, then org-ql will show me all headings down to depth 3, but only under that heading. I'm still experimenting with this but the idea is to be more selective with the headings that org-ql is showing me.

I'm certainly open to other ideas to implement something like that!

@Stewmath Regarding:

This works, but only when the nil parameter is included in the property predicate. If I omit nil then the same error as before occurs.

That shouldn't be giving the same error, because I've added a test for that argument form, and it passes: a00be91#diff-aaf61b821b82dbfc5f6679559b92652d45c66273959968e67b9e41d2afcb4ff2R1347

But I'm afraid the problem here is that, although the property predicate does return a value which can be used in simple expressions, the query pre-processor does not look into the (string-to-number ...) expression that contains it, so the argument form is not normalized to include the nil value argument.

Basically, as the documentation shows, the value argument is not really optional when passing :inherit t. The expected signature is (property &optional value &key inherit). This is because it uses cl-defun argument parsing. You can see the same behavior like so:
(cl-defun foo (property &optional value &key inherit)
  (list property value :inherit inherit))

(foo "PROPERTY" :inherit t)             ; Errors.
(foo "PROPERTY" nil :inherit t) ;; ("PROPERTY" nil :inherit t)
So when writing your code, if you want to specify the inherit keyword argument, you need to pass the value argument also.

I guess this falls under "elisp knowledge", but I assumed that since the value was an "optional" parameter, I didn't need to pass it. I suppose it conflicts with the keyword arguments though.

I'd consider this resolved on my end, now that I know that. I only pointed out the other failure case in my code in case you wanted to fix that, but it sounds like that's not feasible.

alphapapa commented 1 week ago

I didn't think that query-filter or query-prefix would work for my needs, as all they do is modify the input string that gets passed in - a non-sexp query, which doesn't seem very powerful. Certainly I couldn't use string-to-number in them.

Then you probably should define your own query predicate, which could then be applied to the user input as a prefix string. See the tutorial.

Speaking of which, do you know why the following use of string-to-number doesn't work, when this kind of query would work within the context of my hack?

Because that expression is not a valid Org QL query expression in terms of the level predicate. As the documentation says, the second argument must be a number, but you are providing a sexp. Query expressions are pre-processed for optimization in various ways, and that is not an expected query expression. For that to work, you probably need to provide a lambda instead of a query expression; but if you do that, it would preclude regexp-based optimizations, which could make the query slow in a large buffer.

So the best solution is probably to write your own query predicate, which could use the subtree-local value of that property. But you will still need to keep in mind that doing so makes that clause of the query impossible to optimize across a whole buffer, so it should probably be appended as the final clause in the query, and other clauses should be used which can be optimized to regexps to find initial matches, with your local-level-based predicate serving as a final clause.

Stewmath commented 1 week ago

I didn't think that query-filter or query-prefix would work for my needs, as all they do is modify the input string that gets passed in - a non-sexp query, which doesn't seem very powerful. Certainly I couldn't use string-to-number in them.

Then you probably should define your own query predicate, which could then be applied to the user input as a prefix string. See the tutorial.

That sounds logical.

Speaking of which, do you know why the following use of string-to-number doesn't work, when this kind of query would work within the context of my hack?

Because that expression is not a valid Org QL query expression in terms of the level predicate. As the documentation says, the second argument must be a number, but you are providing a sexp. Query expressions are pre-processed for optimization in various ways, and that is not an expected query expression. For that to work, you probably need to provide a lambda instead of a query expression; but if you do that, it would preclude regexp-based optimizations, which could make the query slow in a large buffer.

So the best solution is probably to write your own query predicate, which could use the subtree-local value of that property. But you will still need to keep in mind that doing so makes that clause of the query impossible to optimize across a whole buffer, so it should probably be appended as the final clause in the query, and other clauses should be used which can be optimized to regexps to find initial matches, with your local-level-based predicate serving as a final clause.

I guess my real question is why on earth this sexp was valid in my hacked function:

                            (level '<= (string-to-number (or (property "QL_DEPTH" nil :inherit t) "1")))

...But not in my other example. Because as you say, it doesn't seem valid to use string-to-number like that according to the documentation, and I was mildly surprised that it worked. It seems to be interpreting my inline elisp differently in that hacked-up context.

alphapapa commented 1 week ago

...But not in my other example. Because as you say, it doesn't seem valid to use string-to-number like that according to the documentation, and I was mildly surprised that it worked. It seems to be interpreting my inline elisp differently in that hacked-up context.

I just pushed a fix for that: https://github.com/alphapapa/org-ql/commit/6edcef19ddcf0b5480035e7a8016600c6b43e1d2 Had to look at the backtrace more carefully (was probably too late at night before). I think it should work now; try this branch, please: https://github.com/alphapapa/org-ql/compare/wip/v0.8.9

Stewmath commented 1 week ago

This does seem to fix it. More specifically, this command now works when it didn't before your last changes:

(org-ql-search (current-buffer) '(level '<= (string-to-number (or (property "QL_DEPTH" nil :inherit t) "1"))))

I'm still not sure why I need to quote the <= only when I start using embedded elisp, though.

I will say that I appreciate your fast responses. I'm unsure exactly if I'm finding bugs in org-ql or using it in unintended ways, but regardless I appreciate the help!

alphapapa commented 1 week ago

This does seem to fix it. More specifically, this command now works when it didn't before your last changes:

Great!

I'm still not sure why I need to quote the <= only when I start using embedded elisp, though.

Quoting is often confusing. In this case there are various levels of query pre-processing/rewriting, macro expansion, and pattern-matching going on. Sometimes the best answer is, that's just the way it is. :)

I will say that I appreciate your fast responses. I'm unsure exactly if I'm finding bugs in org-ql or using it in unintended ways, but regardless I appreciate the help!

A little bit of both, I think, which is great, as I could never find all the bugs myself. :) I also appreciate your quick replies so these issues could be iterated on. I'll probably make the v0.8.9 release with these fixes soon.

alphapapa / org-ql