`org-element-cache-map' misses elements at end of buffer

`org-element-cache-map' misses elements at end of buffer

[Morgan Smith]

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

Hello!

I may or may not have been trying to rewrite `org-clock-sum' for like
the 5th time.

Anyways I was wanting to parse a file using `org-element-cache-map' that
looked roughly like this:

========
* clock
:LOGBOOK:
CLOCK: [2024-03-24 Sun 15:18]--[2024-03-24 Sun 15:39] =>  0:21
:END:
=======

However I couldn't get the clock element this way.  After much trail and
error I found that simply adding another heading on the end allowed me
to get the clock element!

I've attached a test so you can reproduce the problem.  In the test it
is a paragraph element that is not showing up.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-failing-test.patch --]
[-- Type: text/x-patch, Size: 1212 bytes --]

From 9deff2111b73bb2ceb9127db5d88486affa04f0b Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Thu, 18 Apr 2024 09:18:51 -0400
Subject: [PATCH] failing test

---
 testing/lisp/test-org-element.el | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/testing/lisp/test-org-element.el b/testing/lisp/test-org-element.el
index 8b2cf1642..313556f49 100644
--- a/testing/lisp/test-org-element.el
+++ b/testing/lisp/test-org-element.el
@@ -4983,6 +4983,19 @@ Text
 
 \f
 ;;; Test Cache.
+(ert-deftest test-org-element/cache-map ()
+  "Test `org-element-cache-map'."
+  (org-test-with-temp-text "* headline\n:DRAWER:\nparagraph\n:END:\n* headline 2"
+    (should
+     (equal
+      '(org-data headline section drawer paragraph headline)
+      (org-element-cache-map #'car :granularity 'element))))
+  (org-test-with-temp-text "* headline\n:DRAWER:\nparagraph\n:END:"
+    (should
+     (equal
+      ;; This fails because paragraph isn't found!
+      '(org-data headline section drawer paragraph)
+      (org-element-cache-map #'car :granularity 'element)))))
 
 (ert-deftest test-org-element/cache ()
   "Test basic expectations and common pitfalls for cache."
-- 
2.41.0

Re: `org-element-cache-map' misses elements at end of buffer

[Morgan Smith]

So I found another bug in `org-element-cache-map'.

Executing the following code just freezes up.  I am struggling to work
through the logic of `org-element-cache-map'.  If no-one else magically
solves my issues, I'll figure it out eventually.  But I would appreciate
some advice on how to debug this stuff (both my issue of missing
elements and freezing).

I guess my next current step is to read the manual on breakpoints.  I'm
getting a little tired of just shoving print statements everywhere.

#+BEGIN_SRC elisp
;; I get that this use case looks weird but in a more complicated
;; situation I also experienced freezing so I'm hoping solving this case
;; will also solve my other situation.
(require 'org-test)
(org-test-with-temp-text "* headline\nCLOCK: => 23:43\n"
    (let ((headline (progn (goto-char (point-min)) (org-element-at-point))))
     (org-element-cache-map
     #'car
     :granularity 'element
     :next-re org-element-clock-line-re
     :after-element headline)))
#+END_SRC

Re: `org-element-cache-map' misses elements at end of buffer

[Ihor Radchenko]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> I may or may not have been trying to rewrite `org-clock-sum' for like
> the 5th time.
>
> Anyways I was wanting to parse a file using `org-element-cache-map' that
> looked roughly like this:
>
> ========
> * clock
> :LOGBOOK:
> CLOCK: [2024-03-24 Sun 15:18]--[2024-03-24 Sun 15:39] =>  0:21
> :END:
> =======
>
> However I couldn't get the clock element this way.  After much trail and
> error I found that simply adding another heading on the end allowed me
> to get the clock element!
>
> I've attached a test so you can reproduce the problem.  In the test it
> is a paragraph element that is not showing up.

Thanks for reporting!
Fixed, on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=942a7320d

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: `org-element-cache-map' misses elements at end of buffer

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

> So I found another bug in `org-element-cache-map'.
>
> Executing the following code just freezes up.  I am struggling to work
> through the logic of `org-element-cache-map'.  If no-one else magically
> solves my issues, I'll figure it out eventually.  But I would appreciate
> some advice on how to debug this stuff (both my issue of missing
> elements and freezing).

Hmm. :after-element keyword logic is broken. It does not account for the
case when :after-element is past the START point.

It is the time to refactor this function yet again.
(a tricky endeavour considering all the edge cases we can encounter when
there are changes in buffer while `org-element-cache-map' is mapping
over it).

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: `org-element-cache-map' misses elements at end of buffer

[Morgan Smith]

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

Ihor Radchenko <yantar92@posteo.net> writes:

>
> Hmm. :after-element keyword logic is broken. It does not account for the
> case when :after-element is past the START point.
>
> It is the time to refactor this function yet again.
> (a tricky endeavour considering all the edge cases we can encounter when
> there are changes in buffer while `org-element-cache-map' is mapping
> over it).

See attached for a way to break :from-pos as well.  I would like to help
refactor but studying this function is a little dizzying for me.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-test-org-element-cache-map-Add-test-for-from-pos.patch --]
[-- Type: text/x-patch, Size: 1632 bytes --]

From eb37560b9c94bed6e91d6834462173d0a6d7d44b Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Fri, 19 Apr 2024 10:46:28 -0400
Subject: [PATCH] test-org-element/cache-map: Add test for :from-pos

* testing/lisp/test-org-element.el (test-org-element/cache-map): Add
test for :from-pos.
---
 testing/lisp/test-org-element.el | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/testing/lisp/test-org-element.el b/testing/lisp/test-org-element.el
index ad4ff1ce9..a0dbc726e 100644
--- a/testing/lisp/test-org-element.el
+++ b/testing/lisp/test-org-element.el
@@ -4994,7 +4994,21 @@ Text
    (equal
     '(org-data headline section drawer paragraph)
     (org-test-with-temp-text "* headline\n:DRAWER:\nparagraph\n:END:"
-      (org-element-cache-map #'car :granularity 'element)))))
+      (org-element-cache-map #'car :granularity 'element))))
+  (should
+   (equal
+    ;; This fails returning '(paragraph paragraph) because it returns
+    ;; the paragraph under "headline".  This is because at
+    ;; org-element.el:8028 we travel up the parents of drawer trying
+    ;; to find a paragraph.
+    '(paragraph)
+    (org-test-with-temp-text "* headline\nparagraph\n<point>** subheading\n:DRAWER:\nparagraph\n:END:\n"
+      (let ((subheading (org-element-at-point)))
+        (org-element-cache-map
+         #'car
+         :granularity 'element
+         :restrict-elements '(paragraph)
+         :from-pos (org-element-contents-begin subheading)))))))
 
 (ert-deftest test-org-element/cache ()
   "Test basic expectations and common pitfalls for cache."
-- 
2.41.0

Re: `org-element-cache-map' misses elements at end of buffer

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

>> It is the time to refactor this function yet again.
>> (a tricky endeavour considering all the edge cases we can encounter when
>> there are changes in buffer while `org-element-cache-map' is mapping
>> over it).
>
> See attached for a way to break :from-pos as well.  I would like to help
> refactor but studying this function is a little dizzying for me.

At this point, it might be easier to rewrite the whole thing from
scratch, just based on the commentary (I tried my best to leave detailed
commentary explaining the intended logic). We have a decent test
coverage when mapping headings in buffer, so edge cases will be checked
by make test.

In particular, it might be a good idea to get rid of the idea of START
variable and just make use of AFTER-ELEMENT. For now, they serve kinda
the same purpose, but the latter was added because START was not enough
when the buffer is modified by FUNC.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>