bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries

[hokomo <hokomo@airmail.cc>]

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

>> `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee
>> that the sexp event occurs at least once. It looks like there's a
>> different bug (?) where, even if an entry is recognized as an
>> arbitrary diary sexp, if it doesn't produce any events, the converter
>> will go ahead with trying to interpret it in a different way and
>> eventually fail. E.g., lowering the enumeration days to 0 gives:
>>
>> "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1)
>> First Sunday in July 2’"
>>
>> after exhausting all of the known entry types. Should I file this as a
>> separate bug?
> 
> No, we can work on this problem here in this bug report.
> 
> Do you have a recipe to reproduce the problem, starting from "emacs -Q"?

Yep, you can use the same testing script and just replace the last 
expression with:

(let ((icalendar-export-sexp-enumeration-days 0))
   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))

which should give you the error I mentioned. I don't have a patch for 
this on hand but it would probably require restructuring the surrounding 
code a little bit.

There's another bug that concerns sexps that contain more than a single 
closing parenthesis. Seems like the icalendar code uses a bunch of 
regexes to parse the sexp (see `icalendar--convert-sexp-to-ical'), 
rather than something like `read' or `read-from-string' (which is what 
diary does, as well as org-agenda). To reproduce (with the same testing 
code as before):

(let ((icalendar-export-sexp-enumeration-days 366))
   (test-diary-sexp "%%(= (calendar-day-of-week date) 0) Sunday 1")
   (test-diary-sexp "%%(= 0 (calendar-day-of-week date)) Sunday 2"))

No matter whether the closing parentheses are bunched together or 
separated, parsing fails either way. I've attached a draft of a patch 
that modifies as little as possible and makes the above two cases work, 
but that's not enough as there's special handling of `and' forms. 
Furthermore, sexps that span multiple lines fail for the same reason, 
even though diary handles them just fine of course. Ideally the whole 
function should be rewritten to just use `read-from-string'. I don't 
understand why `and' forms are handled specially though, so I'm not sure 
how to proceed here.

A last thing to note -- even though org-agenda properly handles 
multiline sexps and sexps with an arbitrary number of closing 
parentheses (because it does the parsing manually and uses 
`forward-sexp'), Org's parser itself does not properly handle multiline 
sexps (but does handle multiple closing parentheses). To confirm, use 
`org-element-at-point' at the beginning of these two diary sexps in an 
org-mode buffer:

%%(let ((a 123) (b 456)) (+ a b))

%%(let ((a 123)
         (b 456))
    (+ a b))

All of these bugs also propagate to ox-icalendar (which is how I got 
into exploring this whole thing) since it relies on the Org parse tree 
to pull out 'diary-sexp elements and on icalendar to export them into 
iCalendar format.

Let me know if you want me to report any of these as separate bugs and 
what would be the best thing to do here.

Regards,
hokomo

[-- Attachment #2: 0002-Draft-fix-nested-diary-sexps.patch --]
[-- Type: text/x-patch, Size: 1197 bytes --]

From e35ea1f3a9c62d9c07d10d9ec89bf879058775c2 Mon Sep 17 00:00:00 2001
From: hokomo <hokomo@airmail.cc>
Date: Sat, 25 Jun 2022 13:43:28 +0200
Subject: [PATCH] Fix nested diary sexps

---
 lisp/calendar/icalendar.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el
index 46bcf4566f..2b6af79812 100644
--- a/lisp/calendar/icalendar.el
+++ b/lisp/calendar/icalendar.el
@@ -1641,9 +1641,11 @@ icalendar--convert-sexp-to-ical
                        entry-main)
          ;; regular sexp entry
          (icalendar--dmsg "diary-sexp %s" entry-main)
-         (let ((p1 (substring entry-main (match-beginning 1) (match-end 1)))
-               (p2 (substring entry-main (match-beginning 2) (match-end 2)))
-               (now (or start (current-time))))
+         (let* ((entry-main (substring entry-main 2))
+                (res (read-from-string entry-main))
+                (p1 (prin1-to-string (car res)))
+                (p2 (substring entry-main (cdr res)))
+                (now (or start (current-time))))
            (delete nil
                    (mapcar
                     (lambda (offset)
-- 
2.35.3