GNU bug report logs - #40693
28.0.50; json-encode-alist changes alist

Previous Next

Package: emacs;

Reported by: Ivan Andrus <darthandrus <at> gmail.com>

Date: Sat, 18 Apr 2020 03:01:02 UTC

Severity: normal

Tags: fixed, patch

Found in version 28.0.50

Fixed in version 28.1

Done: "Basil L. Contovounesios" <contovob <at> tcd.ie>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 40693 in the body.
You can then email your comments to 40693 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 18 Apr 2020 03:01:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Ivan Andrus <darthandrus <at> gmail.com>:
New bug report received and forwarded. Copy sent to bug-gnu-emacs <at> gnu.org. (Sat, 18 Apr 2020 03:01:02 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Ivan Andrus <darthandrus <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 28.0.50; json-encode-alist changes alist
Date: Fri, 17 Apr 2020 20:59:54 -0600
I recently filed #40692 (https://debbugs.gnu.org/cgi/bugreport.cgi?bug=40692) about a problem I was having with magit forge using creating the wrong title because an alist created in a function contained a single constant at the end and it was being changed every time.  I still find it surprising that a back-quoted list would behave that way, but I suspect it will be considered desired, or at least expected behavior.

I was able to track it down to the actual function that was causing the change: json-encode-alist when json-encoding-object-sort-predicate is set.  I have it set, and indeed it's extremely useful to view JSON that in a meaningful order.  I don't think that anyone would expect that encoding json would change the underlying data, so I think json-encode-alist should be changed to make a copy to avoid this situation.


  (require 'json)

  (defun fun-withdraw (amount)
    (json-encode-alist
     `((tamount . , amount)
       (const . some-constant))))

  (fun-withdraw 12) ;; Run this a few times and nothing will change

  (setq json-encoding-object-sort-predicate #'string<)

  (fun-withdraw 12) ;; Now it will change every time


It's late now, but if I have some time in the next few days, I may submit a patch since it seems like a simple enough change.

-Ivan


In GNU Emacs 28.0.50 (build 18, x86_64-apple-darwin18.7.0, NS appkit-1671.60 Version 10.14.6 (Build 18G4032))
of 2020-04-10 built on iandrus-macOS
Repository revision: 965390ca5f206c6358014574fe5140ba40850391
Repository branch: master
Windowing system distributor 'Apple', version 10.3.1671
System Description:  Mac OS X 10.14.6

Configured using:
'configure
PKG_CONFIG_PATH=/opt/X11/lib/pkgconfig:/usr/local/opt/libxml2/lib/pkgconfig
--with-sound=yes --with-ns --with-modules --with-file-notification=yes
--enable-gcc-warnings=warn-only --with-xpm --with-jpeg --with-tiff
--with-gif --with-png --with-rsvg --with-xml2 --with-imagemagick
--with-json --with-xft --with-libotf --with-gnutls=no --with-makeinfo
--with-libgmp'

Configured features:
RSVG IMAGEMAGICK GLIB NOTIFY KQUEUE ACL LIBXML2 ZLIB TOOLKIT_SCROLL_BARS
XIM NS MODULES THREADS JSON PDUMPER GMP

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

Features:
(mailalias mailclient shadow emacsbug sendmail calc-bin calc-yank
gap-smie gap-mode gap-process sage-shell-mode deferred cursor-sensor
ess-r-mode ess-r-flymake ess-r-xref ess-trns ess-r-package
ess-r-completion ess-roxy ess-r-syntax ess-rd ess-s-lang ess-help
ess-mode ess-inf ess-tracebug ess ess-utils ess-custom disass calc-vec
calccomp calc-aent all-the-icons-faces data-material data-weathericons
data-octicons data-fileicons data-faicons data-alltheicons
buttercup-compat cider-find cider-scratch apropos array jsonrpc
jupyter-org-extensions jupyter-rest-api jupyter-org-client jupyter-repl
jupyter-kernel-manager jupyter-channel jupyter-widget-client websocket
simple-httpd jupyter-kernelspec jupyter-env jupyter-client
jupyter-comm-layer jupyter-messages hmac-def jupyter-mime jupyter-base
lsp-html lsp-mode em-glob bindat treemacs org-inlinetask gnus-art mm-uu
mml2015 mm-view mml-smime smime dig ol-bibtex nnir gnus-sum gnus-group
gnus-undo gnus-start gnus-cloud nnimap nnmail mail-source utf7 netrc
nnoo gnus-spec gnus-int gnus-range gnus-win esh-mode doc-view bibtex
ob-octave eshell esh-cmd esh-ext esh-opt esh-proc esh-io esh-arg
esh-module esh-groups esh-util ebnf2ps ps-print ps-print-loaddefs ps-def
lpr cider cider-debug cider-inspector cider-browse-ns cider-mode
cider-completion cider-profile cider-eval cider-repl-history cider-repl
cider-resolve cider-test cider-overlays cider-stacktrace cider-doc
cider-browse-spec cider-clojuredocs cider-popup cider-eldoc cider-client
cider-common cider-util cider-connection sesman-browser nrepl-client
queue nrepl-dict cider-compat sesman clojure-mode parseedn
parseclj-parser parseclj-lex a ob-clojure calc-store calc-trail
org-attach finder perl6-repl pkg-info epl leaf treemacs-compatibility
treemacs-mode treemacs-bookmarks hydra lv thunk treemacs-interface
treemacs-extensions treemacs-persistence treemacs-mouse-interface
treemacs-tag-follow-mode treemacs-filewatch-mode treemacs-tags
treemacs-visuals treemacs-fringe-indicator pulse treemacs-faces
treemacs-follow-mode treemacs-rendering treemacs-async treemacs-icons
treemacs-themes treemacs-workspaces treemacs-scope treemacs-dom
treemacs-core-utils treemacs-macros treemacs-customization ace-window
pfuture inline term ehelp mail-extr compare-w axle vcursor texnfo-upd
texinfo mc-mark-more rnc-mode goto-last-change mc-separate-operations
align info-colors elisp-depmap-exec elisp-depmap-graph
elisp-depmap-parse elisp-depmap-secondhelp xwwp-follow-link xwwp xwidget
async-await iter2 promise promise-rejection-tracking promise-finally
promise-done promise-es6-extensions promise-core autoload tar-mode
arc-mode archive-mode lisp-mnt php-mode etags fileloop xref cc-langs
php-face php php-project projectile ibuf-ext ibuffer ibuffer-loaddefs
cmake-mode writeroom-mode visual-fill-column mixed-pitch vc-annotate
poly-markdown dockerfile-mode forge-list forge-commands forge-semi
forge-bitbucket buck forge-gogs gogs forge-gitea gtea forge-gitlab glab
forge-github ghub-graphql treepy gsexp ghub let-alist forge-notify
forge-revnote forge-pullreq forge-issue forge-topic forge-post
forge-repo forge forge-core forge-db closql emacsql-sqlite emacsql
emacsql-compiler snout-mode mhtml-mode css-mode eww mm-url gnus nnheader
url-queue shr svg find-file whitespace make-mode magit-imenu git-rebase
log-view novice vc-svn vc-cvs project deadgrep spinner lua-mode timezone
mm-archive plantuml-mode gnuplot-gui gnuplot python markdown-mode
edit-indirect conf-mode yaml-mode json-mode json-reformat json-snatcher
js sql view dabbrev inflections mc-edit-lines multiple-cursors-core sort
calc-arith calc-misc calc-math macros rot13 disp-table cperl-mode
hippie-exp org-forms cl-print calc-alg calc-menu calc-ext calc
calc-loaddefs calc-macs artist picture reporter rect ffap skeleton
tabify cal-move org-datetree org-capture sh-script org-duration
cal-julian lunar solar cal-dst cal-iso face-remap perl6-mode perl6-imenu
perl6-indent smie perl6-font-lock char-fold misearch multi-isearch
cap-words superword subword two-column executable bug-reference
magit-extras magit-bookmark magit-submodule magit-obsolete magit-popup
magit-blame magit-stash magit-reflog magit-bisect magit-push magit-pull
magit-fetch magit-clone magit-remote magit-commit magit-sequence
magit-notes magit-worktree magit-tag magit-merge magit-branch
magit-reset magit-files magit-refs magit-status magit magit-repos
magit-apply magit-wip magit-log magit-diff smerge-mode diff magit-core
magit-autorevert magit-margin magit-transient magit-process magit-mode
git-commit transient magit-git magit-section magit-utils writegood-mode
log-edit message rfc822 mml mml-sec gnus-util rmail rmail-loaddefs
mm-decode mm-bodies mailabbrev gmm-utils mailheader pcvs-util
with-editor async-bytecomp async vc vc-dispatcher add-log
ido-completing-read+ memoize cus-edit minibuf-eldef gvol-light-theme
elide-head highlight-tail hl-sexp highlight-symbol adafruit-wisdom
org-eldoc reveal epa-file epa epg epg-config request mail-utils flyspell
ispell poly-org polymode derived poly-lock polymode-base polymode-weave
polymode-export polymode-compat polymode-methods polymode-core
polymode-classes eieio-custom color org-archive eieio-opt speedbar
dframe tls gnutls network-stream url-http url-gw nsm rmc puny url-cache
url-auth url url-proxy url-privacy url-expand url-methods url-history
url-cookie url-domsuf dad-joke time semantic/idle semantic/analyze
semantic/sort semantic/scope semantic/analyze/fcn semantic/db eieio-base
semantic/format ezimage semantic/tag-ls semantic/find semantic/ctxt
org-drill persist org-id org-tempo tempo org-protocol org-mouse
org-habit org-ctags ob-latex ob-gnuplot ob-plantuml ob-js vc-git
diff-mode rng-xsd xsd-regexp rng-cmpct hideshow rng-nxml rng-valid
nxml-mode nxml-outln nxml-rap sgml-mode dom perl6-detect which-func
paren semantic/util-modes semantic/util semantic semantic/tag
semantic/lex semantic/fw mode-local cedet saveplace msb mb-depth
icomplete gud hl-line autorevert filenotify cus-start cus-load helpful
imenu trace edebug f dash-functional help-fns radix-tree elisp-refs loop
avy autoinsert cl-extra yasnippet find-file-in-repository smex ido
flycheck-clang-analyzer flycheck rtags popup repeat docker-tramp
kubernetes-tramp tramp-cache tramp-okta tramp-sh tramp tramp-loaddefs
trampver tramp-integration files-x tramp-compat parse-time iso8601
ls-lisp asm-mode info-look cc-mode cc-fonts cc-guess cc-menus cc-cmds
cc-styles cc-align cc-engine cc-vars cc-defs bookmark
text-property-search beacon fold ripgrep wgrep grep literal-string
ox-pandoc ht ox-org ox-odt rng-loc rng-uri rng-parse rng-match rng-dt
rng-util rng-pttrn nxml-parse nxml-ns nxml-enc xmltok nxml-util ox-latex
ox-icalendar ox-html table ox-ascii ox-publish ox org-element avl-tree
generator org-wc idle-org-agenda org-mac-link org-mobile org-agenda
diary-lib diary-loaddefs org-crypt ob-obxml oberon-shell-mode ob-shell
shell ob-python org-clock orgtbl-ascii-plot org ob ob-tangle ob-ref
ob-lob ob-table ob-exp org-macro org-footnote org-src ob-comint
org-pcomplete pcomplete org-list org-faces org-entities time-date
noutline outline org-version ob-emacs-lisp ob-core ob-eval org-table ol
org-keys org-compat org-macs org-loaddefs cal-menu calendar cal-loaddefs
sage jka-compr xml preview prv-emacs tex-buf latex latex-flymake
flymake-proc flymake compile comint ansi-color ring warnings thingatpt
tex-ispell tex-style tex crm edit-server elnode dired+ image-dired
image-mode exif format-spec image-file dired-x dired-aux dired
dired-loaddefs db web time-stamp s url-util mailcap mm-encode mail-parse
rfc2231 rfc2047 rfc2045 mm-util ietf-drums mail-prsvr fakir dotassoc kv
noflet cl-indent dash ert ewoc debug backtrace help-mode find-func
savehist desktop frameset drag-stuff recentf tree-widget wid-edit
browse-kill-ring delsel backtr keyfreq uptimes pp server assoc advice
windmove finder-inf package-x tab-line pcase easy-mmode cl rx tex-site
info edmacro kmacro package easymenu browse-url url-handlers url-parse
auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json subr-x map url-vars seq byte-opt gv bytecomp
byte-compile cconv cl-loaddefs cl-lib tooltip eldoc electric uniquify
ediff-hook vc-hooks lisp-float-type mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic 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 charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
multi-tty make-network-process emacs)




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 18 Apr 2020 17:30:02 GMT) Full text and rfc822 format available.

Message #8 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 18 Apr 2020 20:29:03 +0300
Hi Ivan,

thanks for the report.

On 18.04.2020 05:59, Ivan Andrus wrote:
> It's late now, but if I have some time in the next few days, I may submit a patch since it seems like a simple enough change.

How about this one?

diff --git a/lisp/json.el b/lisp/json.el
index 18d7fda882..b65884f913 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -564,9 +564,10 @@ json-encode-alist
   "Return a JSON representation of ALIST."
   (when json-encoding-object-sort-predicate
     (setq alist
-          (sort alist (lambda (a b)
-                        (funcall json-encoding-object-sort-predicate
-                                 (car a) (car b))))))
+          (sort (copy-sequence alist)
+                (lambda (a b)
+                  (funcall json-encoding-object-sort-predicate
+                           (car a) (car b))))))
   (format "{%s%s}"
           (json-join
            (json--with-indentation




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 18 Apr 2020 21:01:01 GMT) Full text and rfc822 format available.

Message #11 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Ivan Andrus <darthandrus <at> gmail.com>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 18 Apr 2020 15:00:33 -0600
That's basically what I had in mind, though I was also going to check other json-encode functions (which you may have already done) to make sure they didn't do something similar.

-Ivan

> On Apr 18, 2020, at 11:29 AM, Dmitry Gutov <dgutov <at> yandex.ru> wrote:
> 
> Hi Ivan,
> 
> thanks for the report.
> 
> On 18.04.2020 05:59, Ivan Andrus wrote:
>> It's late now, but if I have some time in the next few days, I may submit a patch since it seems like a simple enough change.
> 
> How about this one?
> 
> diff --git a/lisp/json.el b/lisp/json.el
> index 18d7fda882..b65884f913 100644
> --- a/lisp/json.el
> +++ b/lisp/json.el
> @@ -564,9 +564,10 @@ json-encode-alist
>   "Return a JSON representation of ALIST."
>   (when json-encoding-object-sort-predicate
>     (setq alist
> -          (sort alist (lambda (a b)
> -                        (funcall json-encoding-object-sort-predicate
> -                                 (car a) (car b))))))
> +          (sort (copy-sequence alist)
> +                (lambda (a b)
> +                  (funcall json-encoding-object-sort-predicate
> +                           (car a) (car b))))))
>   (format "{%s%s}"
>           (json-join
>            (json--with-indentation





Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 18 Apr 2020 23:14:01 GMT) Full text and rfc822 format available.

Message #14 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 01:13:04 +0200
Dmitry Gutov <dgutov <at> yandex.ru> writes:

> diff --git a/lisp/json.el b/lisp/json.el
> index 18d7fda882..b65884f913 100644
> --- a/lisp/json.el
> +++ b/lisp/json.el
> @@ -564,9 +564,10 @@ json-encode-alist
>    "Return a JSON representation of ALIST."
>    (when json-encoding-object-sort-predicate
>      (setq alist
> -          (sort alist (lambda (a b)
> -                        (funcall json-encoding-object-sort-predicate
> -                                 (car a) (car b))))))
> +          (sort (copy-sequence alist)
> +                (lambda (a b)
> +                  (funcall json-encoding-object-sort-predicate
> +                           (car a) (car b))))))
>    (format "{%s%s}"
>            (json-join
>             (json--with-indentation

Why?  Isn't this a classical example of modifiable code?  After some
iterations

(symbol-function 'fun-withdraw)

=>

(closure
 (t)
 (amount)
 (json-encode-alist
  (cons
   (cons 'tamount amount)
   '((const . some-constant)
     (tamount . 12)
     (tamount . 12)
     (tamount . 12)))))

So the problem is in the implementation of `fun-withdraw', not in that
of `json-encode-alist' or `sort'.

I mean, it's really an example of an ugly trap, but how is
`json-encode-alist' different from other functions working with lists
that would exploit the same problem?

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 00:11:02 GMT) Full text and rfc822 format available.

Message #17 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 03:10:14 +0300
On 19.04.2020 02:13, Michael Heerdegen wrote:
> I mean, it's really an example of an ugly trap, but how is
> `json-encode-alist' different from other functions working with lists
> that would exploit the same problem?

It's a difference between cl-delete and cl-remove. For example.

But unlike both of them, the semantics of json-encode-alist give no 
indication that the argument is going to be modified.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 00:31:01 GMT) Full text and rfc822 format available.

Message #20 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 02:29:56 +0200
Dmitry Gutov <dgutov <at> yandex.ru> writes:

> [...] the semantics of json-encode-alist give no indication that the
> argument is going to be modified.

Ok, I missed that aspect of the issue.

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 00:35:01 GMT) Full text and rfc822 format available.

Message #23 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 01:33:55 +0100
Michael Heerdegen <michael_heerdegen <at> web.de> writes:

> Why?  Isn't this a classical example of modifiable code?  After some
> iterations
>
> (symbol-function 'fun-withdraw)
>
> =>
>
> (closure
>  (t)
>  (amount)
>  (json-encode-alist
>   (cons
>    (cons 'tamount amount)
>    '((const . some-constant)
>      (tamount . 12)
>      (tamount . 12)
>      (tamount . 12)))))
>
> So the problem is in the implementation of `fun-withdraw', not in that
> of `json-encode-alist' or `sort'.
>
> I mean, it's really an example of an ugly trap, but how is
> `json-encode-alist' different from other functions working with lists
> that would exploit the same problem?

A library function that encodes arbitrary Elisp objects as JSON
shouldn't destructively modify its argument; it should be possible to
call json-encode on the same object twice and get the same result.

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 00:35:02 GMT) Full text and rfc822 format available.

Message #26 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 01:34:38 +0100
[Message part 1 (text/plain, inline)]
Dmitry Gutov <dgutov <at> yandex.ru> writes:

> How about this one?
>
> diff --git a/lisp/json.el b/lisp/json.el
> index 18d7fda882..b65884f913 100644
> --- a/lisp/json.el
> +++ b/lisp/json.el
> @@ -564,9 +564,10 @@ json-encode-alist
>    "Return a JSON representation of ALIST."
>    (when json-encoding-object-sort-predicate
>      (setq alist
> -          (sort alist (lambda (a b)
> -                        (funcall json-encoding-object-sort-predicate
> -                                 (car a) (car b))))))
> +          (sort (copy-sequence alist)
> +                (lambda (a b)
> +                  (funcall json-encoding-object-sort-predicate
> +                           (car a) (car b))))))
>    (format "{%s%s}"
>            (json-join
>             (json--with-indentation

LGTM.  Perhaps add a test as well:

[json-tests.diff (text/x-diff, inline)]
diff --git a/test/lisp/json-tests.el b/test/lisp/json-tests.el
index 05837e83f9..9d7ffd5feb 100644
--- a/test/lisp/json-tests.el
+++ b/test/lisp/json-tests.el
@@ -269,10 +269,13 @@ test-json-encode-plist-with-sort-predicate
     (should (equal (json-encode plist) "{\"a\":1,\"b\":2,\"c\":3}"))))
 
 (ert-deftest test-json-encode-alist-with-sort-predicate ()
-  (let ((alist '((:c . 3) (:a . 1) (:b . 2)))
-        (json-encoding-object-sort-predicate 'string<)
-        (json-encoding-pretty-print nil))
-    (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))))
+  (let* ((alist '((:c . 3) (:a . 1) (:b . 2)))
+         (clone (copy-sequence alist))
+         (json-encoding-object-sort-predicate #'string<)
+         (json-encoding-pretty-print nil))
+    (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))
+    ;; Ensure sorting isn't destructive (bug#40693).
+    (should (equal alist clone))))
 
 (ert-deftest test-json-encode-list ()
   (let ((json-encoding-pretty-print nil))
[Message part 3 (text/plain, inline)]
Eli, would this be okay for emacs-27?

Thanks,

-- 
Basil

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 20:36:01 GMT) Full text and rfc822 format available.

Message #29 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Ivan Andrus <darthandrus <at> gmail.com>
Cc: 40693 <at> debbugs.gnu.org
Subject: Re: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 13:35:25 -0700
[Message part 1 (text/plain, inline)]
> I still find it surprising that a back-quoted list would behave that way

Me too. This should be documented better, and to try to do that I installed the 
attached patch into the emacs-27 branch (this should appear on the master branch 
after the next merge).
[0001-Document-that-quoting-yields-constants.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 21:02:02 GMT) Full text and rfc822 format available.

Message #32 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>, Ivan Andrus <darthandrus <at> gmail.com>
Cc: 40693 <at> debbugs.gnu.org
Subject: RE: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 14:01:33 -0700 (PDT)
> @defspec quote object
>  This special form returns @var{object}, without evaluating it.
> +The returned value is a constant, and should not be modified.
>  @end defspec

Not true.

The returned value _can be_ a constant, e.g., if the
code is byte-compiled, and depending on the kind of
object that's returned.

'foo returns the symbol foo.  Depending on the
context, you can certainly modify the properties of
that symbol - its `symbol-value', `symbol-function',
and `symbol-plist'.

This kind of wholesale change risks making things
less clear instead of more clear.

See also Eli's message about the purpose of the
manual, and the need to handle this overall message
(about the byte-compiler sometimes causing a return
value to become a constant) in a single place.

The message should be about not _depending_ on a
quoted value returning a new object (e.g. new list
structure).  The message should not be that `quote'
never returns a new object.

> +If a subexpression of a backquote construct has
> +no substitutions or splices, it acts like
> +@code{quote} in that it yields a constant that
> +should not be modified.

A constant is not something that _should not_ be
modified.  It's something that _cannot_ be modified.

And something that should not be modified is not
a constant.  For a constant, there's zero reason
to tell users not to modify it or tell them that
they should not modify it - they _cannot_.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 22:15:02 GMT) Full text and rfc822 format available.

Message #35 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Drew Adams <drew.adams <at> oracle.com>, Ivan Andrus <darthandrus <at> gmail.com>
Cc: 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 15:14:11 -0700
[Message part 1 (text/plain, inline)]
On 4/19/20 2:01 PM, Drew Adams wrote:

> 'foo returns the symbol foo.  Depending on the
> context, you can certainly modify the properties of
> that symbol - its `symbol-value', `symbol-function',
> and `symbol-plist'.

Thanks for catching that. I installed the attached further patch to fix that.

> The message should be about not _depending_ on a
> quoted value returning a new object (e.g. new list
> structure).  The message should not be that `quote'
> never returns a new object.

The message is already in the new "Constants and Mutability" section in the 
emacs-27 branch. No doubt the wording could be improved....

> A constant is not something that _should not_ be
> modified.  It's something that _cannot_ be modified.

This is merely a terminology issue; feel free to come up with better terminology.
[0001-Fix-mutability-glitches-reported-by-Drew-Adams.patch (text/x-patch, attachment)]

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sun, 19 Apr 2020 22:31:01 GMT) Full text and rfc822 format available.

Message #38 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 20 Apr 2020 00:29:40 +0200
Paul Eggert <eggert <at> cs.ucla.edu> writes:

> +types include conses, vectors, strings, and symbols.  For example, the string
>  literal @code{"aaa"} yields a constant string, whereas the function
>  call @code{(make-string 3 ?a)} yields a mutable string that can be
>  changed via later calls to @code{aset}.

> -  A program should not attempt to modify a constant because the
> +  Modifying a constant symbol signals an error (@pxref{Constant Variables}).
> +A program should not attempt to modify other types of constants because the

Do you speak about symbols or the binding of symbols here?

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 00:01:02 GMT) Full text and rfc822 format available.

Message #41 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 16:59:59 -0700
On 4/19/20 3:29 PM, Michael Heerdegen wrote:
>> -  A program should not attempt to modify a constant because the
>> +  Modifying a constant symbol signals an error (@pxref{Constant Variables}).
>> +A program should not attempt to modify other types of constants because the
> Do you speak about symbols or the binding of symbols here?

I meant the symbol's value (what symbol-value returns). I could change the 
wording to "Modifying the value of a constant symbol..."




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 00:27:01 GMT) Full text and rfc822 format available.

Message #44 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 20 Apr 2020 02:25:46 +0200
Paul Eggert <eggert <at> cs.ucla.edu> writes:

> On 4/19/20 3:29 PM, Michael Heerdegen wrote:
> >> -  A program should not attempt to modify a constant because the
> >> + Modifying a constant symbol signals an error (@pxref{Constant
> >> Variables}).
> >> +A program should not attempt to modify other types of constants
> >> because the
> > Do you speak about symbols or the binding of symbols here?
>
> I meant the symbol's value (what symbol-value returns). I could change
> the wording to "Modifying the value of a constant symbol..."

This can be confused with variables declared with `defconst' - these are
declared constants but it is possible to change their bindings.  You are
speaking about self-evaluating symbols like nil and t, right?  Because
all symbols are constants, try to change the symbol `foo', there is no
way to change it in any way from Lisp.  But you can evaluate it, and the
binding is not always the same, unlike self-evaluating symbols like nil
or :keyword.

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 00:33:02 GMT) Full text and rfc822 format available.

Message #47 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 17:32:12 -0700
On 4/19/20 5:25 PM, Michael Heerdegen wrote:
> This can be confused with variables declared with `defconst' - these are
> declared constants but it is possible to change their bindings.  You are
> speaking about self-evaluating symbols like nil and t, right?

Yes, and this includes symbols that start with : in the initial obarray (though 
there's a weird exception for (set :foo :foo)).

How about if we change "Modifying a constant symbol signals an error..." to 
"Changing a self-evaluating symbol's value signals an error..."?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 00:58:01 GMT) Full text and rfc822 format available.

Message #50 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Michael Heerdegen <michael_heerdegen <at> web.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 20 Apr 2020 02:57:01 +0200
Paul Eggert <eggert <at> cs.ucla.edu> writes:

> How about if we change "Modifying a constant symbol signals an
> error..." to "Changing a self-evaluating symbol's value signals an
> error..."?

Yes, better (though I would rather say "Trying to..." or "Attempting
to..." - Emacs doesn't let you do this, i.e. you get the error before
it's too late).

Michael.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 02:56:01 GMT) Full text and rfc822 format available.

Message #53 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Heerdegen <michael_heerdegen <at> web.de>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org,
 Drew Adams <drew.adams <at> oracle.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 19:55:47 -0700
On 4/19/20 5:57 PM, Michael Heerdegen wrote:
>> How about if we change "Modifying a constant symbol signals an
>> error..." to "Changing a self-evaluating symbol's value signals an
>> error..."?
> Yes, better (though I would rather say "Trying to..." or "Attempting
> to..." - Emacs doesn't let you do this, i.e. you get the error before
> it's too late).

OK, I put it in with "Trying to". Also, I changed "self-evaluating symbol" to 
"Constant variable" because the latter class includes the former, and also 
includes a few variables like most-positive-fixnum that are not self-evaluating 
but still cannot be modified.

Who came up with the term "Constant variable", anyway? They must have had an odd 
sense of humor.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 05:46:02 GMT) Full text and rfc822 format available.

Message #56 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Drew Adams <drew.adams <at> oracle.com>
To: Michael Heerdegen <michael_heerdegen <at> web.de>, Paul Eggert
 <eggert <at> cs.ucla.edu>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: RE: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sun, 19 Apr 2020 22:45:41 -0700 (PDT)
> > How about if we change "Modifying a constant symbol signals an
> > error..." to "Changing a self-evaluating symbol's value signals an
> > error..."?
> 
> Yes, better (though I would rather say "Trying to..." or "Attempting
> to..." - Emacs doesn't let you do this, i.e. you get the error before
> it's too late).

Yes.  It's good to explicitly say that we're talking
about changing a symbol's value here, and not just
changing a symbol.  The notion of "changing" a symbol
can mean different things to different readers.

Consider (put nil 'toto 42) or (put :foo 'toto 42).
Did we change the symbol nil or :foo, both of which
are self-evaluating?  For some meaning of "change",
we did - we changed its property list.

The message should be just that you can't change the
`symbol-value' of a self-evaluating symbol/variable.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 20 Apr 2020 14:57:02 GMT) Full text and rfc822 format available.

Message #59 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: michael_heerdegen <at> web.de, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 20 Apr 2020 17:56:19 +0300
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Date: Sun, 19 Apr 2020 19:55:47 -0700
> Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
> 
> Who came up with the term "Constant variable", anyway? They must have had an odd 
> sense of humor.

"Constants aren't, variables won't", right?




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 10:12:01 GMT) Full text and rfc822 format available.

Message #62 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 11:11:38 +0100
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Dmitry Gutov <dgutov <at> yandex.ru> writes:
>
>> How about this one?
>>
>> diff --git a/lisp/json.el b/lisp/json.el
>> index 18d7fda882..b65884f913 100644
>> --- a/lisp/json.el
>> +++ b/lisp/json.el
>> @@ -564,9 +564,10 @@ json-encode-alist
>>    "Return a JSON representation of ALIST."
>>    (when json-encoding-object-sort-predicate
>>      (setq alist
>> -          (sort alist (lambda (a b)
>> -                        (funcall json-encoding-object-sort-predicate
>> -                                 (car a) (car b))))))
>> +          (sort (copy-sequence alist)
>> +                (lambda (a b)
>> +                  (funcall json-encoding-object-sort-predicate
>> +                           (car a) (car b))))))
>>    (format "{%s%s}"
>>            (json-join
>>             (json--with-indentation
>
> LGTM.  Perhaps add a test as well:
>
> diff --git a/test/lisp/json-tests.el b/test/lisp/json-tests.el
> index 05837e83f9..9d7ffd5feb 100644
> --- a/test/lisp/json-tests.el
> +++ b/test/lisp/json-tests.el
> @@ -269,10 +269,13 @@ test-json-encode-plist-with-sort-predicate
>      (should (equal (json-encode plist) "{\"a\":1,\"b\":2,\"c\":3}"))))
>  
>  (ert-deftest test-json-encode-alist-with-sort-predicate ()
> -  (let ((alist '((:c . 3) (:a . 1) (:b . 2)))
> -        (json-encoding-object-sort-predicate 'string<)
> -        (json-encoding-pretty-print nil))
> -    (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))))
> +  (let* ((alist '((:c . 3) (:a . 1) (:b . 2)))
> +         (clone (copy-sequence alist))
> +         (json-encoding-object-sort-predicate #'string<)
> +         (json-encoding-pretty-print nil))
> +    (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))
> +    ;; Ensure sorting isn't destructive (bug#40693).
> +    (should (equal alist clone))))
>  
>  (ert-deftest test-json-encode-list ()
>    (let ((json-encoding-pretty-print nil))
>
>
> Eli, would this be okay for emacs-27?

Any chance of getting this fix in?

Thanks,

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 10:32:02 GMT) Full text and rfc822 format available.

Message #65 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, dgutov <at> yandex.ru
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 13:30:58 +0300
> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> Date: Wed, 29 Apr 2020 11:11:38 +0100
> Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
> 
> > Eli, would this be okay for emacs-27?
> 
> Any chance of getting this fix in?

No one tried to come up with arguments why this has to be in emacs-27.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 12:10:01 GMT) Full text and rfc822 format available.

Message #68 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>, "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 15:08:57 +0300
On 29.04.2020 13:30, Eli Zaretskii wrote:
>> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
>> Date: Wed, 29 Apr 2020 11:11:38 +0100
>> Cc: Ivan Andrus <darthandrus <at> gmail.com>, 40693 <at> debbugs.gnu.org
>>
>>> Eli, would this be okay for emacs-27?
>>
>> Any chance of getting this fix in?
> 
> No one tried to come up with arguments why this has to be in emacs-27.

Let me try:

It fixes a bug, one which could be annoying to investigate, the fix is 
small and localized to the case when json-encoding-object-sort-predicate 
is non-nil (so pretty safe).

It's not a regression from Emacs 26, though.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 12:23:01 GMT) Full text and rfc822 format available.

Message #71 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 15:21:45 +0300
> Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Wed, 29 Apr 2020 15:08:57 +0300
> 
> > No one tried to come up with arguments why this has to be in emacs-27.
> 
> Let me try:
> 
> It fixes a bug, one which could be annoying to investigate, the fix is 
> small and localized to the case when json-encoding-object-sort-predicate 
> is non-nil (so pretty safe).

It also makes the function slower.  Which may be an important issue
for JSON processing.  Callers that don't care about the original list
will be "punished" regardless.

How about adding an optional argument instead, by default off, to
request this behavior?  then callers who care about the original alist
could request a non-destructive operation, and others won't suffer any
slowdown.

> It's not a regression from Emacs 26, though.

Right.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 14:29:01 GMT) Full text and rfc822 format available.

Message #74 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 17:28:05 +0300
On 29.04.2020 15:21, Eli Zaretskii wrote:
>> Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
>> From: Dmitry Gutov <dgutov <at> yandex.ru>
>> Date: Wed, 29 Apr 2020 15:08:57 +0300
>>
>>> No one tried to come up with arguments why this has to be in emacs-27.
>>
>> Let me try:
>>
>> It fixes a bug, one which could be annoying to investigate, the fix is
>> small and localized to the case when json-encoding-object-sort-predicate
>> is non-nil (so pretty safe).
> 
> It also makes the function slower.  Which may be an important issue
> for JSON processing.  Callers that don't care about the original list
> will be "punished" regardless.

It's a somewhat fair point (copy-sequence is much faster than sort, 
usually, but if we include GC time, it can be significant). The way 
json-encoding-object-sort-predicate is implemented, though, it's not 
very performance-oriented.

Applications like lsp-mode aren't going to use this variable, so they 
shouldn't be hit (and it isn't supported in native JSON either).

On that subject, users really shouldn't set it either. Rather, any 
user-facing feature that outputs JSON, when this behavior could be 
desirable, should have its own tweaking knob.

> How about adding an optional argument instead, by default off, to
> request this behavior?  then callers who care about the original alist
> could request a non-destructive operation, and others won't suffer any
> slowdown.

The current behavior is unsafe, that's the problem. Also, 
json-encode-alist is called in a recursive fashion, so it'd have to be a 
global variable instead.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 14:41:02 GMT) Full text and rfc822 format available.

Message #77 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 17:40:04 +0300
> Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Wed, 29 Apr 2020 17:28:05 +0300
> 
> > How about adding an optional argument instead, by default off, to
> > request this behavior?  then callers who care about the original alist
> > could request a non-destructive operation, and others won't suffer any
> > slowdown.
> 
> The current behavior is unsafe, that's the problem. Also, 
> json-encode-alist is called in a recursive fashion, so it'd have to be a 
> global variable instead.

Sounds like a somewhat hairy issue.  Then let's put this on master
first.  If this turns out to be a very popular change, and no one
complains, we could later back-port it to some 27.x version.

Thanks.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 14:42:01 GMT) Full text and rfc822 format available.

Message #80 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 15:41:19 +0100
Eli Zaretskii <eliz <at> gnu.org> writes:

>> Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
>> From: Dmitry Gutov <dgutov <at> yandex.ru>
>> Date: Wed, 29 Apr 2020 15:08:57 +0300
>> 
>> > No one tried to come up with arguments why this has to be in emacs-27.
>> 
>> Let me try:
>> 
>> It fixes a bug, one which could be annoying to investigate, the fix is 
>> small and localized to the case when json-encoding-object-sort-predicate 
>> is non-nil (so pretty safe).
>
> It also makes the function slower.  Which may be an important issue
> for JSON processing.  Callers that don't care about the original list
> will be "punished" regardless.
>
> How about adding an optional argument instead, by default off, to
> request this behavior?  then callers who care about the original alist
> could request a non-destructive operation, and others won't suffer any
> slowdown.

That was my first thought too.  I have a local WIP branch where I am
refactoring parts of json.el to improve performance, and avoiding the
proposed copy-sequence is one of the fixes.  I've also found a couple of
test JSON files to benchmark against.

>> It's not a regression from Emacs 26, though.
>
> Right.

The reasons I thought the copy-sequence fix might be desirable in Emacs
27 are:

1. It's a simple enough fix to go into the release branch.

2. Users who enable json-encoding-object-sort-predicate are already
   trading off performance for sort order.  In addition to the cost of
   sorting, hash table and plist objects have to first be converted to
   alists before they can be sorted.  My guess is that an extra
   copy-sequence won't make a big difference here.

3. Users who care about sheer performance of JSON serialisation will
   either avoid json-encoding-object-sort-predicate or use the newer
   Jansson functions.

I don't personally mind whether this gets into emacs-27; I just wanted
to see what others thought.

Thanks,

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 15:04:02 GMT) Full text and rfc822 format available.

Message #83 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 18:02:52 +0300
On 29.04.2020 17:40, Eli Zaretskii wrote:
>> Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
>> From: Dmitry Gutov <dgutov <at> yandex.ru>
>> Date: Wed, 29 Apr 2020 17:28:05 +0300
>>
>>> How about adding an optional argument instead, by default off, to
>>> request this behavior?  then callers who care about the original alist
>>> could request a non-destructive operation, and others won't suffer any
>>> slowdown.
>>
>> The current behavior is unsafe, that's the problem. Also,
>> json-encode-alist is called in a recursive fashion, so it'd have to be a
>> global variable instead.
> 
> Sounds like a somewhat hairy issue.  Then let's put this on master
> first.  If this turns out to be a very popular change, and no one
> complains, we could later back-port it to some 27.x version.

*shrug*, I'm certainly not going to insist, but we could push the 
"slightly slower" simple patch to emacs-27, and go with Basil's refactor 
(sounds exciting!) on master.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Wed, 29 Apr 2020 15:08:02 GMT) Full text and rfc822 format available.

Message #86 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Dmitry Gutov <dgutov <at> yandex.ru>
Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Wed, 29 Apr 2020 18:07:25 +0300
> Cc: contovob <at> tcd.ie, darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org
> From: Dmitry Gutov <dgutov <at> yandex.ru>
> Date: Wed, 29 Apr 2020 18:02:52 +0300
> 
> > Sounds like a somewhat hairy issue.  Then let's put this on master
> > first.  If this turns out to be a very popular change, and no one
> > complains, we could later back-port it to some 27.x version.
> 
> *shrug*, I'm certainly not going to insist, but we could push the 
> "slightly slower" simple patch to emacs-27, and go with Basil's refactor 
> (sounds exciting!) on master.

I'd rather not risk an extra pretest just because of an issue that has
been with us for a long time.  Emacs 27 is already way too late.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 18 May 2020 01:26:02 GMT) Full text and rfc822 format available.

Message #89 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 18 May 2020 02:24:57 +0100
[Message part 1 (text/plain, inline)]
tags 40693 + patch
quit

[0001-Various-json.el-improvements.patch (text/x-diff, attachment)]
[Message part 3 (text/plain, inline)]
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> I have a local WIP branch where I am refactoring parts of json.el to
> improve performance, and avoiding the proposed copy-sequence is one of
> the fixes.

Large patch attached (fear not (too much); the bulk of it is new
regression tests).  CCing João because it also touches jsonrpc.el.

The only backward-incompatible part of it should be the stricter number
parsing.  Until now[1], json.el accepted numbers with a leading plus
sign, leading zeros, and no integer component, all of which are
forbidden by the JSON spec[2] and rejected by Emacs' native JSON parsing
functions.  I was initially willing to ignore this according to "be
liberal in what you accept and strict in what you output," but given
that libraries are already treating json.el and json.c functions as
interchangeable, I think json.el should be fixed.  Who knows, it may
even help to catch some bad JSON generation somewhere, somewhen.

[1]: (json-read-number): New arg.  Handle explicitly signed numbers.
7712319db8 2008-08-28 20:19:17 +0000
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=7712319db8db97391059925b27ae71f304eee7d2

[2]: https://tools.ietf.org/html/rfc8259#section-6

> I've also found a couple of test JSON files to benchmark against.

I downloaded the three large (sizes 617KB-2.2MB) JSON files here:

https://github.com/miloyip/nativejson-benchmark/tree/master/data

and ran the following benchmark:

[json-bench.el (application/emacs-lisp, inline)]
[Message part 5 (text/plain, inline)]
like so:

$ emacs -Q -batch -f batch-byte-compile json-bench.el
$ for e in /old/emacs /new/emacs; do "$e" -Q -script json-bench.elc *.json; done

with the following (manipulated, for easier comparison) output:

decode canada.json
old
(2.147126599 64 0.714606792)
(2.184767529 64 0.772983248)
(2.190970555 64 0.7805527100000003)
(2.1976262359999996 64 0.786285103)
new
(2.240072109 62 0.701617291)
(2.2702309019999998 62 0.751942913)
(2.270719533 62 0.753646107)
(2.2844341569999997 62 0.7555721750000002)

This file is 2MB of numbers, so I'm speculating that the slight slowdown
is due to the stricter number regexp.

encode canada.json
old
(5.672111511 172 1.9869896950000001)
(5.6662210150000005 172 1.9884779629999993)
(5.670608892 172 1.9894344889999998)
(5.670605633999999 172 1.9883765339999986)
new
(2.4715460570000003 96 1.1121157440000005)
(2.4745830389999997 96 1.1193736730000001)
(2.465779169 96 1.1147967909999998)
(2.4715383390000003 96 1.1174477880000007)

decode citm_catalog.json
old
(1.010838497 34 0.3915659290000004)
(1.006386333 34 0.390746824999999)
(1.06575967 34 0.4085684829999998)
(1.0045083860000001 34 0.3905320579999998)
new
(0.9698266640000001 34 0.39840883500000057)
(0.969682733 34 0.3984373899999998)
(0.972970641 34 0.4009393059999997)
(0.974868517 34 0.40203729300000113)

encode citm_catalog.json
old
(5.063678788 292 3.2036687829999995)
(5.056699193 292 3.201438146000001)
(5.048888015999999 292 3.1970336089999982)
(5.046339092 292 3.197215603)
new
(4.38071611 268 3.0482562499999997)
(4.327264702 268 3.011219348999999)
(4.320279397999999 268 3.0082109699999986)
(4.322212972 268 3.010901667999999)

decode twitter.json
old
(0.788541053 32 0.34238300700000224)
(0.776468868 32 0.33285421699999773)
(0.777424967 32 0.3338445579999991)
(0.779168795 32 0.3343039160000032)
new
(0.7064887900000001 32 0.34372805699999986)
(0.6965333029999999 32 0.33550817600000116)
(0.700879238 32 0.33781746400000046)
(0.701126792 32 0.3365076699999996)

encode twitter.json
old
(2.640938486 163 1.6357819669999998)
(2.64012542 163 1.6363964359999983)
(2.6377853709999997 163 1.6349542369999988)
(2.639091697 163 1.635746981000004)
new
(2.2311199000000004 144 1.5285102250000016)
(2.227288643 144 1.5259519570000002)
(2.229322183 144 1.5265554599999973)
(2.223512976 144 1.5231076530000003)

WDYT?  Thanks,

-- 
Basil

Added tag(s) patch. Request was from "Basil L. Contovounesios" <contovob <at> tcd.ie> to control <at> debbugs.gnu.org. (Mon, 18 May 2020 01:26:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 18 May 2020 14:29:01 GMT) Full text and rfc822 format available.

Message #94 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, joaotavora <at> gmail.com,
 dgutov <at> yandex.ru
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Mon, 18 May 2020 17:27:57 +0300
> From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
> Cc: Dmitry Gutov <dgutov <at> yandex.ru>,  darthandrus <at> gmail.com,
>   40693 <at> debbugs.gnu.org, João Távora
>  <joaotavora <at> gmail.com>
> Date: Mon, 18 May 2020 02:24:57 +0100
> 
> Large patch attached (fear not (too much); the bulk of it is new
> regression tests).  CCing João because it also touches jsonrpc.el.

Thanks, LGTM.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 18 May 2020 22:51:02 GMT) Full text and rfc822 format available.

Message #97 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: Dmitry Gutov <dgutov <at> yandex.ru>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>, Eli Zaretskii <eliz <at> gnu.org>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org,
 João Távora <joaotavora <at> gmail.com>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Tue, 19 May 2020 01:50:18 +0300
On 18.05.2020 04:24, Basil L. Contovounesios wrote:
> Large patch attached (fear not (too much); the bulk of it is new
> regression tests).  CCing João because it also touches jsonrpc.el.

LGTM as well. And stricter validation sounds good in this case.




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Mon, 18 May 2020 23:51:02 GMT) Full text and rfc822 format available.

Message #100 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Tue, 19 May 2020 00:50:33 +0100
Hi Basil,

"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> * lisp/jsonrpc.el (jsonrpc--json-read, jsonrpc--json-encode): Check
> whether native JSON functions are fboundp only once, at load time.

I welcome this small improvement: in fact I had a TODO there to make
something similar.  In the TODO i suggested the jsonrpc--json-read could
be a macro.  You used `defalias` instead and that's fine.  However, I
don't understand the need for the ugly (require 'json), defvar and
declare-function there.  Can't we just use sth like `eval-and-compile`
at the top of the file?  

> -(defun jsonrpc--json-read ()
> -  "Read JSON object in buffer, move point to end of buffer."
> -  ;; TODO: I guess we can make these macros if/when jsonrpc.el
> -  ;; goes into Emacs core.
> -  (cond ((fboundp 'json-parse-buffer) (json-parse-buffer
> -                                       :object-type 'plist
> -                                       :null-object nil
> -                                       :false-object :json-false))
> -        (t                            (let ((json-object-type 'plist))
> -                                        (json-read)))))
> +(defalias 'jsonrpc--json-read
> +  (if (fboundp 'json-parse-buffer)
> +      (lambda ()
> +        (json-parse-buffer :object-type 'plist
> +                           :null-object nil
> +                           :false-object :json-false))
> +    (require 'json)
> +    (defvar json-object-type)
> +    (declare-function json-read "json" ())
> +    (lambda ()
> +      (let ((json-object-type 'plist))
> +        (json-read))))
> +  "Read JSON object in buffer, move point to end of buffer.")

Thanks,
João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Thu, 21 May 2020 21:15:02 GMT) Full text and rfc822 format available.

Message #103 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: João Távora <joaotavora <at> gmail.com>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Thu, 21 May 2020 22:14:20 +0100
João Távora <joaotavora <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> * lisp/jsonrpc.el (jsonrpc--json-read, jsonrpc--json-encode): Check
>> whether native JSON functions are fboundp only once, at load time.
>
> I welcome this small improvement: in fact I had a TODO there to make
> something similar.  In the TODO i suggested the jsonrpc--json-read could
> be a macro.  You used `defalias` instead and that's fine.

Thanks.  Indeed I don't think a macro is necessary here for now.

> However, I don't understand the need for the ugly (require 'json),
> defvar and declare-function there.  Can't we just use sth like
> `eval-and-compile` at the top of the file?

The declarations are needed because the byte-compiler does not know that
loading json.el will e.g. define a dynamically bound variable
json-object-type and a nullary function symbol json-read.  It therefore
not only complains but also generates invalid byte-code.

One way to avoid having to write these declarations is e.g.:

(defalias 'jsonrpc--json-read
  (if (fboundp 'json-parse-buffer)
      (lambda () ...)
    (eval-and-compile
      (require 'json))
    (lambda () ...))
  ...)

But I find this more heavy handed and intrusive, since it
unconditionally loads json.el during byte-compilation, even when
json-parse-buffer is available.

If you prefer this approach as a matter of style I'll push the patch
with the corresponding changes made.

>> -(defun jsonrpc--json-read ()
>> -  "Read JSON object in buffer, move point to end of buffer."
>> -  ;; TODO: I guess we can make these macros if/when jsonrpc.el
>> -  ;; goes into Emacs core.
>> -  (cond ((fboundp 'json-parse-buffer) (json-parse-buffer
>> -                                       :object-type 'plist
>> -                                       :null-object nil
>> -                                       :false-object :json-false))
>> -        (t                            (let ((json-object-type 'plist))
>> -                                        (json-read)))))
>> +(defalias 'jsonrpc--json-read
>> +  (if (fboundp 'json-parse-buffer)
>> +      (lambda ()
>> +        (json-parse-buffer :object-type 'plist
>> +                           :null-object nil
>> +                           :false-object :json-false))
>> +    (require 'json)
>> +    (defvar json-object-type)
>> +    (declare-function json-read "json" ())
>> +    (lambda ()
>> +      (let ((json-object-type 'plist))
>> +        (json-read))))
>> +  "Read JSON object in buffer, move point to end of buffer.")

Thanks,

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Thu, 21 May 2020 22:17:02 GMT) Full text and rfc822 format available.

Message #106 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Thu, 21 May 2020 23:16:30 +0100
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
> The declarations are needed because the byte-compiler does not know that
> loading json.el will e.g. define a dynamically bound variable
> json-object-type and a nullary function symbol json-read.  It therefore
> not only complains but also generates invalid byte-code.

Basil, I understand the need for the declarations, but I was suggesting
something different.  This, at the top, near all the other requires.

   (eval-and-compile
     (unless (fboundp 'json-parse-buffer)
       (require 'json)))

and then do the defalias without the declarations below.

   (defalias blabla
      (if (fboundp 'json-parse-buffer)
          (lambda () json-c-things...)
          (lambda () json-el-things...)))

Am I missing something or doesn't this work like you want?  We're
checking json.c function thrice instead of twice, but doesn't seem very
bad, only a 50% increase :-)

> But I find this more heavy handed and intrusive, since it
> unconditionally loads json.el during byte-compilation, even when
> json-parse-buffer is available.

I think the snippet above doesn't have this problem.

Anyway, this is a minor nitpick, push whatever you think is better.

João





Added tag(s) fixed. Request was from "Basil L. Contovounesios" <contovob <at> tcd.ie> to control <at> debbugs.gnu.org. (Fri, 22 May 2020 14:55:02 GMT) Full text and rfc822 format available.

bug marked as fixed in version 28.1, send any further explanations to 40693 <at> debbugs.gnu.org and Ivan Andrus <darthandrus <at> gmail.com> Request was from "Basil L. Contovounesios" <contovob <at> tcd.ie> to control <at> debbugs.gnu.org. (Fri, 22 May 2020 14:55:02 GMT) Full text and rfc822 format available.

Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Fri, 22 May 2020 14:55:02 GMT) Full text and rfc822 format available.

Message #113 received at 40693-done <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: João Távora <joaotavora <at> gmail.com>
Cc: darthandrus <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>,
 40693-done <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Fri, 22 May 2020 15:54:33 +0100
tags 40693 fixed
close 40693 28.1
quit

João Távora <joaotavora <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>> The declarations are needed because the byte-compiler does not know that
>> loading json.el will e.g. define a dynamically bound variable
>> json-object-type and a nullary function symbol json-read.  It therefore
>> not only complains but also generates invalid byte-code.
>
> Basil, I understand the need for the declarations, but I was suggesting
> something different.  This, at the top, near all the other requires.
>
>    (eval-and-compile
>      (unless (fboundp 'json-parse-buffer)
>        (require 'json)))
>
> and then do the defalias without the declarations below.
>
>    (defalias blabla
>       (if (fboundp 'json-parse-buffer)
>           (lambda () json-c-things...)
>           (lambda () json-el-things...)))
>
> Am I missing something or doesn't this work like you want?

There's a problem with the conditional require at byte-compile time.  If
the version of Emacs doing the byte-compilation has native JSON support,
then it will generate invalid byte-code (and complain) for the case
where there is no native JSON support.  In other words, there will be a
bug when an Emacs without native JSON loads a file that was
byte-compiled in an Emacs with native JSON.

> We're checking json.c function thrice instead of twice, but doesn't
> seem very bad, only a 50% increase :-)

Indeed, that wouldn't be an issue.

>> But I find this more heavy handed and intrusive, since it
>> unconditionally loads json.el during byte-compilation, even when
>> json-parse-buffer is available.
>
> I think the snippet above doesn't have this problem.

Indeed, but I think it suffers from the worse aforementioned problem.

> Anyway, this is a minor nitpick, push whatever you think is better.

Thanks.  I've therefore gone with the original patch[1], as it exhibits
correct behaviour and is the least intrusive, but you should obviously
feel free to tweak it as you prefer.

[1]: Various json.el improvements
3f082af536 2020-05-22 15:16:13 +0100
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=3f082af536c33ba713561e7ad4b691aaad488701

Thanks also to everyone who reviewed the patch.
I'm now closing this bug.

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Fri, 22 May 2020 20:15:02 GMT) Full text and rfc822 format available.

Message #116 received at 40693-done <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, Eli Zaretskii <eliz <at> gnu.org>,
 40693-done <at> debbugs.gnu.org, Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Fri, 22 May 2020 21:14:20 +0100
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

>> Am I missing something or doesn't this work like you want?
>
> There's a problem with the conditional require at byte-compile time.  If
> the version of Emacs doing the byte-compilation has native JSON support,
> then it will generate invalid byte-code (and complain) for the case
> where there is no native JSON support.  In other words, there will be a
> bug when an Emacs without native JSON loads a file that was
> byte-compiled in an Emacs with native JSON.

This doesn't give any any warning:

    (defvar foo-42 42)
     
    (provide 'foo)
    ;; foo.el ends here

and a bar.el

    (eval-and-compile
      (defvar bar-have-native-42 t) ;; or nil
     
      (unless bar-have-native-42 
        (require 'foo)))
     
    (defalias 'forty-two
      (if (eval-when-compile bar-have-native-42)
          (lambda () (message "%s" 42.0))
        (lambda () (message "%s" foo-42))))
    ;; bar.el ends here

And it seems to work in both cases:

     ~/tmp ❯❯❯ emacs -Q --batch -L . -f batch-byte-compile bar.el
     ~/tmp ❯❯❯ emacs -Q --batch -L . -l bar.elc -f forty-two
     42.0
     # now change that t to nil
     ~/tmp ❯❯❯ emacs -Q --batch -L . -f batch-byte-compile bar.el
     ~/tmp ❯❯❯ emacs -Q --batch -L . -l bar.elc -f forty-two
     42

But indeed in the recipe I gave you, I had forgotten the second
eval-when-compile.  If you take that away, it warns again.

No idea how to check if byte-code is "valid" or not: I just check the
warnings.  Can you tell me?

> Thanks.  I've therefore gone with the original patch[1], as it exhibits
> correct behaviour and is the least intrusive, but you should obviously
> feel free to tweak it as you prefer.

I think I'll let it be, the reason I'm not a huge fan is the foward
declarations of individual functions and variables, but it's not so bad.

João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 23 May 2020 16:15:02 GMT) Full text and rfc822 format available.

Message #119 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: João Távora <joaotavora <at> gmail.com>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 23 May 2020 17:13:57 +0100
João Távora <joaotavora <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>>> Am I missing something or doesn't this work like you want?
>>
>> There's a problem with the conditional require at byte-compile time.  If
>> the version of Emacs doing the byte-compilation has native JSON support,
>> then it will generate invalid byte-code (and complain) for the case
>> where there is no native JSON support.  In other words, there will be a
>> bug when an Emacs without native JSON loads a file that was
>> byte-compiled in an Emacs with native JSON.
>
> This doesn't give any any warning:
>
>     (defvar foo-42 42)
>      
>     (provide 'foo)
>     ;; foo.el ends here
>
> and a bar.el
>
>     (eval-and-compile
>       (defvar bar-have-native-42 t) ;; or nil
>      
>       (unless bar-have-native-42 
>         (require 'foo)))
>      
>     (defalias 'forty-two
>       (if (eval-when-compile bar-have-native-42)
>           (lambda () (message "%s" 42.0))
>         (lambda () (message "%s" foo-42))))
>     ;; bar.el ends here
>
> And it seems to work in both cases:
>
>      ~/tmp ❯❯❯ emacs -Q --batch -L . -f batch-byte-compile bar.el
>      ~/tmp ❯❯❯ emacs -Q --batch -L . -l bar.elc -f forty-two
>      42.0
>      # now change that t to nil
>      ~/tmp ❯❯❯ emacs -Q --batch -L . -f batch-byte-compile bar.el
>      ~/tmp ❯❯❯ emacs -Q --batch -L . -l bar.elc -f forty-two
>      42
>
> But indeed in the recipe I gave you, I had forgotten the second
> eval-when-compile.  If you take that away, it warns again.

This is not a representative example for the following reasons:

0. bar.el does not use lexical-binding.
1. The second lambda in forty-two does not let-bind foo-42.
2. If you byte-compile bar.el with bar-have-native-42 set to t, and then
   load bar.elc in an Emacs that has bar-have-native-42 set to nil, then
   42.0 gets printed, which is wrong.  This is due to the incorrect
   usage of eval-when-compile: we want the check to happen at runtime as
   well.

Try this instead:

;;; foo.el --- foo -*- lexical-binding: t -*-

(eval-and-compile
  (unless (fboundp 'json-parse-buffer)
    (require 'json)))

(defalias 'foo
  (if (eval-when-compile (fboundp 'json-parse-buffer))
      (lambda ()
        (json-parse-buffer :object-type 'plist
                           :null-object nil
                           :false-object :json-false))
    (lambda ()
      (let ((json-object-type 'plist))
        (json-read)))))

;;; foo.el ends here

> No idea how to check if byte-code is "valid" or not: I just check the
> warnings.  Can you tell me?

0. emacs -Q -batch -f batch-byte-compile foo.el
1. emacs -Q
2. (fset 'json-parse-buffer nil) C-j
3. M-x load-file RET foo.elc RET
4. (disassemble 'foo) C-j

This prints:

  byte code for foo:
    args: nil
  0       constant  plist
  1       constant  json-read
  2       call      0
  3       return    

whereas I'd expect:

  byte code for foo:
    args: nil
  0       constant  plist
  1       varbind   json-object-type
  2       constant  json-read
  3       call      0
  4       unbind    1
  5       return    

>> Thanks.  I've therefore gone with the original patch[1], as it exhibits
>> correct behaviour and is the least intrusive, but you should obviously
>> feel free to tweak it as you prefer.
>
> I think I'll let it be, the reason I'm not a huge fan is the foward
> declarations of individual functions and variables, but it's not so bad.

I think the declarations make the intention explicit to both the reader
and the byte-compiler in a simple way, without wrestling the
eval-*-compile machinery or allowing for subtle bugs like the ones
above.

Thanks,

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 23 May 2020 19:41:01 GMT) Full text and rfc822 format available.

Message #122 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 23 May 2020 20:40:20 +0100
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> João Távora <joaotavora <at> gmail.com> writes:
>> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> 0. bar.el does not use lexical-binding.
> 1. The second lambda in forty-two does not let-bind foo-42.
> 2. If you byte-compile bar.el with bar-have-native-42 set to t, and then
>    load bar.elc in an Emacs that has bar-have-native-42 set to nil, then
>    42.0 gets printed, which is wrong.  This is due to the incorrect
>    usage of eval-when-compile: we want the check to happen at runtime as
>    well.

I think you mean load-time.  Anyway, this is true if you want 27.1 elc's
to be loadable in 26.x. I was labouring under the impression that we
don't care about that (and this is why I thought of the macro approach).
Do we?  The source file is compatible between multiple emacs version,
but is the byte-compiled file also compatible?

>> No idea how to check if byte-code is "valid" or not: I just check the
>> warnings.  Can you tell me?
> 
> 0. emacs -Q -batch -f batch-byte-compile foo.el
> 1. emacs -Q
> 2. (fset 'json-parse-buffer nil) C-j
> 3. M-x load-file RET foo.elc RET
> 4. (disassemble 'foo) C-j

Thanks.

> I think the declarations make the intention explicit to both the reader
> and the byte-compiler in a simple way, without wrestling the
> eval-*-compile machinery or allowing for subtle bugs like the ones
> above.

The problem, of course, is that you're repeating yourself, a maintenance
hazard.  Not too big in this case.

João




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 23 May 2020 22:42:02 GMT) Full text and rfc822 format available.

Message #125 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: "Basil L. Contovounesios" <contovob <at> tcd.ie>
To: João Távora <joaotavora <at> gmail.com>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 23 May 2020 23:41:20 +0100
João Távora <joaotavora <at> gmail.com> writes:

> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> João Távora <joaotavora <at> gmail.com> writes:
>>> "Basil L. Contovounesios" <contovob <at> tcd.ie> writes:
>
>> 0. bar.el does not use lexical-binding.
>> 1. The second lambda in forty-two does not let-bind foo-42.
>> 2. If you byte-compile bar.el with bar-have-native-42 set to t, and then
>>    load bar.elc in an Emacs that has bar-have-native-42 set to nil, then
>>    42.0 gets printed, which is wrong.  This is due to the incorrect
>>    usage of eval-when-compile: we want the check to happen at runtime as
>>    well.
>
> I think you mean load-time.

Yes.

> Anyway, this is true if you want 27.1 elc's to be loadable in 26.x.

It is also true if you want version N .elc files to be loadable in
version N.  The problems I list are not specific to either JSON or
inter-Emacs-version compat.

> I was labouring under the impression that we don't care about that
> (and this is why I thought of the macro approach).  Do we?  The source
> file is compatible between multiple emacs version, but is the
> byte-compiled file also compatible?

This isn't (primarily) about 26/27 compat, but about supporting
--without-json configurations properly.  In the examples I've given so
far I was exclusively using Emacs master --with-json, hence the
(fset 'json-parse-buffer nil).  Native JSON functions are not guaranteed
to be available in any version of Emacs to date, just like D-Bus or X.

The only benefit of the macro approach is zero overhead - no fboundp
checks at load time, and no indirection in calling jsonrpc--json-read.
But those should be negligible costs, and macros come with their slew of
drawbacks that makes them unnecessary in this simple case.

>>> No idea how to check if byte-code is "valid" or not: I just check the
>>> warnings.  Can you tell me?
>> 
>> 0. emacs -Q -batch -f batch-byte-compile foo.el
>> 1. emacs -Q
>> 2. (fset 'json-parse-buffer nil) C-j
>> 3. M-x load-file RET foo.elc RET
>> 4. (disassemble 'foo) C-j
>
> Thanks.
>
>> I think the declarations make the intention explicit to both the reader
>> and the byte-compiler in a simple way, without wrestling the
>> eval-*-compile machinery or allowing for subtle bugs like the ones
>> above.
>
> The problem, of course, is that you're repeating yourself, a maintenance
> hazard.  Not too big in this case.

Agreed.

-- 
Basil




Information forwarded to bug-gnu-emacs <at> gnu.org:
bug#40693; Package emacs. (Sat, 23 May 2020 22:46:02 GMT) Full text and rfc822 format available.

Message #128 received at 40693 <at> debbugs.gnu.org (full text, mbox):

From: João Távora <joaotavora <at> gmail.com>
To: "Basil L. Contovounesios" <contovob <at> tcd.ie>
Cc: darthandrus <at> gmail.com, 40693 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Dmitry Gutov <dgutov <at> yandex.ru>
Subject: Re: bug#40693: 28.0.50; json-encode-alist changes alist
Date: Sat, 23 May 2020 23:45:10 +0100
"Basil L. Contovounesios" <contovob <at> tcd.ie> writes:

> Yes.
>
>> Anyway, this is true if you want 27.1 elc's to be loadable in 26.x.
>
> It is also true if you want version N .elc files to be loadable in
> version N.  The problems I list are not specific to either JSON or
> inter-Emacs-version compat.

You're right, I forgot about --without-json/--with-json builds.

> The only benefit of the macro approach is zero overhead - no fboundp
> checks at load time, and no indirection in calling jsonrpc--json-read.
> But those should be negligible costs, and macros come with their slew of
> drawbacks that makes them unnecessary in this simple case.

Agree.  And macros don't solve the intercompatibility problems.

All good, thanks for explaining!

João





bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 21 Jun 2020 11:24:09 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 282 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.