bug#74848: [PATCH] calendar: check for presuppositions in `calendar-date-is-valid-p'

bug#74848: [PATCH] calendar: check for presuppositions in `calendar-date-is-valid-p'

[Richard Lawrence]

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

Tags: patch

Tags: patch

Hi all,

Here is a patch to make calendar-date-is-valid-p return nil, instead of
signaling an error, when it is passed a value that isn't a list of the
form (MONTH DAY YEAR).

This (a) makes the function a proper predicate, as its name suggests,
which (b) is necessary in some of the new iCalendar code I've been
working on (to be posted soon).

(I've deleted the build information about the Emacs I'm sending from,
because it is different from Emacs master, against which the patch is
formatted.)

This is my first time submitting a patch with submit-emacs-patch, so
please let me know if I did anything wrong.

Thanks!
Richard



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-calendar-check-for-presuppositions-in-calendar-date-.patch --]
[-- Type: text/patch, Size: 2622 bytes --]

From 794c27fac28260498b26e1cb60aa7576ca487f08 Mon Sep 17 00:00:00 2001
From: Richard Lawrence <rwl@recursewithless.net>
Date: Fri, 13 Dec 2024 10:41:02 +0100
Subject: [PATCH] calendar: check for presuppositions in
 `calendar-date-is-valid-p'

This function previously would signal an error if passed a value which
was not a three-element list of integers, making it not unusable as a
predicate for valid date values.
---
 lisp/calendar/calendar.el | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/lisp/calendar/calendar.el b/lisp/calendar/calendar.el
index 345687d1775..7c883617aca 100644
--- a/lisp/calendar/calendar.el
+++ b/lisp/calendar/calendar.el
@@ -2459,19 +2459,22 @@ calendar-nongregorian-visible-p
 
 (defun calendar-date-is-valid-p (date)
   "Return t if DATE is a valid date."
-  (let ((month (calendar-extract-month date))
-        (day (calendar-extract-day date))
-        (year (calendar-extract-year date)))
-    (and (<= 1 month) (<= month 12)
-         ;; (calendar-read-date t) used to return a date with day = nil.
-         ;; Should not be valid (?), since many funcs prob assume integer.
-         ;; (calendar-read-date 'noday) returns (month year), which
-         ;; currently results in calendar-extract-year returning nil.
-         day year (<= 1 day) (<= day (calendar-last-day-of-month month year))
-         ;; BC dates left as non-valid, to suppress errors from
-         ;; complex holiday algorithms not suitable for years BC.
-         ;; Note there are side effects on calendar navigation.
-         (<= 1 year))))
+  (when (and (listp date)
+             (length= date 3))
+    (let ((month (calendar-extract-month date))
+          (day (calendar-extract-day date))
+          (year (calendar-extract-year date)))
+      (and (integerp month) (integerp day) (integerp year)
+           (<= 1 month) (<= month 12)
+           ;; (calendar-read-date t) used to return a date with day = nil.
+           ;; Should not be valid (?), since many funcs prob assume integer.
+           ;; (calendar-read-date 'noday) returns (month year), which
+           ;; currently results in calendar-extract-year returning nil.
+           day year (<= 1 day) (<= day (calendar-last-day-of-month month year))
+           ;; BC dates left as non-valid, to suppress errors from
+           ;; complex holiday algorithms not suitable for years BC.
+           ;; Note there are side effects on calendar navigation.
+           (<= 1 year)))))
 
 (defun calendar-date-equal (date1 date2)
   "Return t if the DATE1 and DATE2 are the same."
-- 
2.39.5

bug#74848: [PATCH] calendar: check for presuppositions in `calendar-date-is-valid-p'

[Eli Zaretskii]

> From: Richard Lawrence <rwl@recursewithless.net>
> Date: Fri, 13 Dec 2024 13:02:36 +0100
> 
> Here is a patch to make calendar-date-is-valid-p return nil, instead of
> signaling an error, when it is passed a value that isn't a list of the
> form (MONTH DAY YEAR).
> 
> This (a) makes the function a proper predicate, as its name suggests,
> which (b) is necessary in some of the new iCalendar code I've been
> working on (to be posted soon).

Thanks.

> This is my first time submitting a patch with submit-emacs-patch, so
> please let me know if I did anything wrong.

The commit log message should include the file and the function(s)
where the changes are done, as well as a short description of the
changes.  See CONTRIBUTE for more details, and see "git log" for the
examples of how we format commit log messages.  Please don't forget to
mention the bug number in the commit log message.

P.S. I presume you've run the relevant parts of the test suite and
verified that they still succeed.  If not, please do.

bug#74848: [PATCH] calendar: check for presuppositions in `calendar-date-is-valid-p'

[Richard Lawrence]

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

Eli Zaretskii <eliz@gnu.org> writes:

> The commit log message should include the file and the function(s)
> where the changes are done, as well as a short description of the
> changes.  See CONTRIBUTE for more details, and see "git log" for the
> examples of how we format commit log messages.  Please don't forget to
> mention the bug number in the commit log message.

Woops, sorry, I did look at that file but I guess I missed that. New
patch attached.

> P.S. I presume you've run the relevant parts of the test suite and
> verified that they still succeed.  If not, please do.

Oddly enough, unless I'm missing something, there don't seem to be any
tests for calendar.el. There *are* a bunch of test files in
test/lisp/calendar, but they are all for other files, not for
calendar.el itself. The attached patch includes a new test file with a
basic test of the predicate, and make check reveals no relevant
problems.

Best,
Richard


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Check-for-presuppositions-in-calendar-date-is-valid-.patch --]
[-- Type: text/patch, Size: 4495 bytes --]

From 03be4ce15fbc4f3620bf332ab608b9abc0bf2500 Mon Sep 17 00:00:00 2001
From: Richard Lawrence <rwl@recursewithless.net>
Date: Fri, 13 Dec 2024 10:41:02 +0100
Subject: [PATCH] Check for presuppositions in `calendar-date-is-valid-p'

Do not signal an error in `calendar-date-is-valid-p' if passed a value
which is not a three-element list of integers. Signaling an error makes
the function unusable as a predicate for valid date values. (Bug#74848)

* lisp/calendar/calendar.el (calendar-date-is-valid-p): add the checks
* test/lisp/calendar/calendar-tests.el (calendar-test-validity-predicate):
test that the predicate returns nil for several invalid values
---
 lisp/calendar/calendar.el            | 29 +++++++++++++-----------
 test/lisp/calendar/calendar-tests.el | 34 ++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 13 deletions(-)
 create mode 100644 test/lisp/calendar/calendar-tests.el

diff --git a/lisp/calendar/calendar.el b/lisp/calendar/calendar.el
index 345687d1775..7c883617aca 100644
--- a/lisp/calendar/calendar.el
+++ b/lisp/calendar/calendar.el
@@ -2459,19 +2459,22 @@ calendar-nongregorian-visible-p
 
 (defun calendar-date-is-valid-p (date)
   "Return t if DATE is a valid date."
-  (let ((month (calendar-extract-month date))
-        (day (calendar-extract-day date))
-        (year (calendar-extract-year date)))
-    (and (<= 1 month) (<= month 12)
-         ;; (calendar-read-date t) used to return a date with day = nil.
-         ;; Should not be valid (?), since many funcs prob assume integer.
-         ;; (calendar-read-date 'noday) returns (month year), which
-         ;; currently results in calendar-extract-year returning nil.
-         day year (<= 1 day) (<= day (calendar-last-day-of-month month year))
-         ;; BC dates left as non-valid, to suppress errors from
-         ;; complex holiday algorithms not suitable for years BC.
-         ;; Note there are side effects on calendar navigation.
-         (<= 1 year))))
+  (when (and (listp date)
+             (length= date 3))
+    (let ((month (calendar-extract-month date))
+          (day (calendar-extract-day date))
+          (year (calendar-extract-year date)))
+      (and (integerp month) (integerp day) (integerp year)
+           (<= 1 month) (<= month 12)
+           ;; (calendar-read-date t) used to return a date with day = nil.
+           ;; Should not be valid (?), since many funcs prob assume integer.
+           ;; (calendar-read-date 'noday) returns (month year), which
+           ;; currently results in calendar-extract-year returning nil.
+           day year (<= 1 day) (<= day (calendar-last-day-of-month month year))
+           ;; BC dates left as non-valid, to suppress errors from
+           ;; complex holiday algorithms not suitable for years BC.
+           ;; Note there are side effects on calendar navigation.
+           (<= 1 year)))))
 
 (defun calendar-date-equal (date1 date2)
   "Return t if the DATE1 and DATE2 are the same."
diff --git a/test/lisp/calendar/calendar-tests.el b/test/lisp/calendar/calendar-tests.el
new file mode 100644
index 00000000000..c41f14d3b54
--- /dev/null
+++ b/test/lisp/calendar/calendar-tests.el
@@ -0,0 +1,34 @@
+;;; calendar-tests.el --- tests for calendar/calendar.el  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; Author: Richard Lawrence <rwl@recursewithless.net>
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'calendar)
+
+(ert-deftest calendar-test-validity-predicate ()
+  (should (eq (calendar-date-is-valid-p nil) nil))
+  (should (eq (calendar-date-is-valid-p "invalid") nil))
+  (should (eq (calendar-date-is-valid-p (list 1 2)) nil))
+  (should (eq (calendar-date-is-valid-p (list 5 1 2025)) t)))
+
+(provide 'calendar-tests)
+;;; calendar-tests.el ends here
-- 
2.39.5

bug#74848: [PATCH] calendar: check for presuppositions in `calendar-date-is-valid-p'

[Eli Zaretskii]

> From: Richard Lawrence <rwl@recursewithless.net>
> Cc: 74848@debbugs.gnu.org
> Date: Sat, 14 Dec 2024 13:19:19 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The commit log message should include the file and the function(s)
> > where the changes are done, as well as a short description of the
> > changes.  See CONTRIBUTE for more details, and see "git log" for the
> > examples of how we format commit log messages.  Please don't forget to
> > mention the bug number in the commit log message.
> 
> Woops, sorry, I did look at that file but I guess I missed that. New
> patch attached.
> 
> > P.S. I presume you've run the relevant parts of the test suite and
> > verified that they still succeed.  If not, please do.
> 
> Oddly enough, unless I'm missing something, there don't seem to be any
> tests for calendar.el. There *are* a bunch of test files in
> test/lisp/calendar, but they are all for other files, not for
> calendar.el itself. The attached patch includes a new test file with a
> basic test of the predicate, and make check reveals no relevant
> problems.

Thanks, installed on the master branch, and closing the bug.

I needed to tweak the commit log message somewhat; please see how I
changed it and try to follow these conventions in the future.