bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Gustavo Barros]

Hi All,

I've reported this some time ago at the Org bugtracker
(https://list.orgmode.org/878rtv9kx6.fsf@gmail.com/T/#u) and, since I
was testing the pre-release this week and the issue is still around, I
reiterated it.  But I was asked to report it to the Emacs bug tracker
instead, thus this report.

I met a particularly strange issue related to Org Agenda's
`mode-name'.  And one space in particular, the one that is added
before `org-agenda-current-span'.  The `mode-name' for the Agenda is
set by `org-agenda-set-mode-name', and inside it we find:

#+begin_src emacs-lisp
" "
'(:eval (org-agenda-span-name org-agenda-current-span))
#+end_src

Now, this space somehow gets propertized.

A recipe for it.  Start `emacs -Q'.  Set things up:

#+begin_src emacs-lisp
(setq org-agenda-files '("~/agenda.org"))
(setq eval-expression-print-level nil)
(setq eval-expression-print-length nil)
#+end_src

Let's say =agenda.org= contains:

#+begin_src org
,* TODO Task
SCHEDULED: <2023-04-14 Fri>
#+end_src

Call `M-x org-agenda RET a'.  Now examine `mode-name' with `M-:
mode-name RET' to get:

#+begin_src emacs-lisp
("Org-Agenda" "" #(" " 0 1 (org-category "agenda" tags nil
org-priority-highest 65 org-priority-lowest 67 time-of-day nil
duration nil breadcrumbs nil txt #("TODO Task" 0 9 (org-heading t
effort-minutes nil effort nil fontified nil)) level " " time "" extra
"Scheduled: " format (((org-prefix-has-time t) (org-prefix-has-tag
nil) (org-prefix-category-length 12) (org-prefix-has-effort nil)
(org-prefix-has-breadcrumbs nil)) (format " %s %s%s%s" (format "%s"
(if (member category-icon '("" nil)) "" (concat category-icon ""
(get-text-property 0 'extra-space category-icon)))) (format "%-12s"
(if (member category '("" nil)) "" (concat category ":"
(get-text-property 0 'extra-space category)))) (if (member time '(""
nil)) "" (format "%-12s" (concat time ""))) (format "%s" (if (member
extra '("" nil)) "" (concat extra " " (get-text-property 0
'extra-space extra)))))) dotime time org-not-done-regexp "\\(TODO\\)"
org-todo-regexp "\\(DONE\\|TODO\\)" org-complex-heading-regexp
"^\\(\\*+\\)\\(?: +\\(DONE\\|TODO\\)\\)?\\(?: +\\(\\[#.\\]\\)\\)?\\(?:
+\\(.*?\\)\\)??\\(?:[     ]+\\(:[[:alnum:]_@#%:]+:\\)\\)?[     ]*$"
done-face org-agenda-done mouse-face highlight help-echo "mouse-2 or
RET jump to Org file ~/agenda.org" undone-face org-scheduled-today
face org-scheduled-today org-marker #<marker (moves after insertion)
at 24 in agenda.org> org-hd-marker #<marker (moves after insertion) at
1 in agenda.org> type "scheduled" date (4 14 2023) ts-date 738624
warntime nil effort nil effort-minutes nil priority 1099 org-habit-p
nil todo-state #("TODO" 0 4 (fontified nil)))) (:eval
(org-agenda-span-name org-agenda-current-span)) "" "" "" " Ddl" "
Grid" "" "" "" "" "" "" "" "" "")
#+end_src

So, it appears that the Org Agenda buffer's properties are somehow
getting to that particular space in `mode-name'.  It completely beats
me how it is so but, alas, it is there.

This is a problem because, depending on what the content of your
agenda is, this might result in this space getting some visually
distinctive property.  In my case, I get a blank gap in the mode-line
at this point.  I couldn't generate a simple ECM that gets this
effect.  But, at this point, it should be clear it can happen, given
these properties are there.

Best regards,
Gustavo.




In GNU Emacs 29.0.90 (build 2, x86_64-pc-linux-gnu, GTK+ Version
 3.24.33, cairo version 1.16.0) of 2023-04-10 built on gusbrs-desktop
Windowing system distributor 'The X.Org Foundation', version 11.0.12101003
System Description: Linux Mint 21.1

Configured using:
 'configure --with-mailutils --with-xwidgets --with-native-compilation
 --without-compress-install'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES
NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3
THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER WEBP X11 XDBE XIM XINPUT2
XPM XWIDGETS GTK3 ZLIB

Important settings:
  value of $LC_MONETARY: pt_BR.UTF-8
  value of $LC_NUMERIC: pt_BR.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Messages

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  buffer-read-only: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug diary-lib diary-loaddefs cal-iso
oc-basic ol-eww eww url-queue thingatpt mm-url ol-rmail ol-mhe ol-irc
ol-info ol-gnus nnselect gnus-art mm-uu mml2015 mm-view mml-smime smime
gnutls dig gnus-sum shr pixel-fill kinsoku url-file svg dom browse-url
url url-proxy url-privacy url-expand url-methods url-history url-cookie
generate-lisp-file url-domsuf url-util url-parse auth-source eieio
eieio-core json map byte-opt url-vars gnus-group gnus-undo gnus-start
gnus-dbus dbus xml gnus-cloud nnimap nnmail mail-source utf7 nnoo
parse-time gnus-spec gnus-int gnus-range message sendmail mailcap
yank-media puny rfc822 mml mml-sec password-cache epa derived epg
rfc6068 epg-config mm-decode mm-bodies mm-encode mail-parse rfc2231
rfc2047 rfc2045 ietf-drums mailabbrev gmm-utils mailheader gnus-win gnus
nnheader gnus-util text-property-search mail-utils range mm-util
mail-prsvr wid-edit ol-docview doc-view filenotify jka-compr image-mode
exif dired dired-loaddefs ol-bibtex bibtex iso8601 ol-bbdb ol-w3m ol-doi
org-link-doi face-remap org-agenda org-element org-persist xdg org-id
avl-tree generator org-refile org ob ob-tangle ob-ref ob-lob ob-table
ob-exp org-macro org-src ob-comint org-pcomplete pcomplete comint
ansi-osc ansi-color ring org-list org-footnote org-faces org-entities
noutline outline ob-emacs-lisp ob-core ob-eval org-cycle org-table
org-keys oc org-loaddefs find-func cal-menu calendar cal-loaddefs ol
org-fold org-fold-core org-compat org-version org-macs format-spec
time-date cl-loaddefs comp comp-cstr warnings icons subr-x rx cl-seq
cl-macs gv cl-extra help-mode bytecomp byte-compile cl-lib rmc
iso-transl tooltip cconv eldoc paren electric uniquify ediff-hook
vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads xwidget-internal dbusbind inotify lcms2 dynamic-setting
system-font-setting font-render-setting cairo move-toolbar gtk x-toolkit
xinput2 x multi-tty make-network-process native-compile emacs)

Memory information:
((conses 16 258488 14637)
 (symbols 48 21279 0)
 (strings 32 72219 2491)
 (string-bytes 1 2252850)
 (vectors 16 39079)
 (vector-slots 8 711251 27711)
 (floats 8 337 81)
 (intervals 56 389 0)
 (buffers 984 13))

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Gustavo Barros <gusbrs.2016@gmail.com> writes:

> Call `M-x org-agenda RET a'.  Now examine `mode-name' with `M-:
> mode-name RET' to get:
>
> #+begin_src emacs-lisp
> ("Org-Agenda" "" #(" " 0 1 (org-category "agenda" tags nil...

I suspect that it is Emacs-related because I see nothing wrong done on
Org side.

The code setting `mode-name' is

(defun org-agenda-set-mode-name ()
  "Set the mode name to indicate all the small mode settings."
  (setq mode-name
	(list "Org-Agenda"
	      (if (get 'org-agenda-files 'org-restrict) " []" "")
	      " "
	      '(:eval (org-agenda-span-name org-agenda-current-span))
              ...)))

Note the third " " in `mode-name' list.
Org does not modify the mode-name by side effect.

This bug is only reproducible when using built-in Org. When I tried to
reproduce with Git version of Org (the same release tag), no extra
properties are present in `mode-name'.

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> Cc: 62847@debbugs.gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Sat, 15 Apr 2023 09:38:43 +0000
> 
> Gustavo Barros <gusbrs.2016@gmail.com> writes:
> 
> > Call `M-x org-agenda RET a'.  Now examine `mode-name' with `M-:
> > mode-name RET' to get:
> >
> > #+begin_src emacs-lisp
> > ("Org-Agenda" "" #(" " 0 1 (org-category "agenda" tags nil...
> 
> I suspect that it is Emacs-related because I see nothing wrong done on
> Org side.
> 
> The code setting `mode-name' is
> 
> (defun org-agenda-set-mode-name ()
>   "Set the mode name to indicate all the small mode settings."
>   (setq mode-name
> 	(list "Org-Agenda"
> 	      (if (get 'org-agenda-files 'org-restrict) " []" "")
> 	      " "
> 	      '(:eval (org-agenda-span-name org-agenda-current-span))
>               ...)))
> 
> Note the third " " in `mode-name' list.
> Org does not modify the mode-name by side effect.
> 
> This bug is only reproducible when using built-in Org. When I tried to
> reproduce with Git version of Org (the same release tag), no extra
> properties are present in `mode-name'.

I'm afraid I don't understand this bug report, and cannot reproduce
what I thought I understood from it.  Any chance of presenting a
recipe starting from "emacs -Q", for someone who doesn't use
org-agenda?

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm afraid I don't understand this bug report, and cannot reproduce
> what I thought I understood from it.  Any chance of presenting a
> recipe starting from "emacs -Q", for someone who doesn't use
> org-agenda?

1. emacs -Q
2. Create and open a new Org file with the following contents:

* TODO Task
SCHEDULED: <2023-04-14 Fri>

3. M-x org-agenda <RET> < a
4. M-: mode-name <RET>
5. Observe the third element (" ") in the list propertized unexpectedly,
despite `org-agenda-set-mode-name' using " " without properties.

I suspect some compiled shared " " object, but it should not happen with
strings, AFAIK. And it does not happen when I compile the same Org
version from Git.

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: gusbrs.2016@gmail.com, 62847@debbugs.gnu.org
> Date: Sat, 15 Apr 2023 10:02:34 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'm afraid I don't understand this bug report, and cannot reproduce
> > what I thought I understood from it.  Any chance of presenting a
> > recipe starting from "emacs -Q", for someone who doesn't use
> > org-agenda?
> 
> 1. emacs -Q
> 2. Create and open a new Org file with the following contents:
> 
> * TODO Task
> SCHEDULED: <2023-04-14 Fri>
> 
> 3. M-x org-agenda <RET> < a
> 4. M-: mode-name <RET>
> 5. Observe the third element (" ") in the list propertized unexpectedly,
> despite `org-agenda-set-mode-name' using " " without properties.

Thanks, but it isn't propertized here, I see a literal " " with no
properties.

What happens if you display mode-name inside org-agenda-set-mode-name:
does it have the properties there already?

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, but it isn't propertized here, I see a literal " " with no
> properties.

Note: I tried with Emacs from Git - also cannot reproduce.
Only using installed version of Emacs 28, 29, and master. Not Emacs 27.

> What happens if you display mode-name inside org-agenda-set-mode-name:
> does it have the properties there already?

First time, it does not. From the second agenda rebuild onward, the
properties are there.

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: gusbrs.2016@gmail.com, 62847@debbugs.gnu.org
> Date: Sat, 15 Apr 2023 10:40:36 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks, but it isn't propertized here, I see a literal " " with no
> > properties.
> 
> Note: I tried with Emacs from Git - also cannot reproduce.

On the emacs-29 branch as well?  That's what I tried.

> Only using installed version of Emacs 28, 29, and master. Not Emacs 27.
> 
> > What happens if you display mode-name inside org-agenda-set-mode-name:
> > does it have the properties there already?
> 
> First time, it does not. From the second agenda rebuild onward, the
> properties are there.

I've now tried the installed Emacs 29.0.90, and I cannot reproduce
there, either.  Not even if I invoke org-agenda twice in a row.  So
this sounds like some weird heisenbug.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Gustavo Barros]

Hi Eli,

thanks for looking into this.

On Sat, 15 Apr 2023 at 07:55, Eli Zaretskii <eliz@gnu.org> wrote:

> So this sounds like some weird heisenbug.

I'm not sure it helps much, but I do have a functioning workaround,
which doesn't do much but may offer some hint on the source of the
problem. It basically "replaces" the space with a space in `mode-name`
with an `:after` advice:

    (defun gb/org-agenda-set-mode-name-advice ()
      (let ((before (butlast mode-name
                             (length (member " " mode-name))))
            (after (cdr (member " " mode-name))))
        (when (= (length before) 2)
          (setq mode-name (append before (list " ") after))
          (force-mode-line-update))))
    (advice-add 'org-agenda-set-mode-name
                :after #'gb/org-agenda-set-mode-name-advice)

I don't know why the space set in my init file is different from the
one set in `org-agenda.el`. But Ihor's insight that the problem only
happens on an installed version suggests a compilation related issue,
as he says.

Best,
Gustavo.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: gusbrs.2016@gmail.com, 62847@debbugs.gnu.org
> Date: Sat, 15 Apr 2023 10:40:36 +0000
> 
> > What happens if you display mode-name inside org-agenda-set-mode-name:
> > does it have the properties there already?
> 
> First time, it does not. From the second agenda rebuild onward, the
> properties are there.

So maybe replace " " with (copy-sequence " ").

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Thanks, but it isn't propertized here, I see a literal " " with no
>> > properties.
>> 
>> Note: I tried with Emacs from Git - also cannot reproduce.
>
> On the emacs-29 branch as well?  That's what I tried.

Same for me, when using Emacs git - origin/emacs-29 branch. Cannot
reproduce. Only the installed Emacs. I also tried using the same
./configure options. No luck.

> I've now tried the installed Emacs 29.0.90, and I cannot reproduce
> there, either.  Not even if I invoke org-agenda twice in a row.  So
> this sounds like some weird heisenbug.

Maybe someone who is using Linux Mint can try to reproduce? Or Gentoo.

Gustavo, what happens if you put (load "org-agenda.el") in your init.el?

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> First time, it does not. From the second agenda rebuild onward, the
>> properties are there.
>
> So maybe replace " " with (copy-sequence " ").

But that should not be necessary, right?

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: gusbrs.2016@gmail.com, 62847@debbugs.gnu.org
> Date: Sat, 15 Apr 2023 11:44:52 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So maybe replace " " with (copy-sequence " ").
> 
> But that should not be necessary, right?

I'll let the byte-compilation experts (CC'ed) comment on that.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Gustavo Barros]

On Sat, 15 Apr 2023 at 08:41, Ihor Radchenko <yantar92@posteo.net> wrote:

> Gustavo, what happens if you put (load "org-agenda.el") in your init.el?

Problem gone, the space is not propertized.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

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

Gustavo Barros <gusbrs.2016@gmail.com> writes:

> On Sat, 15 Apr 2023 at 08:41, Ihor Radchenko <yantar92@posteo.net> wrote:
>
>> Gustavo, what happens if you put (load "org-agenda.el") in your init.el?
>
> Problem gone, the space is not propertized.

So, it really looks like compilation problem.

I am now looking into Elisp manual

    2.9 Mutability
    
       When similar constants occur as parts of a program, the Lisp
    interpreter might save time or space by reusing existing constants or
    their components.  For example, ‘(eq "abc" "abc")’ returns ‘t’ if the
    interpreter creates only one instance of the string literal ‘"abc"’, and
    returns ‘nil’ if it creates two instances.  Lisp programs should be
    written so that they work regardless of whether this optimization is in
    use.

So, it should be a good idea to avoid setting text properties in string
constants in general.

See the attached patch.
Though I have no clue if this is enough to fix the bug...


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org-agenda.el-Try-not-to-modify-string-constant.patch --]
[-- Type: text/x-patch, Size: 2771 bytes --]

From 6e74e7746936a5584fce9c8539dd1ad96b37bb7e Mon Sep 17 00:00:00 2001
Message-Id: <6e74e7746936a5584fce9c8539dd1ad96b37bb7e.1681560403.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Sat, 15 Apr 2023 14:04:19 +0200
Subject: [PATCH] * lisp/org-agenda.el: Try not to modify string constants by
 side effect

(org-agenda-propertize-selected-todo-keywords):
(org-agenda-format-item):
(org-agenda-highlight-todo): Make sure that we use a fresh string
constant copy when adding properties.  This avoids race modifications
when compiler use shared object for several string constants in
org-agenda.

See Emacs bug#62847.

Reported-by: Gustavo Barros <gusbrs.2016@gmail.com>
Link: https://orgmode.org/list/CAM9ALR95F_ZHV2_WsqAz0-35-S2rwxbHqsA5VGftvq51Yz3ZAQ@mail.gmail.com
---
 lisp/org-agenda.el | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lisp/org-agenda.el b/lisp/org-agenda.el
index 2ec2f4c00..8e6b84362 100644
--- a/lisp/org-agenda.el
+++ b/lisp/org-agenda.el
@@ -4923,7 +4923,9 @@ (defun org-agenda-propertize-selected-todo-keywords (keywords)
   "Use `org-todo-keyword-faces' for the selected todo KEYWORDS."
   (concat
    (if (or (equal keywords "ALL") (not keywords))
-       (propertize "ALL" 'face 'org-agenda-structure-filter)
+       (propertize
+        (copy-sequence "ALL") ; Avoid modifying `eq' string constants.
+        'face 'org-agenda-structure-filter)
      (mapconcat
       (lambda (kw)
         (propertize kw 'face (list (org-get-todo-face kw) 'org-agenda-structure)))
@@ -7251,7 +7253,9 @@         (defvar level) (defvar tag) (defvar time))
 			     "")))
 	     (category-icon (org-agenda-get-category-icon category))
 	     (category-icon (if category-icon
-				(propertize " " 'display category-icon)
+				(propertize
+                                 (copy-sequence " ") ; Avoid modifying `eq' " ".
+                                 'display category-icon)
 			      ""))
 	     (effort (and (not (string= txt ""))
 			  (get-text-property 1 'effort txt)))
@@ -7724,8 +7728,10 @@ (defun org-agenda-highlight-todo (x)
                    (unless (string= org-agenda-todo-keyword-format "")
                      ;; Remove `display' property as the icon could leak
                      ;; on the white space.
-                     (org-add-props " " (org-plist-delete (text-properties-at 0 x)
-                                                          'display)))
+                     (org-add-props
+                         (copy-sequence " ") ; Avoid modifying `eq' " ".
+                         (org-plist-delete (text-properties-at 0 x)
+                                           'display)))
                    (substring x (match-end 3)))))))
       x)))
 
-- 
2.40.0


[-- Attachment #3: Type: text/plain, Size: 224 bytes --]


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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Mattias Engdegård]

15 apr. 2023 kl. 13.45 skrev Eli Zaretskii <eliz@gnu.org>:
>>> So maybe replace " " with (copy-sequence " ").
>> 
>> But that should not be necessary, right?

Ideally not -- setting properties on literal strings should indeed be avoided for a variety of reasons, one being that the byte compiler shares equal string literals:

(defun ff ()
  (list "abc" (let ((s "abc"))
                (put-text-property 0 3 'aa 'bb s)
                s)))

(ff)
 -> ("abc" #("abc" 0 3 (aa bb)))                 ; interpreted
 -> (#("abc" 0 3 (aa bb)) #("abc" 0 3 (aa bb)))  ; byte-compiled

`org-agenda-set-mode-name` uses the literal " " twice so if either is modified the other will appear to be, too. Where this setting of properties is done I have no idea

There is currently no automatic sharing of string literals between byte-code functions but this may change, and I've no idea what the native compiler is up to in this respect.

`propertize` is safe because it makes a copy of its string argument, so there shouldn't be any reason to copy that argument explicitly.

`org-add-props` calls `add-text-properties` and is clearly destructive.

Interpreted code is less affected by the problem because literals aren't shared throughout a function, but trouble can still occur:

(defun hh (x)
  (let ((s "abc"))
    (when x
      (put-text-property 0 3 'aa 'bb s))
    s))

(hh nil) -> "abc"
(hh t)   -> #("abc" 0 3 (aa bb))
(hh nil) -> #("abc" 0 3 (aa bb))
...

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

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

> So, it really looks like compilation problem.
>
> I am now looking into Elisp manual
>
>     2.9 Mutability
>     
>        When similar constants occur as parts of a program, the Lisp
>     interpreter might save time or space by reusing existing constants or
>     their components.  For example, ‘(eq "abc" "abc")’ returns ‘t’ if the
>     interpreter creates only one instance of the string literal ‘"abc"’, and
>     returns ‘nil’ if it creates two instances.  Lisp programs should be
>     written so that they work regardless of whether this optimization is in
>     use.
>
> So, it should be a good idea to avoid setting text properties in string
> constants in general.

Indeed, since it modifies the object, it's "undefined behavior" territory.

> -       (propertize "ALL" 'face 'org-agenda-structure-filter)
> +       (propertize
> +        (copy-sequence "ALL") ; Avoid modifying `eq' string constants.
> +        'face 'org-agenda-structure-filter)

`propertize` does not modify its string argument (it returns a new
string instead), so the `copy-sequence` here is a pure waste.

> -				(propertize " " 'display category-icon)
> +				(propertize
> +                                 (copy-sequence " ") ; Avoid modifying `eq' " ".
> +                                 'display category-icon)

Same here.

> -                     (org-add-props " " (org-plist-delete (text-properties-at 0 x)
> -                                                          'display)))
> +                     (org-add-props
> +                         (copy-sequence " ") ; Avoid modifying `eq' " ".
> +                         (org-plist-delete (text-properties-at 0 x)
> +                                           'display)))

This hunk fixes a real bug, OTOH.
Maybe you should use `(apply #'propertize` instead or `org-add-props`?


        Stefan

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> -                     (org-add-props " " (org-plist-delete (text-properties-at 0 x)
>> -                                                          'display)))
>> +                     (org-add-props
>> +                         (copy-sequence " ") ; Avoid modifying `eq' " ".
>> +                         (org-plist-delete (text-properties-at 0 x)
>> +                                           'display)))
>
> This hunk fixes a real bug, OTOH.
> Maybe you should use `(apply #'propertize` instead or `org-add-props`?

Indeed.
Done.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a8a95b6c2

Gustavo, AFAIU, your issue might be "fixed" if you use Org version from
ELPA. If not, you will need to wait until the next bugfix release or use
git version of Org.

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Mattias Engdegård <mattiase@acm.org> writes:

> 15 apr. 2023 kl. 13.45 skrev Eli Zaretskii <eliz@gnu.org>:
>>>> So maybe replace " " with (copy-sequence " ").
>>> 
>>> But that should not be necessary, right?
>
> Ideally not -- setting properties on literal strings should indeed be avoided for a variety of reasons, one being that the byte compiler shares equal string literals:

Thanks for the clarification!
I am wondering if there is any relevant warning raised by the byte
compiler when doing something like
 (let ((str " ")) (add-text-properties 0 1 '(foo bar) str))

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Gustavo Barros]

On Sun, 16 Apr 2023 at 08:21, Ihor Radchenko <yantar92@posteo.net> wrote:

> Indeed.
> Done.
> https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=a8a95b6c2
>
> Gustavo, AFAIU, your issue might be "fixed" if you use Org version from
> ELPA. If not, you will need to wait until the next bugfix release or use
> git version of Org.

I thank you (all) for looking into this and for the possible fix.
I've been opting for the built-in Org for some time to keep things
sane here, and I'll be happy to get it when it comes.
And since the commit went to the bugfix branch, it won't be too long.
Good reason to try the prerelease. :)

Best,
Gustavo.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Mattias Engdegård]

16 apr. 2023 kl. 13.29 skrev Ihor Radchenko <yantar92@posteo.net>:

> I am wondering if there is any relevant warning raised by the byte
> compiler when doing something like
> (let ((str " ")) (add-text-properties 0 1 '(foo bar) str))

No, but it wouldn't be very hard to add one, but it would miss a lot of code that has no idea whether it is working on a string literal or not.

Counter to my usual preference for static checks I'd prefer a dynamic warning in this case because it would be precise and the run-time cost would likely be acceptable.

That requires a 'literal' flag for string objects and there's no obvious space for one, but perhaps we can grab the LSB of the `intervals` pointer.

Even if such a check defaults to off, it could be enabled selectively to root out bugs like this one. I'll see what I can do.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Mattias Engdegård <mattiase@acm.org> writes:

> Counter to my usual preference for static checks I'd prefer a dynamic warning in this case because it would be precise and the run-time cost would likely be acceptable.
>
> That requires a 'literal' flag for string objects and there's no obvious space for one, but perhaps we can grab the LSB of the `intervals` pointer.

Side note: It might be interesting to have something similar to
`add-variable-watcher' and `debug-on-variable-change', but for mutable
objects being modified.

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Daniel Mendler]

Hello Mattias!

> That requires a 'literal' flag for string objects and there's no
obvious space  for one, but perhaps we can grab the LSB of the
`intervals` pointer.

There was a recent report where I pointed out a segfault for `aset` on
"literal strings" (symbol names which are C strings located in the
read-only memory of the process). If you consider adding such a literal
string flag or read-only check, could this please be added there too? I
am in favor of adding immutable strings to Elisp. This would certainly
open up optimization potential for the bytecode compiler, in addition to
guarding against bugs like the one being discussed here.

Daniel

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>,
>  gusbrs.2016@gmail.com, 62847@debbugs.gnu.org
> Date: Sun, 16 Apr 2023 12:17:31 +0000
> 
> Side note: It might be interesting to have something similar to
> `add-variable-watcher' and `debug-on-variable-change', but for mutable
> objects being modified.

Propertizing a string doesn't mutate the string itself, though.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> Side note: It might be interesting to have something similar to
>> `add-variable-watcher' and `debug-on-variable-change', but for mutable
>> objects being modified.
>
> Propertizing a string doesn't mutate the string itself, though.

You mean the char vector representing a string? But doesn't the real
string object also include the plist?

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: mattiase@acm.org, monnier@iro.umontreal.ca, gusbrs.2016@gmail.com,
>  62847@debbugs.gnu.org
> Date: Sun, 16 Apr 2023 13:14:51 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Side note: It might be interesting to have something similar to
> >> `add-variable-watcher' and `debug-on-variable-change', but for mutable
> >> objects being modified.
> >
> > Propertizing a string doesn't mutate the string itself, though.
> 
> You mean the char vector representing a string? But doesn't the real
> string object also include the plist?

It includes a pointer to the string's intervals, which is where the
properties are recorded.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Mattias Engdegård]

16 apr. 2023 kl. 14.02 skrev Mattias Engdegård <mattiase@acm.org>:

> Even if such a check defaults to off, it could be enabled selectively to root out bugs like this one. I'll see what I can do.

Here's a patch suitable for debugging (applies to master). It's not proposed for inclusion in Emacs.

Attempts to change properties of literal strings result in an error by default. You can also make it warn or do nothing.
The patch does not attempt to be very subtle or efficient about it but it should be sound.

If you find it hard to find where the problem is, attach a debugger and set a breakpoint on the function `string_literal_prop_change`.

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Ihor Radchenko]

Eli Zaretskii <eliz@gnu.org> writes:

>> You mean the char vector representing a string? But doesn't the real
>> string object also include the plist?
>
> It includes a pointer to the string's intervals, which is where the
> properties are recorded.

That's internal implementation detail.
From Elisp perspective, string vector + string plist constitute string
object. Or do I miss something?

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

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Mattias Engdegård]

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

> Here's a patch

And here's one that I actually attached. Sorry!


[-- Attachment #2: string-literal-prop-change.diff --]
[-- Type: application/octet-stream, Size: 6743 bytes --]

diff --git a/src/alloc.c b/src/alloc.c
index d09fc41dec6..616b4fa7a66 100644
--- a/src/alloc.c
+++ b/src/alloc.c
@@ -2606,6 +2606,9 @@ make_clear_multibyte_string (EMACS_INT nchars, EMACS_INT nbytes, bool clearit)
 
   s = allocate_string ();
   s->u.s.intervals = NULL;
+#ifdef CHECK_STRING_LITERALS
+  s->u.s.literal = false;
+#endif
   allocate_string_data (s, nchars, nbytes, clearit, false);
   XSETSTRING (string, s);
   string_chars_consed += nbytes;
diff --git a/src/lisp.h b/src/lisp.h
index 4e17e369312..30c0a001c34 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -1560,6 +1560,8 @@ CDR_SAFE (Lisp_Object c)
   return CONSP (c) ? XCDR (c) : Qnil;
 }
 
+#define CHECK_STRING_LITERALS 1
+
 /* In a string or vector, the sign bit of u.s.size is the gc mark bit.  */
 
 struct Lisp_String
@@ -1579,6 +1581,10 @@ CDR_SAFE (Lisp_Object c)
 
       INTERVAL intervals;	/* Text properties in this string.  */
       unsigned char *data;
+#ifdef CHECK_STRING_LITERALS
+      /* Whether this string originated as a string literal in Lisp code.  */
+      bool literal;
+#endif
     } s;
     struct Lisp_String *next;
     GCALIGNED_UNION_MEMBER
diff --git a/src/lread.c b/src/lread.c
index 273120315df..23d8a1489c1 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -1411,6 +1411,10 @@ DEFUN ("load", Fload, Sload, 1, 5, 0,
   specbind (Qlread_unescaped_character_literals, Qnil);
   record_unwind_protect (load_warn_unescaped_character_literals, file);
 
+#ifdef CHECK_STRING_LITERALS
+  specbind (Qlread_unescaped_character_literals, Qnil);
+#endif
+
   bool is_elc = suffix_p (found, ".elc");
   if (is_elc
       /* version = 1 means the file is empty, in which case we can
@@ -2230,6 +2234,10 @@ readevalloop (Lisp_Object readcharfun,
 	     ? Qnil : list1 (Qt)));
   specbind (Qmacroexp__dynvars, Vmacroexp__dynvars);
 
+#ifdef CHECK_STRING_LITERALS
+  specbind (Qread_string_literals, Qt);
+#endif
+
   /* Ensure sourcename is absolute, except whilst preloading.  */
   if (!will_dump_p ()
       && !NILP (sourcename) && !NILP (Ffile_name_absolute_p (sourcename)))
@@ -3163,6 +3171,10 @@ read_string_literal (Lisp_Object readcharfun)
   Lisp_Object obj = make_specified_string (read_buffer, nchars, p - read_buffer,
 					   (force_multibyte
 					    || (p - read_buffer != nchars)));
+#ifdef CHECK_STRING_LITERALS
+  if (!NILP (Vread_string_literals))
+    XSTRING (obj)->u.s.literal = true;
+#endif
   return unbind_to (count, obj);
 }
 
@@ -3363,7 +3375,14 @@ string_props_from_rev_list (Lisp_Object elems, Lisp_Object readcharfun)
 	invalid_syntax ("Invalid string property list", readcharfun);
       Lisp_Object plist = XCAR (tl);
       tl = XCDR (tl);
+#ifdef CHECK_STRING_LITERALS
+      bool lit = XSTRING (obj)->u.s.literal;
+      XSTRING (obj)->u.s.literal = false;
+#endif
       Fset_text_properties (beg, end, plist, obj);
+#ifdef CHECK_STRING_LITERALS
+      XSTRING (obj)->u.s.literal = lit;
+#endif
     }
   return obj;
 }
@@ -5452,6 +5471,21 @@ syms_of_lread (void)
 	       doc: /* Non-nil means read recursive structures using #N= and #N# syntax.  */);
   Vread_circle = Qt;
 
+#ifdef CHECK_STRING_LITERALS
+  DEFVAR_LISP ("read-string-literals", Vread_string_literals,
+	       doc: /* Non-nil means read string literals as literals.  */);
+  Vread_string_literals = Qnil;
+  DEFSYM (Qread_string_literals, "read-string-literals");
+  DEFVAR_LISP ("string-literal-property-change",
+	       Vstring_literal_property_change,
+	       doc: /* How to handle changes to properties in literal strings.
+If `error', raise an error.
+If `warn', emit a warning.
+If `nil', do nothing.  */);
+  Vstring_literal_property_change = Qerror;
+  DEFSYM (Qwarn, "warn");
+#endif
+
   DEFVAR_LISP ("load-path", Vload_path,
 	       doc: /* List of directories to search for files to load.
 Each element is a string (directory file name) or nil (meaning
diff --git a/src/textprop.c b/src/textprop.c
index f88fff19c59..2a57262ec67 100644
--- a/src/textprop.c
+++ b/src/textprop.c
@@ -1164,6 +1164,36 @@ DEFUN ("previous-single-property-change", Fprevious_single_property_change,
     return make_fixnum (previous->position + LENGTH (previous));
 }
 \f
+
+#ifdef CHECK_STRING_LITERALS
+__attribute__((noinline)) void
+string_literal_prop_change (Lisp_Object object);
+
+__attribute__((noinline)) void
+string_literal_prop_change (Lisp_Object object)
+{
+  Lisp_Object how = Vstring_literal_property_change;
+  if (EQ (how, Qerror))
+    error ("Attempt to modify properties of literal string \"%s\"",
+	   SSDATA (object));
+  else if (EQ (how, Qwarn))
+    call2 (intern ("warn"),
+	   build_string
+	   ("Attempt to modify properties of literal string %S"),
+	   object);
+}
+#endif
+
+static void
+check_string_literal_prop_change (Lisp_Object object)
+{
+#ifdef CHECK_STRING_LITERALS
+  if (STRINGP (object) && XSTRING (object)->u.s.literal
+      && SCHARS (object) > 0)
+    string_literal_prop_change (object);
+#endif
+}
+
 /* Used by add-text-properties and add-face-text-property. */
 
 static Lisp_Object
@@ -1184,6 +1214,8 @@ add_text_properties_1 (Lisp_Object start, Lisp_Object end,
 						      destructive));
     }
 
+  check_string_literal_prop_change (object);
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1399,6 +1431,15 @@ set_text_properties (Lisp_Object start, Lisp_Object end, Lisp_Object properties,
 					     object, coherent_change_p));
     }
 
+#ifdef CHECK_STRING_LITERALS
+  // Don't complain about removal of properties from a string without any.
+  if (STRINGP (object) && !(!string_intervals (object)
+			    && NILP (properties)
+			    && BASE_EQ (start, make_fixnum (0))
+			    && BASE_EQ (end, make_fixnum (SCHARS (object)))))
+    check_string_literal_prop_change (object);
+#endif
+
   INTERVAL i;
   bool first_time = true;
 
@@ -1483,6 +1524,8 @@ set_text_properties_1 (Lisp_Object start, Lisp_Object end,
       return;
     }
 
+  check_string_literal_prop_change (object);
+
   INTERVAL prev_changed = NULL;
   ptrdiff_t s = XFIXNUM (start);
   ptrdiff_t len = XFIXNUM (end) - s;
@@ -1578,6 +1621,8 @@ DEFUN ("remove-text-properties", Fremove_text_properties,
 						 object));
     }
 
+  check_string_literal_prop_change (object);
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;
@@ -1704,6 +1749,8 @@ DEFUN ("remove-list-of-text-properties", Fremove_list_of_text_properties,
 							 object));
     }
 
+  check_string_literal_prop_change (object);
+
   INTERVAL i, unchanged;
   ptrdiff_t s, len;
   bool modified = false;

bug#62847: 29.0.90; Propertized space in Org Agenda's mode-name

[Eli Zaretskii]

> From: Ihor Radchenko <yantar92@posteo.net>
> Cc: mattiase@acm.org, monnier@iro.umontreal.ca, gusbrs.2016@gmail.com,
>  62847@debbugs.gnu.org
> Date: Sun, 16 Apr 2023 14:52:02 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> You mean the char vector representing a string? But doesn't the real
> >> string object also include the plist?
> >
> > It includes a pointer to the string's intervals, which is where the
> > properties are recorded.
> 
> That's internal implementation detail.
> >From Elisp perspective, string vector + string plist constitute string
> object. Or do I miss something?

I don't necessarily agree.  We have equal-including-properties for a
reason.