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

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

[hokomo]

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

icalendar is capable of exporting arbitrary diary sexp entries, but it 
looks like there's a bug in the code. Each conversion function 
`icalendar--convert-*-to-ical' can return either a dotted pair or a list 
of such pairs, but the code fails to differentiate the two cases properly.

To reproduce:

(require 'icalendar)

(defun test-diary-sexp (sexp)
   (message "Testing %S\n" sexp)
   (let ((file (make-temp-file "export.ics")))
     (with-temp-buffer
       (insert sexp)
       (icalendar-export-region (point-min) (point-max) file))
     (with-current-buffer (get-buffer "*icalendar-errors*")
       (message "export: %S\n" (with-temp-buffer
                             (insert-file-contents file)
                             (buffer-string)))
       (message "errors: %S" (buffer-string))))
   (terpri))

(defun my-float (&rest args)
   (apply #'diary-float args))

(let ((icalendar-export-sexp-enumeration-days 366))
   (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"))

Exporting the my-float sexp will fail with:

"Error in line 0 -- (wrong-type-argument listp First Sunday in July 2): 
‘%%(my-float 7 0 1) First Sunday in July 2"

because the returned list is confused for a dotted pair.

`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?

Regards,
hokomo

[-- Attachment #2: 0001-Fix-detecting-dotted-pairs.patch --]
[-- Type: text/x-patch, Size: 1083 bytes --]

From 691f34a301359893bcffff8e4e0c8794a1fd5e68 Mon Sep 17 00:00:00 2001
From: hokomo <hokomo@airmail.cc>
Date: Sat, 25 Jun 2022 09:44:54 +0200
Subject: [PATCH] Fix detecting dotted pairs

* lisp/calendar/icalendar.el (icalendar-export-region): Fix detecting
dotted pairs.
---
 lisp/calendar/icalendar.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el
index 1a5a071e20..cf54293989 100644
--- a/lisp/calendar/icalendar.el
+++ b/lisp/calendar/icalendar.el
@@ -1144,7 +1144,8 @@ icalendar-export-region
                                      (cdr contents-n-summary))))
                       (setq result (concat result header contents alarm
                                            "\nEND:VEVENT")))
-                    (if (consp cns-cons-or-list)
+                    (if (and (consp cns-cons-or-list)
+                             (not (listp (cdr cns-cons-or-list))))
                         (list cns-cons-or-list)
                       cns-cons-or-list)))
           ;; handle errors
-- 
2.35.3

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

[Lars Ingebrigtsen]

hokomo <hokomo@airmail.cc> writes:

> Exporting the my-float sexp will fail with:
>
> "Error in line 0 -- (wrong-type-argument listp First Sunday in July
> 2): ‘%%(my-float 7 0 1) First Sunday in July 2"
>
> because the returned list is confused for a dotted pair.

Thanks; patch pushed to Emacs 29.

> `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"?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[hokomo]

[-- 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

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

[Lars Ingebrigtsen]

hokomo <hokomo@airmail.cc> writes:

> 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.

Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
he has some comments.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[hokomo]

> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
> he has some comments.

It's been a week since then. Any ideas on how to move forward or should 
we give it more time? :-)

To summarize, the problems are:

(1) A diary sexp not generating any events for the configured number of 
enumeration days is treated effectively as a parsing failure, meaning 
that the next type of diary entry is tried instead of just not producing 
any events.

(2) Improper parsing of diary sexps (using regexes instead of e.g. 
`read') leads to diary sexps not being able to span multiple lines or 
have more than a single closing parenthesis (i.e. any sort of nested 
expression).

(3) Not strictly in the scope of icalendar itself, but still related the 
handling of diary sexps: Org's parser seems to assume that each diary 
sexp is on a single line. This and the two limitations above end up 
propagating to ox-icalendar.

hokomo

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

[Lars Ingebrigtsen]

hokomo <hokomo@airmail.cc> writes:

> It's been a week since then. Any ideas on how to move forward or
> should we give it more time? :-)

If you have a possible patch for the issues, that'd be appreciated.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

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

[Ulf Jasper]

Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
> hokomo <hokomo@airmail.cc> writes:
>
>> 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.
>
> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
> he has some comments.

Sorry for the late reply.  Thanks for the bug report, the patch and for
analysing the problems.  I'll try to have a look at the icalendar code
the next days (or so).

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

[Stefan Kangas]

Ulf Jasper <ulf.jasper@web.de> writes:

> Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
>> hokomo <hokomo@airmail.cc> writes:
>>
>>> 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.
>>
>> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
>> he has some comments.
>
> Sorry for the late reply.  Thanks for the bug report, the patch and for
> analysing the problems.  I'll try to have a look at the icalendar code
> the next days (or so).

That was 15 weeks ago, so here's a friendly ping.

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

[Ulf Jasper]

I added a test to icalendar-tests.el that verifies that patch
0001-Fix-detecting-dotted-pairs.patch works as expected.

I also prepared two additional tests for the other examples where
icalendar-export fails.  For the time being they are commented out as
there is no fix available yet.  Patch
0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples
but apparently breaks test 'icalendar-real-world'.  So a look at
'icalendar--convert-sexp-to-ical' seems to be necessary.

Best,
Ulf

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

[Ulf Jasper]

Am 23.11.2022 um 21:24 (+0100) schrieb Ulf Jasper:
> I added a test to icalendar-tests.el that verifies that patch
> 0001-Fix-detecting-dotted-pairs.patch works as expected.
>
> I also prepared two additional tests for the other examples where
> icalendar-export fails.  For the time being they are commented out as
> there is no fix available yet.  Patch
> 0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples
> but apparently breaks test 'icalendar-real-world'.  So a look at
> 'icalendar--convert-sexp-to-ical' seems to be necessary.

There was a testcase in 'icalendar-real-world' that relied on
icalendar-export to fail on a particular sexp.  With patch 0002... that
testcase behaves differently (i.e. export works correctly now).  In
other words: breaking that test was correct.

Applied patch 0002... and updated icalendar-tests.

Best,
Ulf

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

[Brendan O'Dea]

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

On Wed, Nov 23, 2022 at 09:24:45PM +0100, Ulf Jasper wrote:
>I added a test to icalendar-tests.el that verifies that patch
>0001-Fix-detecting-dotted-pairs.patch works as expected.

Note that this test is failing on both the current 'master' and
'emacs-29' branch (excerpt, full log attached):

  "Error in line 0 -- (error Could not parse date): `%%(icalendar-tests--diary-float 7 0 1) First Sunday in July 2'\n"
  FAILED  25/42  icalendar-export-bug-56241-dotted-pair (0.003671 sec) at lisp/calendar/icalendar-tests.el:1000

From what I can tell, the problem is that while these calls look like
they should be identical:

  (diary-float 7 0 1)
  (icalendar-tests--diary-float 7 0 1)  ;; calls diary-float

in practice, they are handled by different code paths:

  icalendar--convert-float-to-ical
  icalendar--convert-sexp-to-ical

resp.  The key difference being that the first one sets 'date' and
'entry' with calendar-dlet when calling diary-float*, and the latter
does not.  diary-float is documented as requiring those to be set.†

I'm not sure of the fix here: if the expectation is that some arbitrary
sexp should be able to indirectly call some of the diary-*‡ functions,
then it may be that icalendar--convert-sexp-to-ical may need to
similarly set those values.  Alternately, if that is not supported, then
maybe disable/remove the test.

--bod

* https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/calendar/icalendar.el?id=b42cd524b46a4f29ef13e9d03be9d3df917f9aa3#n1787
† https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/calendar/diary-lib.el?id=b42cd524b46a4f29ef13e9d03be9d3df917f9aa3#n1891
‡ diary-date, diary-block, diary-float, diary-anniversary, diary-cyclic,
  and diary-offset (there may be others, those were the ones with
  an explicit comment)

[-- Attachment #2: icalendar-tests.log.gz --]
[-- Type: application/gzip, Size: 1770 bytes --]

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

[Mattias Engdegård]

Thank you for looking into these failures.

The test started failing around the first Sunday of July, which is unlikely to be a coincidence.
Calendar tests had better not depend on the time or date when the test is run. If necessary, these should be mocked up by the tests.

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

[Mattias Engdegård]

The `icalendar-export-bug-56241-dotted-pair` test has now been marked :unstable, which means that it is not run automatically by `make check`.
The problem still remains, and it would be good to have a working regression test for the bug, assuming it has indeed been fixed.

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

[Jeremy Harrison via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Not sure if this helps but I was wondering why exporting '%%(org-anniversary 2020 02 02) blah %d blah' worked but '%%(org-anniversary 2020 03 02) blah %d blah' did not.

Long story short, icalendar-export-sexp-enumeration-days was set to 14 by default. I have changed this to 366 and the export works again.

If relevant, I noticed that one of the tests mentioned above does not set the value explicitly while the others do.

I'm not a programmer so take my observations with due caution!

Jeremy

[-- Attachment #2: Type: text/html, Size: 1343 bytes --]