bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Maxim Nikulin]

Consider the following example:

     (format-time-string
      "%F %T %Z %z"
      (encode-time
       (make-decoded-time :year 2022 :month 3 :day 31
     			 :hour 23 :minute 30 :second 0
     			 :zone "Europe/Madrid"))
      "Europe/Madrid")
     "2022-04-01 00:30:00 CEST +0200"

I believe that the result should be
     "2022-03-31 23:30:00 CEST +0200"
It can be obtained if :dst -1 is explicitly added to the 
`make-decoded-time' arguments.

Since `make-decoded-time' is defined using `cl-defun', I think, it is 
better to use -1 ("guess") default value for the :dst argument, not nil 
that explicitly says "no daylight saving time".

There is `decoded-time-set-defaults', but it does not help

     (format-time-string
      "%F %T %Z %z"
      (encode-time
       (decoded-time-set-defaults
        (make-decoded-time :year 2022 :month 3 :day 31
			  :hour 23 :minute 30)
        "Europe/Madrid"))
      "Europe/Madrid")
     "2022-04-01 01:30:00 CEST +0200"

This case I have no idea how to fix the issue.

An example in the `decoded-time-add' docstring
 >   (decoded-time-add (decode-time) (make-decoded-time :month 2))
adds even more confusion. If `make-decoded-time' is intended for 
intervals, not timestamps than it should not have DST and TZ values at 
all. Time interval may be added to timestamp, and time zone and daylight 
saving time flag is the property of particular timestamp while the same 
interval may be added to various timestamps and the actual result 
depends on the base timestamp.

Timestamp and interval are different types and should not be used 
interchangeably. nil/t/-1 interpretation difference for DST causes 
issues like (bug#54731), so it should be handled with care.

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Lars Ingebrigtsen]

Maxim Nikulin <m.a.nikulin@gmail.com> writes:

> Consider the following example:
>
>     (format-time-string
>      "%F %T %Z %z"
>      (encode-time
>       (make-decoded-time :year 2022 :month 3 :day 31
>     			 :hour 23 :minute 30 :second 0
>     			 :zone "Europe/Madrid"))
>      "Europe/Madrid")
>     "2022-04-01 00:30:00 CEST +0200"
>
> I believe that the result should be
>     "2022-03-31 23:30:00 CEST +0200"
> It can be obtained if :dst -1 is explicitly added to the
> `make-decoded-time' arguments.
>
> Since `make-decoded-time' is defined using `cl-defun', I think, it is
> better to use -1 ("guess") default value for the :dst argument, not
> nil that explicitly says "no daylight saving time".

I think that makes some sense, but on the other hand, that's just a
simple helper function that does what it says -- "with only the keywords
given filled out".  But perhaps -1 is less "filled out" than nil in this
case.

> There is `decoded-time-set-defaults', but it does not help
>
>     (format-time-string
>      "%F %T %Z %z"
>      (encode-time
>       (decoded-time-set-defaults
>        (make-decoded-time :year 2022 :month 3 :day 31
> 			  :hour 23 :minute 30)
>        "Europe/Madrid"))
>      "Europe/Madrid")
>     "2022-04-01 01:30:00 CEST +0200"
>
> This case I have no idea how to fix the issue.

It's this code, I guess:

  ;; When we don't have a time zone, default to DEFAULT-ZONE without
  ;; DST if DEFAULT-ZONE if given, and to unknown DST otherwise.
  (unless (decoded-time-zone time)
    (if default-zone
	(progn (setf (decoded-time-zone time) default-zone)
	       (setf (decoded-time-dst time) nil))
      (setf (decoded-time-dst time) -1)))

I've added Paul to the CCs; perhaps he has some comments.

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

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Paul Eggert]

On 5/26/22 05:13, Lars Ingebrigtsen wrote:
> perhaps -1 is less "filled out" than nil in this
> case.

Indeed it is, and make-decoded-time's DST flag should default to -1.

It's unfortunate that nil means "standard time" in these contexts. In 
hindsight some other symbol should have been used to mean "standard 
time". Could be too late to change this though.


> It's this code, I guess:
> 
>    ;; When we don't have a time zone, default to DEFAULT-ZONE without
>    ;; DST if DEFAULT-ZONE if given, and to unknown DST otherwise.
>    (unless (decoded-time-zone time)
>      (if default-zone
> 	(progn (setf (decoded-time-zone time) default-zone)
> 	       (setf (decoded-time-dst time) nil))
>        (setf (decoded-time-dst time) -1)))

This looks wrong. Shouldn't it leave the DST flag alone? I.e., just this:

   (unless (decoded-time-zone time)
     (setf (decoded-time-zone-time) default-zone))

That is, if we assume that for the DST component -1 means "unknown" and 
nil means "standard time", it should be OK for decoded-time-set-defaults 
to leave the DST component alone, for the same reason that it leaves the 
DOW component alone.

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

>> perhaps -1 is less "filled out" than nil in this
>> case.
>
> Indeed it is, and make-decoded-time's DST flag should default to -1.

So I've now made this change in Emacs 29.

>> It's this code, I guess:
>>    ;; When we don't have a time zone, default to DEFAULT-ZONE
>> without
>>    ;; DST if DEFAULT-ZONE if given, and to unknown DST otherwise.
>>    (unless (decoded-time-zone time)
>>      (if default-zone
>> 	(progn (setf (decoded-time-zone time) default-zone)
>> 	       (setf (decoded-time-dst time) nil))
>>        (setf (decoded-time-dst time) -1)))
>
> This looks wrong. Shouldn't it leave the DST flag alone? I.e., just this:
>
>   (unless (decoded-time-zone time)
>     (setf (decoded-time-zone-time) default-zone))
>
> That is, if we assume that for the DST component -1 means "unknown"
> and nil means "standard time", it should be OK for
> decoded-time-set-defaults to leave the DST component alone, for the
> same reason that it leaves the DOW component alone.

Yes, I think so.  But you changed this in a391ffa2f03, and you usually
have a good reason for changes like this, so I thought there must be
something subtle going on here I didn't quite get.  😀

The old code doesn't look quite right, either, I think...

-  ;; When we don't have a time zone and we don't have a DST, then mark
-  ;; it as unknown.
-  (when (and (not (decoded-time-zone time))
-             (not (decoded-time-dst time)))
-    (setf (decoded-time-dst time) -1))
-
-  (when (and (not (decoded-time-zone time))
-             default-zone)
-    (setf (decoded-time-zone time) 0))


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

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Paul Eggert]

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

On 5/27/22 03:40, Lars Ingebrigtsen wrote:
>> This looks wrong. Shouldn't it leave the DST flag alone? I.e., just this:
>>
>>    (unless (decoded-time-zone time)
>>      (setf (decoded-time-zone-time) default-zone))
>>
>> That is, if we assume that for the DST component -1 means "unknown"
>> and nil means "standard time", it should be OK for
>> decoded-time-set-defaults to leave the DST component alone, for the
>> same reason that it leaves the DOW component alone.

> Yes, I think so.  But you changed this in a391ffa2f03, and you usually
> have a good reason for changes like this, so I thought there must be
> something subtle going on here I didn't quite get.  😀

Thanks for the compliment, not sure it's deserved here....


> The old code doesn't look quite right, either, I think...
> 
> -  ;; When we don't have a time zone and we don't have a DST, then mark
> -  ;; it as unknown.
> -  (when (and (not (decoded-time-zone time))
> -             (not (decoded-time-dst time)))
> -    (setf (decoded-time-dst time) -1))
> -
> -  (when (and (not (decoded-time-zone time))
> -             default-zone)
> -    (setf (decoded-time-zone time) 0))

Yes, that old code was wrong because it incorrectly assumeed that (not 
(decoded-time-dst time)) means the DST flag is unspecified, whereas it 
really means that the DST flag is specifying standard time.

It also looked odd because default-zone was used only as a boolean, even 
though its name suggests that it's the default time zone. This usage 
dates back to commit fa648a59c9818ae284209ac7ae4f3700aebd92c9 which you 
installed in July 2019. The only call using default-zone in Emacs is in 
newsticker--decode-iso8601-date, which passes 0 so that the oddity in 
the implementation makes no difference there.

Part of the confusion here is that nil doesn't mean "no time zone is 
known"; it means "use the Emacs default time zone". In other words, nil 
has the same interpretation problem in time zones that it has in DST 
flags - it doesn't mean "unknown".

To try to lessen the confusion I installed the attached, which fixes the 
reported bug so I'll close the bug report. Please feel free to revert if 
you see a problem with it (I'm just trying to save time here by being bold).

[-- Attachment #2: 0001-decoded-time-set-defaults-now-leaves-DST-alone.patch --]
[-- Type: text/x-patch, Size: 1714 bytes --]

From fe38cbc14fb627f39e145b9a85f029af96f903c7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 27 May 2022 12:19:43 -0700
Subject: [PATCH] decoded-time-set-defaults now leaves DST alone
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/calendar/time-date.el (decoded-time-set-defaults):
Don’t mess with decoded-time-dst (Bug#55635).
---
 lisp/calendar/time-date.el | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/calendar/time-date.el b/lisp/calendar/time-date.el
index dc77a7c7e0..40374c3bb4 100644
--- a/lisp/calendar/time-date.el
+++ b/lisp/calendar/time-date.el
@@ -562,6 +562,9 @@ decoded-time-set-defaults
 This year is used to guarantee portability; see Info
 node `(elisp) Time of Day'.
 
+Optional argument DEFAULT-ZONE specifies what time zone to
+default to when TIME's time zone is nil (meaning local time).
+
 TIME is modified and returned."
   (unless (decoded-time-second time)
     (setf (decoded-time-second time) 0))
@@ -577,13 +580,11 @@ decoded-time-set-defaults
   (unless (decoded-time-year time)
     (setf (decoded-time-year time) 1970))
 
-  ;; When we don't have a time zone, default to DEFAULT-ZONE without
-  ;; DST if DEFAULT-ZONE if given, and to unknown DST otherwise.
   (unless (decoded-time-zone time)
-    (if default-zone
-	(progn (setf (decoded-time-zone time) default-zone)
-	       (setf (decoded-time-dst time) nil))
-      (setf (decoded-time-dst time) -1)))
+    (setf (decoded-time-zone time) default-zone))
+
+  ;; Do not set decoded-time-weekday or decoded-time-dst,
+  ;; as encode-time can infer them well enough when unknown.
 
   time)
 
-- 
2.34.1

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> To try to lessen the confusion I installed the attached, which fixes
> the reported bug so I'll close the bug report. Please feel free to
> revert if you see a problem with it (I'm just trying to save time here
> by being bold).

Thanks; I think that looks like the correct thing here.

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

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Maxim Nikulin]

On 28/05/2022 17:41, Lars Ingebrigtsen wrote:
> Paul Eggert writes:
> 
>> To try to lessen the confusion I installed the attached, which fixes
>> the reported bug so I'll close the bug report. Please feel free to
>> revert if you see a problem with it (I'm just trying to save time here
>> by being bold).
> 
> Thanks; I think that looks like the correct thing here.

Thank you, with the committed changes my examples work as I expect.

Paul, do you have any comment concerning the last part of the bug report?

`decoded-time-add' docstring:
 >   (decoded-time-add (decode-time) (make-decoded-time :month 2))

I think, it is confusing that `make-decoded-time' is used to create 
timestamps *and* time intervals. They are different types, for example 
sum of intervals is meaningful (despite may be ambiguous) while there is 
no point to add timestamps. Daylight saving time and timezone are 
something alien for intervals. Though I am unsure if it is reasonable to 
mark intervals by e.g. 'time-interval symbol or to make these types 
distinct by some other way.

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Eli Zaretskii]

> Cc: 55635-done@debbugs.gnu.org
> From: Maxim Nikulin <m.a.nikulin@gmail.com>
> Date: Sat, 28 May 2022 23:31:43 +0700
> 
> I think, it is confusing that `make-decoded-time' is used to create 
> timestamps *and* time intervals. They are different types, for example 
> sum of intervals is meaningful (despite may be ambiguous) while there is 
> no point to add timestamps.

But this situation already exists with time units anyway.  You can add
an hour to some other time, but there's also a valid time stamp that
expresses 1 hour past the epoch UTC, and their values are exactly
identical.

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Paul Eggert]

On 5/28/22 09:53, Eli Zaretskii wrote:
> this situation already exists with time units anyway.  You can add
> an hour to some other time, but there's also a valid time stamp that
> expresses 1 hour past the epoch UTC, and their values are exactly
> identical.

Quite true for encoded times that count seconds. However, Max also has a 
point that decoded timestamps like (19 17 10 28 5 2022 6 t -25200) are 
problematic as relative times. Although their first six elements can be 
treated as either relative or absolute, their last three elements don't 
make much sense in relative times.

The recent change in the master branch that lets encode-time take 
six-element lists suggests that perhaps a better way to represent a 
relative decoded time would be as a 6-element list.

With that in mind, here are three suggestions.

1. decoded-time-dst should return -1, instead of nil, when given a 
6-element list, since nil means standard time and -1 means DST is unknown.

2. make-decoded-time should generate a six-element list unless given a 
DST or ZONE arg.

3. Document the above nicely. (This is the hardest part....)

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Lars Ingebrigtsen]

Paul Eggert <eggert@cs.ucla.edu> writes:

> With that in mind, here are three suggestions.
>
> 1. decoded-time-dst should return -1, instead of nil, when given a
> 6-element list, since nil means standard time and -1 means DST is
> unknown.

I think that sounds correct.

> 2. make-decoded-time should generate a six-element list unless given a
> DST or ZONE arg.

I don't think we should do this.  Yes, the remaining elements are
nonsensical when talking about intervals, but people rely on that
function to return its documented value.  If somebody wants to do
interval calculations and passes in a DST to make-decoded-time, that's a
classic "well, don't do that" situation.

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

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Paul Eggert]

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

On 5/29/22 06:10, Lars Ingebrigtsen wrote:
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
>> With that in mind, here are three suggestions.
>>
>> 1. decoded-time-dst should return -1, instead of nil, when given a
>> 6-element list, since nil means standard time and -1 means DST is
>> unknown.
> 
> I think that sounds correct.

I looked into that, and since decoded-time-dst is defined by 
cl-defstruct it's not easy to change how it works. For now, I just 
documented the glitch; see the 3rd attached patch.

I also installed the 1st attached patch which mentions that Emacs's 
decoded dst flag differs from that of Common Lisp, and the 2nd attached 
patch which makes iso8601 parsing compatible with what we've discussed 
here by having it return -1 for unknown DST flags.


>> > 2. make-decoded-time should generate a six-element list unless given a
>> > DST or ZONE arg.
> 
> I don't think we should do this.

OK, I left that alone.

[-- Attachment #2: 0001-Doc-fix-for-dst-flag.patch --]
[-- Type: text/x-patch, Size: 982 bytes --]

From 01e814b6eee70d7ba63b4bc2b183e43b900ed187 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 29 May 2022 08:52:13 -0700
Subject: [PATCH 1/3] Doc fix for dst flag

* doc/lispref/os.texi (Time Conversion): Note Common Lisp dst differs.
---
 doc/lispref/os.texi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/doc/lispref/os.texi b/doc/lispref/os.texi
index f4dd2e7072..11a0d02338 100644
--- a/doc/lispref/os.texi
+++ b/doc/lispref/os.texi
@@ -1649,8 +1649,8 @@ Time Conversion
 a particular form should specify @var{form}.
 
 @strong{Common Lisp Note:} Common Lisp has different meanings for
-@var{dow} and @var{utcoff}, and its @var{second} is an integer between
-0 and 59 inclusive.
+@var{dow}, @code{dst} and @var{utcoff}, and its @var{second} is an
+integer between 0 and 59 inclusive.
 
 To access (or alter) the elements in the calendrical information, the
 @code{decoded-time-second}, @code{decoded-time-minute},
-- 
2.34.1


[-- Attachment #3: 0002-ISO-8601-strings-sans-Z-don-t-specify-DST-flag.patch --]
[-- Type: text/x-patch, Size: 5043 bytes --]

From 477302d490adf3e7c4c1d772aaba7dbdd454b60a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 29 May 2022 13:07:50 -0700
Subject: [PATCH 2/3] =?UTF-8?q?ISO=208601=20strings=20sans=20"Z"=20don?=
 =?UTF-8?q?=E2=80=99t=20specify=20DST=20flag?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/calendar/iso8601.el (iso8601--zone-dst): New function.
(iso8601-parse, iso8601-parse-time): Use it.
(iso8601--decoded-time): Default dst to -1, not nil.
* test/lisp/calendar/iso8601-tests.el (test-iso8601-combined)
(standard-test-time-of-day-zone): Adjust to new behavior.
---
 lisp/calendar/iso8601.el            | 12 +++++++++---
 test/lisp/calendar/iso8601-tests.el | 20 ++++++++++----------
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/lisp/calendar/iso8601.el b/lisp/calendar/iso8601.el
index e31120f52f..6827a957a6 100644
--- a/lisp/calendar/iso8601.el
+++ b/lisp/calendar/iso8601.el
@@ -114,6 +114,11 @@ iso8601--duration-match
          iso8601--duration-week-match
          iso8601--duration-combined-match)))
 
+;; "Z" dnd "z" are standard time; nil and [-+][0-9][0-9]... are local time
+;; with unknown DST.
+(defun iso8601--zone-dst (zone)
+  (if (= (length zone) 1) nil -1))
+
 (defun iso8601-parse (string &optional form)
   "Parse an ISO 8601 date/time string and return a `decode-time' structure.
 
@@ -140,7 +145,7 @@ iso8601-parse
         (setf (decoded-time-zone date)
               ;; The time zone in decoded times are in seconds.
 	      (* (iso8601-parse-zone zone-string) 60))
-	(setf (decoded-time-dst date) nil))
+	(setf (decoded-time-dst date) (iso8601--zone-dst zone-string)))
       date)))
 
 (defun iso8601-parse-date (string)
@@ -256,6 +261,7 @@ iso8601-parse-time
           (iso8601--decoded-time :hour hour
                                  :minute (or minute 0)
                                  :second (or second 0)
+				 :dst (iso8601--zone-dst zone)
                                  :zone (and zone
                                             (* 60 (iso8601-parse-zone
                                                    zone)))))))))
@@ -364,7 +370,7 @@ iso8601--value
 
 (cl-defun iso8601--decoded-time (&key second minute hour
                                       day month year
-                                      dst zone)
+				      (dst -1) zone)
   (list (iso8601--value second)
         (iso8601--value minute)
         (iso8601--value hour)
@@ -372,7 +378,7 @@ iso8601--decoded-time
         (iso8601--value month)
         (iso8601--value year)
         nil
-	(if (or dst zone) dst -1)
+	dst
         zone))
 
 (defun iso8601--encode-time (time)
diff --git a/test/lisp/calendar/iso8601-tests.el b/test/lisp/calendar/iso8601-tests.el
index 6c9e85ec92..f64c498c02 100644
--- a/test/lisp/calendar/iso8601-tests.el
+++ b/test/lisp/calendar/iso8601-tests.el
@@ -82,9 +82,9 @@ test-iso8601-combined
   (should (equal (iso8601-parse "2008-03-02T13:47:30Z")
                  '(30 47 13 2 3 2008 nil nil 0)))
   (should (equal (iso8601-parse "2008-03-02T13:47:30+01:00")
-                 '(30 47 13 2 3 2008 nil nil 3600)))
+                 '(30 47 13 2 3 2008 nil -1 3600)))
   (should (equal (iso8601-parse "2008-03-02T13:47:30-01")
-                 '(30 47 13 2 3 2008 nil nil -3600))))
+                 '(30 47 13 2 3 2008 nil -1 -3600))))
 
 (ert-deftest test-iso8601-duration ()
   (should (equal (iso8601-parse-duration "P3Y6M4DT12H30M5S")
@@ -221,24 +221,24 @@ standard-test-time-of-day-utc
 
 (ert-deftest standard-test-time-of-day-zone ()
   (should (equal (iso8601-parse-time "152746+0100")
-                 '(46 27 15 nil nil nil nil nil 3600)))
+                 '(46 27 15 nil nil nil nil -1 3600)))
   (should (equal (iso8601-parse-time "15:27:46+0100")
-                 '(46 27 15 nil nil nil nil nil 3600)))
+                 '(46 27 15 nil nil nil nil -1 3600)))
 
   (should (equal (iso8601-parse-time "152746+01")
-                 '(46 27 15 nil nil nil nil nil 3600)))
+                 '(46 27 15 nil nil nil nil -1 3600)))
   (should (equal (iso8601-parse-time "15:27:46+01")
-                 '(46 27 15 nil nil nil nil nil 3600)))
+                 '(46 27 15 nil nil nil nil -1 3600)))
 
   (should (equal (iso8601-parse-time "152746-0500")
-                 '(46 27 15 nil nil nil nil nil -18000)))
+                 '(46 27 15 nil nil nil nil -1 -18000)))
   (should (equal (iso8601-parse-time "15:27:46-0500")
-                 '(46 27 15 nil nil nil nil nil -18000)))
+                 '(46 27 15 nil nil nil nil -1 -18000)))
 
   (should (equal (iso8601-parse-time "152746-05")
-                 '(46 27 15 nil nil nil nil nil -18000)))
+                 '(46 27 15 nil nil nil nil -1 -18000)))
   (should (equal (iso8601-parse-time "15:27:46-05")
-                 '(46 27 15 nil nil nil nil nil -18000))))
+                 '(46 27 15 nil nil nil nil -1 -18000))))
 
 
 (defun test-iso8601-format-time-string-zone-round-trip (offset-minutes z-format)
-- 
2.34.1


[-- Attachment #4: 0003-Document-decoded-time-string-issue-on-6-elt-args.patch --]
[-- Type: text/x-patch, Size: 1151 bytes --]

From 63da939a7afac05775f6c8183d59ab8f78ce0f01 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 29 May 2022 14:57:48 -0700
Subject: [PATCH 3/3] Document decoded-time-string issue on 6-elt args

* lisp/simple.el: Document problematic use of decoded-time-dst on
6-element args.
---
 lisp/simple.el | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lisp/simple.el b/lisp/simple.el
index a254ee2251..d6b7045432 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10519,6 +10519,14 @@ capitalize-dwim
 the number of seconds east of Greenwich.")
   )
 
+;; Document that decoded-time-dst is problematic on 6-element lists.
+;; It should return -1 indicating unknown DST, but currently returns
+;; nil indicating standard time.
+(put 'decoded-time-dst 'function-documentation
+     (append (get 'decoded-time-dst 'function-documentation)
+             "As a special case, `decoded-time-dst' returns an unspecified
+value when given a list too short to have a dst element."))
+
 (defun get-scratch-buffer-create ()
   "Return the *scratch* buffer, creating a new one if needed."
   (or (get-buffer "*scratch*")
-- 
2.34.1

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Maxim Nikulin]

On 30/05/2022 05:04, Paul Eggert wrote:
> diff --git a/lisp/simple.el b/lisp/simple.el
> index a254ee2251..d6b7045432 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -10519,6 +10519,14 @@ capitalize-dwim
>  the number of seconds east of Greenwich.")
>    )
>  
> +;; Document that decoded-time-dst is problematic on 6-element lists.
> +;; It should return -1 indicating unknown DST, but currently returns
> +;; nil indicating standard time.
> +(put 'decoded-time-dst 'function-documentation
> +     (append (get 'decoded-time-dst 'function-documentation)
> +             "As a special case, `decoded-time-dst' returns an unspecified
> +value when given a list too short to have a dst element."))
> +

Paul, thank you for your efforts to fix the issues.

I have never used `cl-defstruct' before (and frankly speaking I am 
rather confused that the `decoded-time' struct and its constructor 
`make-decoded-time' are defined in different files and even directories 
are not the same), so my question may be naïve. Why did not you just add 
this new sentence to the :documentation property of the DST slot a bit 
above?

By the way, after updating of `make-decoded-time', default value for DST 
should be updated in `cl-defstruct' as well, otherwise
     (describe-symbol 'decoded-time)
reports that the default is nil.

It may be reasonable to cross-link `decoded-time' and 
`make-decoded-time' in docstrings.

Concerning timestamp vs. interval, first of all, I do not request 
immediate changes. I raised the question to make developers aware that 
the problem exist and it should be taken into account during further 
modifications or implementation of new features.

Lars Ingebrigtsen, Sun, 29 May 2022 15:10:19 +0200.
> If somebody wants to do
> interval calculations and passes in a DST to make-decoded-time, that's a
> classic "well, don't do that" situation.

DST slot with -1 (default) value is rather confusing for intervals but 
it safer for timestamps. Certainly it possible to do anything with bytes 
in memory, but direction of programming languages and libraries 
development is to allow users to clearly express intentions code and to 
add some measures that prevents bugs. That is why I mentioned tagged 
structure for interval type to distinguish it from timestamps.

Eli Zaretskii, Sat, 28 May 2022 19:53:47 +0300. >> From: Maxim Nikulin 
Date: Sat, 28 May 2022 23:31:43 +0700
>>
>> I think, it is confusing that `make-decoded-time' is used to create 
>> timestamps *and* time intervals. They are different types, for example 
>> sum of intervals is meaningful (despite may be ambiguous) while there is 
>> no point to add timestamps.
> 
> But this situation already exists with time units anyway.  You can add
> an hour to some other time, but there's also a valid time stamp that
> expresses 1 hour past the epoch UTC, and their values are exactly
> identical.

Certainly timestamps are actually intervals with various reference 
points. Encoded time is counted from 1970, decoded time from 0 a.d., so 
trying to add the same timestamps you will get result depending on their 
representation. Decoded time in some cases is more convenient since 1 
day may be not the same as 24 hours, not to mention varying duration of 
1 month.

The problem is that `decoded-time' time have field that are alien for 
intervals (timezone). Using the same constructor for both types makes 
code more obscure, it is impossible to enforce particular type of 
function argument to catch a programming error.

It is possible to use the same type for timestamps and intervals 
further, I am trying to dispute that it is the best design choice.

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Paul Eggert]

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

On 5/31/22 05:25, Maxim Nikulin wrote:

> I have never used `cl-defstruct' before (and frankly speaking I am 
> rather confused that the `decoded-time' struct and its constructor 
> `make-decoded-time' are defined in different files and even directories 
> are not the same), so my question may be naïve. Why did not you just add 
> this new sentence to the :documentation property of the DST slot a bit 
> above?

I am not sure I understand the question. The slot itself has a specified 
value (t, nil, or -1) whereas the decoded-time-dst function returns an 
unspecified value when there is no slot.

> By the way, after updating of `make-decoded-time', default value for DST 
> should be updated in `cl-defstruct' as well, otherwise
>      (describe-symbol 'decoded-time)
> reports that the default is nil.
> 
> It may be reasonable to cross-link `decoded-time' and 
> `make-decoded-time' in docstrings.

Thanks for the suggestions; I installed the attached.

[-- Attachment #2: 0001-Default-decoded-time-dst-slot-to-1.patch --]
[-- Type: text/x-patch, Size: 2667 bytes --]

From 5678829a62752eb332caef3abebeb64cb0722708 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 13 Jun 2022 14:25:58 -0700
Subject: [PATCH] Default decoded-time dst slot to -1

* lisp/simple.el (decoded-time): Default dst slot to -1.
Improve related doc strings.
---
 lisp/calendar/time-date.el |  3 ++-
 lisp/simple.el             | 16 ++++++++++------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/lisp/calendar/time-date.el b/lisp/calendar/time-date.el
index 40374c3bb4..d19134db83 100644
--- a/lisp/calendar/time-date.el
+++ b/lisp/calendar/time-date.el
@@ -557,7 +557,8 @@ make-decoded-time
   (list second minute hour day month year nil dst zone))
 
 (defun decoded-time-set-defaults (time &optional default-zone)
-  "Set any nil values in `decoded-time' TIME to default values.
+  "Set most nil values in `decoded-time' TIME to default values.
+This can set TIME's year, month, day, hour, minute and second.
 The default value is based on January 1st, 1970 at midnight.
 This year is used to guarantee portability; see Info
 node `(elisp) Time of Day'.
diff --git a/lisp/simple.el b/lisp/simple.el
index f6932339c9..05a0855a96 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10511,10 +10511,10 @@ capitalize-dwim
   (year nil :documentation "This is a four digit integer.")
   (weekday nil :documentation "\
 This is a number between 0 and 6, and 0 is Sunday.")
-  (dst nil :documentation "\
+  (dst -1 :documentation "\
 This is t if daylight saving time is in effect, nil if it is not
-in effect, and -1 if daylight saving information is not
-available.")
+in effect, and -1 if daylight saving information is not available.
+Also see `decoded-time-dst'.")
   (zone nil :documentation "\
 This is an integer indicating the UTC offset in seconds, i.e.,
 the number of seconds east of Greenwich.")
@@ -10524,9 +10524,13 @@ capitalize-dwim
 ;; It should return -1 indicating unknown DST, but currently returns
 ;; nil indicating standard time.
 (put 'decoded-time-dst 'function-documentation
-     (append (get 'decoded-time-dst 'function-documentation)
-             "As a special case, `decoded-time-dst' returns an unspecified
-value when given a list too short to have a dst element."))
+     "Access slot \"dst\" of `decoded-time' struct CL-X.
+This is t if daylight saving time is in effect, nil if it is not
+in effect, and -1 if daylight saving information is not available.
+As a special case, return an unspecified value when given a list
+too short to have a dst element.
+
+(fn CL-X)")
 
 (defun get-scratch-buffer-create ()
   "Return the *scratch* buffer, creating a new one if needed."
-- 
2.34.1

bug#55635: `make-decoded-time' incorrectly sets DST to nil, it should be -1 (guess)

[Maxim Nikulin]

On 14/06/2022 04:30, Paul Eggert wrote:
> On 5/31/22 05:25, Maxim Nikulin wrote:
> 
>> I have never used `cl-defstruct' before (and frankly speaking I am 
>> rather confused that the `decoded-time' struct and its constructor 
>> `make-decoded-time' are defined in different files and even 
>> directories are not the same), so my question may be naïve. Why did 
>> not you just add this new sentence to the :documentation property of 
>> the DST slot a bit above?
> 
> I am not sure I understand the question. The slot itself has a specified 
> value (t, nil, or -1) whereas the decoded-time-dst function returns an 
> unspecified value when there is no slot.

See the diff at the end of this message. Maybe I do not see any 
difference because I tried such definitions and (describe-function 
`decoded-time-dst), (describe-symbol 'decoded-time) in Emacs-27, not for 
the development version.

>> It may be reasonable to cross-link `decoded-time' and 
>> `make-decoded-time' in docstrings.
> 
> Thanks for the suggestions; I installed the attached.

First of all, I am sorry, for some reason I missed that 
`make-decoded-time' already has "`decoded-time'" in its docstring. Maybe 
I am not familiar with help system enough, but my idea was to add a 
docstring to `cl-defstruct' as a whole (currently only slots are 
documented) that has "`make-decoded-time'" reference in the opposite 
direction to existing one. I admit that I missed something else and it 
is not necessary.

diff --git a/lisp/simple.el b/lisp/simple.el
index 99c951b24b..3054d79d44 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -10511,27 +10511,19 @@ This is an integer between 1 and 12 
(inclusive).  January is 1.")
    (year nil :documentation "This is a four digit integer.")
    (weekday nil :documentation "\
  This is a number between 0 and 6, and 0 is Sunday.")
+  ;; decoded-time-dst is problematic on 6-element lists.
+  ;; It should return -1 indicating unknown DST, but currently returns
+  ;; nil indicating standard time.
    (dst -1 :documentation "\
  This is t if daylight saving time is in effect, nil if it is not
  in effect, and -1 if daylight saving information is not available.
-Also see `decoded-time-dst'.")
+As a special case, return an unspecified value when given a list
+too short to have a dst element.")
    (zone nil :documentation "\
  This is an integer indicating the UTC offset in seconds, i.e.,
  the number of seconds east of Greenwich.")
    )

-;; Document that decoded-time-dst is problematic on 6-element lists.
-;; It should return -1 indicating unknown DST, but currently returns
-;; nil indicating standard time.
-(put 'decoded-time-dst 'function-documentation
-     "Access slot \"dst\" of `decoded-time' struct CL-X.
-This is t if daylight saving time is in effect, nil if it is not
-in effect, and -1 if daylight saving information is not available.
-As a special case, return an unspecified value when given a list
-too short to have a dst element.
-
-(fn CL-X)")
-
  (defun get-scratch-buffer-create ()
    "Return the *scratch* buffer, creating a new one if needed."
    (or (get-buffer "*scratch*")