bug#40693: 28.0.50; json-encode-alist changes alist

bug#40693: 28.0.50; json-encode-alist changes alist

[Ivan Andrus]

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)

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Ivan Andrus]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Michael Heerdegen]

Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Michael Heerdegen]

Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

Michael Heerdegen <michael_heerdegen@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

Dmitry Gutov <dgutov@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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: json-tests.diff --]
[-- Type: text/x-diff, Size: 1010 bytes --]

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

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


Eli, would this be okay for emacs-27?

Thanks,

-- 
Basil

bug#40693: 28.0.50; json-encode-alist changes alist

[Paul Eggert]

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

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

[-- Attachment #2: 0001-Document-that-quoting-yields-constants.patch --]
[-- Type: text/x-patch, Size: 1551 bytes --]

From 81e7d7f111872c9f2aaf8885db50a22ed746d7b5 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 19 Apr 2020 09:37:51 -0700
Subject: [PATCH] Document that quoting yields constants

* doc/lispref/eval.texi (Quoting, Backquote):
Mention that quoted expressions yield a constant (Bug#40693).
---
 doc/lispref/eval.texi | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/doc/lispref/eval.texi b/doc/lispref/eval.texi
index f6f36ed342..46cfab164b 100644
--- a/doc/lispref/eval.texi
+++ b/doc/lispref/eval.texi
@@ -558,6 +558,7 @@ and vectors.)
 
 @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
 
 @cindex @samp{'} for quoting
@@ -612,10 +613,12 @@ only part of a list, while computing and substituting other parts.
 
   @dfn{Backquote constructs} allow you to quote a list, but
 selectively evaluate elements of that list.  In the simplest case, it
-is identical to the special form @code{quote}
+is identical to the special form
 @iftex
+@code{quote}.
 @end iftex
 @ifnottex
+@code{quote}
 (described in the previous section; @pxref{Quoting}).
 @end ifnottex
 For example, these two forms yield identical results:
@@ -693,6 +696,9 @@ Here are some examples:
 @end group
 @end example
 
+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.
 
 @node Eval
 @section Eval
-- 
2.17.1

bug#40693: 28.0.50; json-encode-alist changes alist

[Drew Adams]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Paul Eggert]

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

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.

[-- Attachment #2: 0001-Fix-mutability-glitches-reported-by-Drew-Adams.patch --]
[-- Type: text/x-patch, Size: 3444 bytes --]

From e1d42da0d686e93534ee2abebe79bff95f18cb4d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sun, 19 Apr 2020 15:09:02 -0700
Subject: [PATCH] Fix mutability glitches reported by Drew Adams

See Bug#40693#32.
* doc/lispref/eval.texi (Self-Evaluating Forms, Backquote):
Say that these yield constant conses, vectors and strings,
not constant symbols.
* doc/lispref/objects.texi (Constants and Mutability): Say that an
attempt to modify a constant variable signals an error, instead of
saying that it has undefined behavior.
---
 doc/lispref/eval.texi    |  8 ++++----
 doc/lispref/objects.texi | 10 ++++++----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/doc/lispref/eval.texi b/doc/lispref/eval.texi
index deb288943a..f33c2faac1 100644
--- a/doc/lispref/eval.texi
+++ b/doc/lispref/eval.texi
@@ -158,8 +158,8 @@ contents unchanged.
 @end group
 @end example
 
-  A self-evaluating form yields a constant, and you should not attempt
-to modify the constant's contents via @code{setcar}, @code{aset} or
+  A self-evaluating form yields constant conses, vectors and strings, and you
+should not attempt to modify their contents via @code{setcar}, @code{aset} or
 similar primitives.  The Lisp interpreter might unify the constants
 yielded by your program's self-evaluating forms, so that these
 constants might share structure.  @xref{Constants and Mutability}.
@@ -704,8 +704,8 @@ Here are some examples:
 @end example
 
 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.
+splices, it acts like @code{quote} in that it yields constant conses,
+vectors and strings that should not be modified.
 
 @node Eval
 @section Eval
diff --git a/doc/lispref/objects.texi b/doc/lispref/objects.texi
index 98b001afd2..b45eb7ad8a 100644
--- a/doc/lispref/objects.texi
+++ b/doc/lispref/objects.texi
@@ -2395,17 +2395,19 @@ somewhere else.
 
   Although numbers are always constants and markers are always
 mutable, some types contain both constant and mutable members.  These
-types include conses, vectors, and strings.  For example, the string
+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
 resulting behavior is undefined: the Lisp interpreter might or might
 not detect the error, and if it does not detect the error the
 interpreter can behave unpredictably thereafter.  Another way to put
-this is that mutable objects are safe to change, whereas constants are
-not safely mutable: if you try to change a constant your program might
+this is that although mutable objects are safe to change and constant
+symbols reliably reject attempts to change them, other constants are
+not safely mutable: if you try to change one your program might
 behave as you expect but it might crash or worse.  This problem occurs
 with types that have both constant and mutable members, and that have
 mutators like @code{setcar} and @code{aset} that are valid on mutable
-- 
2.17.1

bug#40693: 28.0.50; json-encode-alist changes alist

[Michael Heerdegen]

Paul Eggert <eggert@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Paul Eggert]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Michael Heerdegen]

Paul Eggert <eggert@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Paul Eggert]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Michael Heerdegen]

Paul Eggert <eggert@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Paul Eggert]

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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Drew Adams]

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

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Sun, 19 Apr 2020 19:55:47 -0700
> Cc: Ivan Andrus <darthandrus@gmail.com>, 40693@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?

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

> Dmitry Gutov <dgutov@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Wed, 29 Apr 2020 11:11:38 +0100
> Cc: Ivan Andrus <darthandrus@gmail.com>, 40693@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

On 29.04.2020 13:30, Eli Zaretskii wrote:
>> From: "Basil L. Contovounesios" <contovob@tcd.ie>
>> Date: Wed, 29 Apr 2020 11:11:38 +0100
>> Cc: Ivan Andrus <darthandrus@gmail.com>, 40693@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> Cc: darthandrus@gmail.com, 40693@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

On 29.04.2020 15:21, Eli Zaretskii wrote:
>> Cc: darthandrus@gmail.com, 40693@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> Cc: contovob@tcd.ie, darthandrus@gmail.com, 40693@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: darthandrus@gmail.com, 40693@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

On 29.04.2020 17:40, Eli Zaretskii wrote:
>> Cc: contovob@tcd.ie, darthandrus@gmail.com, 40693@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> Cc: contovob@tcd.ie, darthandrus@gmail.com, 40693@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

tags 40693 + patch
quit


[-- Attachment #2: 0001-Various-json.el-improvements.patch --]
[-- Type: text/x-diff, Size: 93301 bytes --]

From 74215f7c5eeb414273afa9dc2c3f51a47c7b6e75 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sat, 16 May 2020 13:23:48 +0100
Subject: [PATCH] Various json.el improvements

* etc/NEWS: Announce that json-read-number is now stricter.

* json.el: Bump package version.
(json-encoding-lisp-style-closings, json-pre-element-read-function)
(json-post-element-read-function, json-advance, json-peek)
(json--path): Clarify and improve style of doc strings.
(json-join): Define as an obsolete alias of string-join.
(json-alist-p, json-plist-p): Refactor for speed and declare as
pure, side-effect-free, and error-free.
(json--plist-reverse): Rename function...
(json--plist-nreverse): ...to this, making it destructive for speed.
All callers changed.
(json--plist-to-alist): Remove, replacing single use with map-pairs.
(json--with-indentation): Accept multiple forms as arguments, fix
their indentation, and allow them to be instrumented for debugging.
Add docstring.
(json-pop, json-read-keyword, json-add-to-object)
(json-encode-array): Simplify for speed.
(json-skip-whitespace): Put newline before carriage return for
likely frequency of occurrence, and so that the characters appear in
increasing order.
(json--check-position): Use 1+.
(json-path-to-position): Open code apply-partially.
(json-keywords): Turn into a defconst and mark as obsolete now that
it is no longer used.
(json--post-value, json--number, json--escape): New rx definitions.
(json-encode-keyword): Declare as side-effect-free.
(json-read-number): Reject leading zeros and plus signs, and make
integer part mandatory in accordance with JSON standards and for
consistency with native JSON parsing functions.  Eagerly signal
json-number-format when garbage follows a valid number, e.g., when
reading "1.1.1", instead of leaving that up to the caller.  Remove
optional internal argument from advertised calling convention now
that the function is no longer recursive.
(json-encode-number): Define as an alias of number-to-string.
(json-special-chars): Turn into a defconst.
(json-read-escaped-char, json-new-object, json-read-file)
(json-pretty-print): Simplify.
(json-read-string): For consistency with other json.el error
reporting, remove check for leading '"', and use the integer value
rather than the printed representation of characters in error data.
At EOB signal json-end-of-file instead of json-string-format.
(json--long-string-threshold, json--string-buffer): New variables.
(json-encode-string): Reimplement in terms of buffer manipulation
for speed (bug#20154).
(json-read-object): Escape ?\} properly.
(json--encode-alist): New function extracted from json-encode-alist.
(json-encode-hash-table, json-encode-alist, json-encode-plist): Use
it to avoid destructively modifying the argument when
json-encoding-object-sort-predicate is non-nil without incurring
unnecessary copying (bug#40693).  Encode empty object as "{}" even
when pretty-printing.  Simplify for speed.
(json-read-array): Avoid recomputing list length on each iteration
when json-pre-element-read-function is non-nil.  Make first element
of json-array-format error data a string for consistency with
json-object-format and to make the displayed error message clearer.
(json-readtable-dispatch): Accept any kind of argument, not just
symbols.  Generate the table in a simpler manner so the dispatch
order is clearer.  Remove dispatch on ?+ and ?. now that
json-read-number is stricter and for consistency with native JSON
parsing functions.  Signal json-end-of-file if argument is nil.
(json-read): Simplify accordingly.
(json-encode): Avoid allocating a list on each invocation.

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

* lisp/progmodes/python.el (python--parse-json-array): New function.
(python-shell-prompt-detect): Use it to parse JSON directly as a
list rather than converting from a vector.

* test/lisp/json-tests.el (json-tests--with-temp-buffer): Allow
instrumenting for debugging.
(test-json-join, test-json-plist-to-alist): Remove tests.

(test-json-alist-p, test-json-plist-p, test-json-advance)
(test-json-peek, test-json-pop, test-json-skip-whitespace)
(test-json-read-keyword, test-json-encode-keyword)
(test-json-encode-number, test-json-read-escaped-char)
(test-json-read-string, test-json-encode-string)
(test-json-encode-key, test-json-new-object)
(test-json-encode-hash-table, test-json-encode-plist)
(test-json-encode-list, test-json-read-array)
(test-json-encode-array, test-json-read)
(test-json-read-from-string, test-json-encode): Extend tests.

(test-json-plist-reverse): Rename test...
(test-json-plist-nreverse): ...to this and avoid modifying literal
lists.
(test-json-read-number): Rename test...
(test-json-read-integer): ...to this, focusing on integers.
(test-json-add-to-object): Rename test...
(test-json-add-to-alist): ...to this, focusing on alists.
(json-encode-simple-alist): Rename test...
(test-json-encode-alist): ...to this, extending it.
(test-json-encode-alist-with-sort-predicate): Rename test...
(test-json-encode-alist-sort): ...to this, extending it.
(test-json-encode-plist-with-sort-predicate): Rename test...
(test-json-encode-plist-sort): ...to this, extending it.

(test-json-read-keyword-invalid, test-json-read-fraction)
(test-json-read-exponent, test-json-read-fraction-exponent)
(test-json-read-number-invalid)
(test-json-read-escaped-char-invalid, test-json-add-to-plist)
(test-json-add-to-hash-table, test-json-read-object-empty)
(test-json-read-object-invalid, test-json-read-object-function)
(test-json-encode-hash-table-pretty)
(test-json-encode-hash-table-lisp-style)
(test-json-encode-hash-table-sort, test-json-encode-alist-pretty)
(test-json-encode-alist-lisp-style, test-json-encode-plist-pretty)
(test-json-encode-plist-lisp-style, test-json-read-array-function)
(test-json-encode-array-pretty, test-json-encode-array-lisp-style)
(test-json-read-invalid): New tests.

(test-json-path-to-position-no-match): Use should-not.
(test-json-read-object): Move error check to new test
test-json-read-object-invalid.
(test-json-pretty-print-object): Adapt test now that empty objects
are pretty-printed as "{}".
---
 etc/NEWS                 |   9 +
 lisp/json.el             | 569 +++++++++++++-------------
 lisp/jsonrpc.el          |  48 ++-
 lisp/progmodes/python.el |  21 +-
 test/lisp/json-tests.el  | 857 ++++++++++++++++++++++++++++++++-------
 5 files changed, 1054 insertions(+), 450 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 303036ece3..e35ddddf9b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -353,6 +353,15 @@ symbol property to the browsing functions.  With a new command
 'browse-url-with-browser-kind', an URL can explicitly be browsed with
 either an internal or external browser.
 
+** json.el
+
+---
+*** JSON number parsing is now stricter.
+Numbers with a leading plus sign, leading zeros, or a missing integer
+component are now rejected by 'json-read' and friends.  This makes
+them more compliant with the JSON specification and consistent with
+the native JSON parsing functions.
+
 \f
 * New Modes and Packages in Emacs 28.1
 
diff --git a/lisp/json.el b/lisp/json.el
index 6f3b791ed1..93874a2cb8 100644
--- a/lisp/json.el
+++ b/lisp/json.el
@@ -3,7 +3,7 @@
 ;; Copyright (C) 2006-2020 Free Software Foundation, Inc.
 
 ;; Author: Theresa O'Connor <ted@oconnor.cx>
-;; Version: 1.4
+;; Version: 1.5
 ;; Keywords: convenience
 
 ;; This file is part of GNU Emacs.
@@ -29,11 +29,11 @@
 ;; Learn all about JSON here: <URL:http://json.org/>.
 
 ;; The user-serviceable entry points for the parser are the functions
-;; `json-read' and `json-read-from-string'. The encoder has a single
+;; `json-read' and `json-read-from-string'.  The encoder has a single
 ;; entry point, `json-encode'.
 
 ;; Since there are several natural representations of key-value pair
-;; mappings in elisp (alist, plist, hash-table), `json-read' allows you
+;; mappings in Elisp (alist, plist, hash-table), `json-read' allows you
 ;; to specify which you'd prefer (see `json-object-type' and
 ;; `json-array-type').
 
@@ -113,8 +113,10 @@ json-encoding-pretty-print
   "If non-nil, then the output of `json-encode' will be pretty-printed.")
 
 (defvar json-encoding-lisp-style-closings nil
-  "If non-nil, ] and } closings will be formatted lisp-style,
-without indentation.")
+  "If non-nil, delimiters ] and } will be formatted Lisp-style.
+This means they will be placed on the same line as the last
+element of the respective array or object, without indentation.
+Used only when `json-encoding-pretty-print' is non-nil.")
 
 (defvar json-encoding-object-sort-predicate nil
   "Sorting predicate for JSON object keys during encoding.
@@ -124,88 +126,81 @@ json-encoding-object-sort-predicate
 ordered alphabetically.")
 
 (defvar json-pre-element-read-function nil
-  "Function called (if non-nil) by `json-read-array' and
-`json-read-object' right before reading a JSON array or object,
-respectively.  The function is called with one argument, which is
-the current JSON key.")
+  "If non-nil, a function to call before reading a JSON array or object.
+It is called by `json-read-array' and `json-read-object',
+respectively, with one argument, which is the current JSON key.")
 
 (defvar json-post-element-read-function nil
-  "Function called (if non-nil) by `json-read-array' and
-`json-read-object' right after reading a JSON array or object,
-respectively.")
+  "If non-nil, a function to call after reading a JSON array or object.
+It is called by `json-read-array' and `json-read-object',
+respectively, with no arguments.")
 
 \f
 
 ;;; Utilities
 
-(defun json-join (strings separator)
-  "Join STRINGS with SEPARATOR."
-  (mapconcat 'identity strings separator))
+(define-obsolete-function-alias 'json-join #'string-join "28.1")
 
 (defun json-alist-p (list)
-  "Non-null if and only if LIST is an alist with simple keys."
-  (while (consp list)
-    (setq list (if (and (consp (car list))
-                        (atom (caar list)))
-                   (cdr list)
-                 'not-alist)))
+  "Non-nil if and only if LIST is an alist with simple keys."
+  (declare (pure t) (side-effect-free error-free))
+  (while (and (consp (car-safe list))
+              (atom (caar list))
+              (setq list (cdr list))))
   (null list))
 
 (defun json-plist-p (list)
-  "Non-null if and only if LIST is a plist with keyword keys."
-  (while (consp list)
-    (setq list (if (and (keywordp (car list))
-                        (consp (cdr list)))
-                   (cddr list)
-                 'not-plist)))
+  "Non-nil if and only if LIST is a plist with keyword keys."
+  (declare (pure t) (side-effect-free error-free))
+  (while (and (keywordp (car-safe list))
+              (consp (cdr list))
+              (setq list (cddr list))))
   (null list))
 
-(defun json--plist-reverse (plist)
-  "Return a copy of PLIST in reverse order.
-Unlike `reverse', this keeps the property-value pairs intact."
-  (let (res)
-    (while plist
-      (let ((prop (pop plist))
-            (val (pop plist)))
-        (push val res)
-        (push prop res)))
-    res))
+(defun json--plist-nreverse (plist)
+  "Return PLIST in reverse order.
+Unlike `nreverse', this keeps the ordering of each property
+relative to its value intact.  Like `nreverse', this function may
+destructively modify PLIST to produce the result."
+  (let (prev (next (cddr plist)))
+    (while next
+      (setcdr (cdr plist) prev)
+      (setq prev plist plist next next (cddr next))
+      (setcdr (cdr plist) prev)))
+  plist)
 
-(defun json--plist-to-alist (plist)
-  "Return an alist of the property-value pairs in PLIST."
-  (let (res)
-    (while plist
-      (let ((prop (pop plist))
-            (val (pop plist)))
-        (push (cons prop val) res)))
-    (nreverse res)))
-
-(defmacro json--with-indentation (body)
+(defmacro json--with-indentation (&rest body)
+  "Evaluate BODY with the correct indentation for JSON encoding.
+This macro binds `json--encoding-current-indentation' according
+to `json-encoding-pretty-print' around BODY."
+  (declare (debug t) (indent 0))
   `(let ((json--encoding-current-indentation
           (if json-encoding-pretty-print
               (concat json--encoding-current-indentation
                       json-encoding-default-indentation)
             "")))
-     ,body))
+     ,@body))
 
 ;; Reader utilities
 
 (define-inline json-advance (&optional n)
-  "Advance N characters forward."
+  "Advance N characters forward, or 1 character if N is nil.
+On reaching the end of the accessible region of the buffer, stop
+and signal an error."
   (inline-quote (forward-char ,n)))
 
 (define-inline json-peek ()
-  "Return the character at point."
+  "Return the character at point.
+At the end of the accessible region of the buffer, return 0."
   (inline-quote (following-char)))
 
 (define-inline json-pop ()
-  "Advance past the character at point, returning it."
+  "Advance past the character at point, returning it.
+Signal `json-end-of-file' if called at the end of the buffer."
   (inline-quote
-   (let ((char (json-peek)))
-     (if (zerop char)
-         (signal 'json-end-of-file nil)
-       (json-advance)
-       char))))
+   (prog1 (or (char-after)
+              (signal 'json-end-of-file ()))
+     (json-advance))))
 
 (define-inline json-skip-whitespace ()
   "Skip past the whitespace at point."
@@ -213,7 +208,7 @@ json-skip-whitespace
   ;; https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf
   ;; or https://tools.ietf.org/html/rfc7159#section-2 for the
   ;; definition of whitespace in JSON.
-  (inline-quote (skip-chars-forward "\t\r\n ")))
+  (inline-quote (skip-chars-forward "\t\n\r ")))
 
 \f
 
@@ -236,8 +231,8 @@ 'json-end-of-file
 ;;; Paths
 
 (defvar json--path '()
-  "Used internally by `json-path-to-position' to keep track of
-the path during recursive calls to `json-read'.")
+  "Keeps track of the path during recursive calls to `json-read'.
+Used internally by `json-path-to-position'.")
 
 (defun json--record-path (key)
   "Record the KEY to the current JSON path.
@@ -248,7 +243,7 @@ json--check-position
   "Check if the last parsed JSON structure passed POSITION.
 Used internally by `json-path-to-position'."
   (let ((start (caar json--path)))
-    (when (< start position (+ (point) 1))
+    (when (< start position (1+ (point)))
       (throw :json-path (list :path (nreverse (mapcar #'cdr json--path))
                               :match-start start
                               :match-end (point)))))
@@ -266,13 +261,13 @@ json-path-to-position
 :path        -- A list of strings and numbers forming the path to
                 the JSON element at the given position.  Strings
                 denote object names, while numbers denote array
-                indexes.
+                indices.
 
 :match-start -- Position where the matched JSON element begins.
 
 :match-end   -- Position where the matched JSON element ends.
 
-This can for instance be useful to determine the path to a JSON
+This can, for instance, be useful to determine the path to a JSON
 element in a deeply nested structure."
   (save-excursion
     (unless string
@@ -280,7 +275,7 @@ json-path-to-position
     (let* ((json--path '())
            (json-pre-element-read-function #'json--record-path)
            (json-post-element-read-function
-            (apply-partially #'json--check-position position))
+            (lambda () (json--check-position position)))
            (path (catch :json-path
                    (if string
                        (json-read-from-string string)
@@ -290,38 +285,33 @@ json-path-to-position
 
 ;;; Keywords
 
-(defvar json-keywords '("true" "false" "null")
+(defconst json-keywords '("true" "false" "null")
   "List of JSON keywords.")
+(make-obsolete-variable 'json-keywords "it is no longer used." "28.1")
 
 ;; Keyword parsing
 
+;; Characters that can follow a JSON value.
+(rx-define json--post-value (| (in "\t\n\r ,]}") eos))
+
 (defun json-read-keyword (keyword)
-  "Read a JSON keyword at point.
-KEYWORD is the keyword expected."
-  (unless (member keyword json-keywords)
-    (signal 'json-unknown-keyword (list keyword)))
-  (mapc (lambda (char)
-          (when (/= char (json-peek))
-            (signal 'json-unknown-keyword
-                    (list (save-excursion
-                            (backward-word-strictly 1)
-                            (thing-at-point 'word)))))
-          (json-advance))
-        keyword)
-  (json-skip-whitespace)
-  (unless (looking-at "\\([],}]\\|$\\)")
-    (signal 'json-unknown-keyword
-            (list (save-excursion
-                    (backward-word-strictly 1)
-                    (thing-at-point 'word)))))
-  (cond ((string-equal keyword "true") t)
-        ((string-equal keyword "false") json-false)
-        ((string-equal keyword "null") json-null)))
+  "Read the expected JSON KEYWORD at point."
+  (prog1 (cond ((equal keyword "true")  t)
+               ((equal keyword "false") json-false)
+               ((equal keyword "null")  json-null)
+               (t (signal 'json-unknown-keyword (list keyword))))
+    (or (looking-at-p keyword)
+        (signal 'json-unknown-keyword (list (thing-at-point 'word))))
+    (json-advance (length keyword))
+    (or (looking-at-p (rx json--post-value))
+        (signal 'json-unknown-keyword (list (thing-at-point 'word))))
+    (json-skip-whitespace)))
 
 ;; Keyword encoding
 
 (defun json-encode-keyword (keyword)
   "Encode KEYWORD as a JSON value."
+  (declare (side-effect-free t))
   (cond ((eq keyword t)          "true")
         ((eq keyword json-false) "false")
         ((eq keyword json-null)  "null")))
@@ -330,37 +320,31 @@ json-encode-keyword
 
 ;; Number parsing
 
-(defun json-read-number (&optional sign)
- "Read the JSON number following point.
-The optional SIGN argument is for internal use.
+(rx-define json--number
+  (: (? ?-)                                   ; Sign.
+     (| (: (in "1-9") (* digit)) ?0)          ; Integer.
+     (? ?. (+ digit))                         ; Fraction.
+     (? (in "Ee") (? (in ?+ ?-)) (+ digit)))) ; Exponent.
 
-N.B.: Only numbers which can fit in Emacs Lisp's native number
-representation will be parsed correctly."
- ;; If SIGN is non-nil, the number is explicitly signed.
- (let ((number-regexp
-        "\\([0-9]+\\)?\\(\\.[0-9]+\\)?\\([Ee][+-]?[0-9]+\\)?"))
-   (cond ((and (null sign) (= (json-peek) ?-))
-          (json-advance)
-          (- (json-read-number t)))
-         ((and (null sign) (= (json-peek) ?+))
-          (json-advance)
-          (json-read-number t))
-         ((and (looking-at number-regexp)
-               (or (match-beginning 1)
-                   (match-beginning 2)))
-          (goto-char (match-end 0))
-          (string-to-number (match-string 0)))
-         (t (signal 'json-number-format (list (point)))))))
+(defun json-read-number (&optional _sign)
+  "Read the JSON number following point."
+  (declare (advertised-calling-convention () "28.1"))
+  (or (looking-at (rx json--number))
+      (signal 'json-number-format (list (point))))
+  (goto-char (match-end 0))
+  (prog1 (string-to-number (match-string 0))
+    (or (looking-at-p (rx json--post-value))
+        (signal 'json-number-format (list (point))))
+    (json-skip-whitespace)))
 
 ;; Number encoding
 
-(defun json-encode-number (number)
-  "Return a JSON representation of NUMBER."
-  (format "%s" number))
+(defalias 'json-encode-number #'number-to-string
+  "Return a JSON representation of NUMBER.")
 
 ;;; Strings
 
-(defvar json-special-chars
+(defconst json-special-chars
   '((?\" . ?\")
     (?\\ . ?\\)
     (?b . ?\b)
@@ -368,7 +352,7 @@ json-special-chars
     (?n . ?\n)
     (?r . ?\r)
     (?t . ?\t))
-  "Characters which are escaped in JSON, with their elisp counterparts.")
+  "Characters which are escaped in JSON, with their Elisp counterparts.")
 
 ;; String parsing
 
@@ -378,48 +362,47 @@ json--decode-utf-16-surrogates
 
 (defun json-read-escaped-char ()
   "Read the JSON string escaped character at point."
-  ;; Skip over the '\'
+  ;; Skip over the '\'.
   (json-advance)
-  (let* ((char (json-pop))
-         (special (assq char json-special-chars)))
+  (let ((char (json-pop)))
     (cond
-     (special (cdr special))
-     ((not (eq char ?u)) char)
+     ((cdr (assq char json-special-chars)))
+     ((/= char ?u) char)
      ;; Special-case UTF-16 surrogate pairs,
      ;; cf. <https://tools.ietf.org/html/rfc7159#section-7>.  Note that
      ;; this clause overlaps with the next one and therefore has to
      ;; come first.
      ((looking-at
-       (rx (group (any "Dd") (any "89ABab") (= 2 (any xdigit)))
-           "\\u" (group (any "Dd") (any "C-Fc-f") (= 2 (any xdigit)))))
+       (rx (group (any "Dd") (any "89ABab") (= 2 xdigit))
+           "\\u" (group (any "Dd") (any "C-Fc-f") (= 2 xdigit))))
       (json-advance 10)
       (json--decode-utf-16-surrogates
        (string-to-number (match-string 1) 16)
        (string-to-number (match-string 2) 16)))
      ((looking-at (rx (= 4 xdigit)))
-      (let ((hex (match-string 0)))
-        (json-advance 4)
-        (string-to-number hex 16)))
+      (json-advance 4)
+      (string-to-number (match-string 0) 16))
      (t
       (signal 'json-string-escape (list (point)))))))
 
 (defun json-read-string ()
   "Read the JSON string at point."
-  (unless (= (json-peek) ?\")
-    (signal 'json-string-format (list "doesn't start with `\"'!")))
-  ;; Skip over the '"'
+  ;; Skip over the '"'.
   (json-advance)
   (let ((characters '())
         (char (json-peek)))
-    (while (not (= char ?\"))
+    (while (/= char ?\")
       (when (< char 32)
-        (signal 'json-string-format (list (prin1-char char))))
+        (if (zerop char)
+            (signal 'json-end-of-file ())
+          (signal 'json-string-format (list char))))
       (push (if (= char ?\\)
                 (json-read-escaped-char)
-              (json-pop))
+              (json-advance)
+              char)
             characters)
       (setq char (json-peek)))
-    ;; Skip over the '"'
+    ;; Skip over the '"'.
     (json-advance)
     (if characters
         (concat (nreverse characters))
@@ -427,29 +410,47 @@ json-read-string
 
 ;; String encoding
 
+;; Escape only quotation mark, backslash, and the control
+;; characters U+0000 to U+001F (RFC 4627, ECMA-404).
+(rx-define json--escape (in ?\" ?\\ cntrl))
+
+(defvar json--long-string-threshold 200
+  "Length above which strings are considered long for JSON encoding.
+It is generally faster to manipulate such strings in a buffer
+rather than directly.")
+
+(defvar json--string-buffer nil
+  "Buffer used for encoding Lisp strings as JSON.
+Initialized lazily by `json-encode-string'.")
+
 (defun json-encode-string (string)
   "Return a JSON representation of STRING."
-  ;; Reimplement the meat of `replace-regexp-in-string', for
-  ;; performance (bug#20154).
-  (let ((l (length string))
-        (start 0)
-        res mb)
-    ;; Only escape quotation mark, backslash and the control
-    ;; characters U+0000 to U+001F (RFC 4627, ECMA-404).
-    (while (setq mb (string-match "[\"\\[:cntrl:]]" string start))
-      (let* ((c (aref string mb))
-             (special (rassq c json-special-chars)))
-        (push (substring string start mb) res)
-        (push (if special
-                  ;; Special JSON character (\n, \r, etc.).
-                  (string ?\\ (car special))
-                ;; Fallback: UCS code point in \uNNNN form.
-                (format "\\u%04x" c))
-              res)
-        (setq start (1+ mb))))
-    (push (substring string start l) res)
-    (push "\"" res)
-    (apply #'concat "\"" (nreverse res))))
+  ;; Try to avoid buffer overhead in trivial cases, while also
+  ;; avoiding searching pathological strings for escape characters.
+  ;; Since `string-match-p' doesn't take a LIMIT argument, we use
+  ;; string length as our heuristic.  See also bug#20154.
+  (if (and (< (length string) json--long-string-threshold)
+           (not (string-match-p (rx json--escape) string)))
+      (concat "\"" string "\"")
+    (with-current-buffer
+        (or json--string-buffer
+            (with-current-buffer (generate-new-buffer " *json-string*")
+              ;; This seems to afford decent performance gains.
+              (setq-local inhibit-modification-hooks t)
+              (setq json--string-buffer (current-buffer))))
+      (insert ?\" string)
+      (goto-char (1+ (point-min)))
+      (while (re-search-forward (rx json--escape) nil 'move)
+        (let ((char (preceding-char)))
+          (delete-char -1)
+          (insert ?\\ (or
+                       ;; Special JSON character (\n, \r, etc.).
+                       (car (rassq char json-special-chars))
+                       ;; Fallback: UCS code point in \uNNNN form.
+                       (format "u%04x" char)))))
+      (insert ?\")
+      ;; Empty buffer for next invocation.
+      (delete-and-extract-region (point-min) (point-max)))))
 
 (defun json-encode-key (object)
   "Return a JSON representation of OBJECT.
@@ -460,15 +461,13 @@ json-encode-key
       (signal 'json-key-format (list object)))
     encoded))
 
-;;; JSON Objects
+;;; Objects
 
 (defun json-new-object ()
-  "Create a new Elisp object corresponding to a JSON object.
+  "Create a new Elisp object corresponding to an empty JSON object.
 Please see the documentation of `json-object-type'."
-  (cond ((eq json-object-type 'hash-table)
-         (make-hash-table :test 'equal))
-        (t
-         ())))
+  (and (eq json-object-type 'hash-table)
+       (make-hash-table :test #'equal)))
 
 (defun json-add-to-object (object key value)
   "Add a new KEY -> VALUE association to OBJECT.
@@ -476,10 +475,10 @@ json-add-to-object
     (setq obj (json-add-to-object obj \"foo\" \"bar\"))
 Please see the documentation of `json-object-type' and `json-key-type'."
   (let ((json-key-type
-         (or json-key-type
-             (cdr (assq json-object-type '((hash-table . string)
-                                           (alist . symbol)
-                                           (plist . keyword)))))))
+         (cond (json-key-type)
+               ((eq json-object-type 'hash-table) 'string)
+               ((eq json-object-type 'alist)      'symbol)
+               ((eq json-object-type 'plist)      'keyword))))
     (setq key
           (cond ((eq json-key-type 'string)
                  key)
@@ -499,13 +498,13 @@ json-add-to-object
 
 (defun json-read-object ()
   "Read the JSON object at point."
-  ;; Skip over the "{"
+  ;; Skip over the '{'.
   (json-advance)
   (json-skip-whitespace)
-  ;; read key/value pairs until "}"
+  ;; Read key/value pairs until '}'.
   (let ((elements (json-new-object))
         key value)
-    (while (not (= (json-peek) ?}))
+    (while (/= (json-peek) ?\})
       (json-skip-whitespace)
       (setq key (json-read-string))
       (json-skip-whitespace)
@@ -520,94 +519,94 @@ json-read-object
         (funcall json-post-element-read-function))
       (setq elements (json-add-to-object elements key value))
       (json-skip-whitespace)
-      (when (/= (json-peek) ?})
+      (when (/= (json-peek) ?\})
         (if (= (json-peek) ?,)
             (json-advance)
           (signal 'json-object-format (list "," (json-peek))))))
-    ;; Skip over the "}"
+    ;; Skip over the '}'.
     (json-advance)
     (pcase json-object-type
       ('alist (nreverse elements))
-      ('plist (json--plist-reverse elements))
+      ('plist (json--plist-nreverse elements))
       (_ elements))))
 
 ;; Hash table encoding
 
 (defun json-encode-hash-table (hash-table)
   "Return a JSON representation of HASH-TABLE."
-  (if json-encoding-object-sort-predicate
-      (json-encode-alist (map-into hash-table 'list))
-    (format "{%s%s}"
-            (json-join
-             (let (r)
-               (json--with-indentation
-                (maphash
-                 (lambda (k v)
-                   (push (format
-                          (if json-encoding-pretty-print
-                              "%s%s: %s"
-                            "%s%s:%s")
-                          json--encoding-current-indentation
-                          (json-encode-key k)
-                          (json-encode v))
-                         r))
-                 hash-table))
-               r)
-             json-encoding-separator)
-            (if (or (not json-encoding-pretty-print)
-                    json-encoding-lisp-style-closings)
-                ""
-              json--encoding-current-indentation))))
+  (cond ((hash-table-empty-p hash-table) "{}")
+        (json-encoding-object-sort-predicate
+         (json--encode-alist (map-pairs hash-table) t))
+        (t
+         (let ((kv-sep (if json-encoding-pretty-print ": " ":"))
+               result)
+           (json--with-indentation
+             (maphash
+              (lambda (k v)
+                (push (concat json--encoding-current-indentation
+                              (json-encode-key k)
+                              kv-sep
+                              (json-encode v))
+                      result))
+              hash-table))
+           (concat "{"
+                   (string-join (nreverse result) json-encoding-separator)
+                   (and json-encoding-pretty-print
+                        (not json-encoding-lisp-style-closings)
+                        json--encoding-current-indentation)
+                   "}")))))
 
 ;; List encoding (including alists and plists)
 
-(defun json-encode-alist (alist)
-  "Return a JSON representation of ALIST."
+(defun json--encode-alist (alist &optional destructive)
+  "Return a JSON representation of ALIST.
+DESTRUCTIVE non-nil means it is safe to modify ALIST by
+side-effects."
   (when json-encoding-object-sort-predicate
-    (setq alist
-          (sort alist (lambda (a b)
+    (setq alist (sort (if destructive alist (copy-sequence alist))
+                      (lambda (a b)
                         (funcall json-encoding-object-sort-predicate
                                  (car a) (car b))))))
-  (format "{%s%s}"
-          (json-join
-           (json--with-indentation
-            (mapcar (lambda (cons)
-                      (format (if json-encoding-pretty-print
-                                  "%s%s: %s"
-                                "%s%s:%s")
-                              json--encoding-current-indentation
-                              (json-encode-key (car cons))
-                              (json-encode (cdr cons))))
-                    alist))
-           json-encoding-separator)
-          (if (or (not json-encoding-pretty-print)
-                  json-encoding-lisp-style-closings)
-              ""
-            json--encoding-current-indentation)))
+  (concat "{"
+          (let ((kv-sep (if json-encoding-pretty-print ": " ":")))
+            (json--with-indentation
+              (mapconcat (lambda (cons)
+                           (concat json--encoding-current-indentation
+                                   (json-encode-key (car cons))
+                                   kv-sep
+                                   (json-encode (cdr cons))))
+                         alist
+                         json-encoding-separator)))
+          (and json-encoding-pretty-print
+               (not json-encoding-lisp-style-closings)
+               json--encoding-current-indentation)
+          "}"))
+
+(defun json-encode-alist (alist)
+  "Return a JSON representation of ALIST."
+  (if alist (json--encode-alist alist) "{}"))
 
 (defun json-encode-plist (plist)
   "Return a JSON representation of PLIST."
-  (if json-encoding-object-sort-predicate
-      (json-encode-alist (json--plist-to-alist plist))
-    (let (result)
-      (json--with-indentation
-       (while plist
-         (push (concat
-                json--encoding-current-indentation
-                (json-encode-key (car plist))
-                (if json-encoding-pretty-print
-                    ": "
-                  ":")
-                (json-encode (cadr plist)))
+  (cond ((null plist) "{}")
+        (json-encoding-object-sort-predicate
+         (json--encode-alist (map-pairs plist) t))
+        (t
+         (let ((kv-sep (if json-encoding-pretty-print ": " ":"))
                result)
-         (setq plist (cddr plist))))
-      (concat "{"
-              (json-join (nreverse result) json-encoding-separator)
-              (if (and json-encoding-pretty-print
-                       (not json-encoding-lisp-style-closings))
-                  json--encoding-current-indentation
-                "")
-              "}"))))
+           (json--with-indentation
+             (while plist
+               (push (concat json--encoding-current-indentation
+                             (json-encode-key (pop plist))
+                             kv-sep
+                             (json-encode (pop plist)))
+                     result)))
+           (concat "{"
+                   (string-join (nreverse result) json-encoding-separator)
+                   (and json-encoding-pretty-print
+                        (not json-encoding-lisp-style-closings)
+                        json--encoding-current-indentation)
+                   "}")))))
 
 (defun json-encode-list (list)
   "Return a JSON representation of LIST.
@@ -625,15 +624,17 @@ json-encode-list
 
 (defun json-read-array ()
   "Read the JSON array at point."
-  ;; Skip over the "["
+  ;; Skip over the '['.
   (json-advance)
   (json-skip-whitespace)
-  ;; read values until "]"
-  (let (elements)
-    (while (not (= (json-peek) ?\]))
+  ;; Read values until ']'.
+  (let (elements
+        (len 0))
+    (while (/= (json-peek) ?\])
       (json-skip-whitespace)
       (when json-pre-element-read-function
-        (funcall json-pre-element-read-function (length elements)))
+        (funcall json-pre-element-read-function len)
+        (setq len (1+ len)))
       (push (json-read) elements)
       (when json-post-element-read-function
         (funcall json-post-element-read-function))
@@ -641,8 +642,8 @@ json-read-array
       (when (/= (json-peek) ?\])
         (if (= (json-peek) ?,)
             (json-advance)
-          (signal 'json-array-format (list ?, (json-peek))))))
-    ;; Skip over the "]"
+          (signal 'json-array-format (list "," (json-peek))))))
+    ;; Skip over the ']'.
     (json-advance)
     (pcase json-array-type
       ('vector (nreverse (vconcat elements)))
@@ -653,42 +654,43 @@ json-read-array
 (defun json-encode-array (array)
   "Return a JSON representation of ARRAY."
   (if (and json-encoding-pretty-print
-           (> (length array) 0))
+           (not (seq-empty-p array)))
       (concat
+       "["
        (json--with-indentation
-         (concat (format "[%s" json--encoding-current-indentation)
-                 (json-join (mapcar 'json-encode array)
-                            (format "%s%s"
-                                    json-encoding-separator
+         (concat json--encoding-current-indentation
+                 (mapconcat #'json-encode array
+                            (concat json-encoding-separator
                                     json--encoding-current-indentation))))
-       (format "%s]"
-               (if json-encoding-lisp-style-closings
-                   ""
-                 json--encoding-current-indentation)))
+       (unless json-encoding-lisp-style-closings
+         json--encoding-current-indentation)
+       "]")
     (concat "["
-            (mapconcat 'json-encode array json-encoding-separator)
+            (mapconcat #'json-encode array json-encoding-separator)
             "]")))
 
 \f
 
-;;; JSON reader.
+;;; Reader
 
 (defmacro json-readtable-dispatch (char)
-  "Dispatch reader function for CHAR."
-  (declare (debug (symbolp)))
-  (let ((table
-         '((?t json-read-keyword "true")
-           (?f json-read-keyword "false")
-           (?n json-read-keyword "null")
-           (?{ json-read-object)
-           (?\[ json-read-array)
-           (?\" json-read-string)))
-        res)
-    (dolist (c '(?- ?+ ?. ?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9))
-      (push (list c 'json-read-number) table))
-    (pcase-dolist (`(,c . ,rest) table)
-      (push `((eq ,char ,c) (,@rest)) res))
-    `(cond ,@res (t (signal 'json-readtable-error (list ,char))))))
+  "Dispatch reader function for CHAR at point.
+If CHAR is nil, signal `json-end-of-file'."
+  (declare (debug t))
+  (macroexp-let2 nil char char
+    `(cond ,@(map-apply
+              (lambda (key expr)
+                `((eq ,char ,key) ,expr))
+              `((?\" ,#'json-read-string)
+                (?\[ ,#'json-read-array)
+                (?\{ ,#'json-read-object)
+                (?n  ,#'json-read-keyword "null")
+                (?f  ,#'json-read-keyword "false")
+                (?t  ,#'json-read-keyword "true")
+                ,@(mapcar (lambda (c) (list c #'json-read-number))
+                          '(?- ?0 ?1 ?2 ?3 ?4 ?5 ?6 ?7 ?8 ?9))))
+           (,char (signal 'json-readtable-error (list ,char)))
+           (t     (signal 'json-end-of-file ())))))
 
 (defun json-read ()
   "Parse and return the JSON object following point.
@@ -706,10 +708,7 @@ json-read
          ((c . :json-false))])
    (b . \"foo\"))"
   (json-skip-whitespace)
-  (let ((char (json-peek)))
-    (if (zerop char)
-        (signal 'json-end-of-file nil)
-      (json-readtable-dispatch char))))
+  (json-readtable-dispatch (char-after)))
 
 ;; Syntactic sugar for the reader
 
@@ -724,12 +723,11 @@ json-read-file
   "Read the first JSON object contained in FILE and return it."
   (with-temp-buffer
     (insert-file-contents file)
-    (goto-char (point-min))
     (json-read)))
 
 \f
 
-;;; JSON encoder
+;;; Encoder
 
 (defun json-encode (object)
   "Return a JSON representation of OBJECT as a string.
@@ -737,20 +735,21 @@ json-encode
 OBJECT should have a structure like one returned by `json-read'.
 If an error is detected during encoding, an error based on
 `json-error' is signaled."
-  (cond ((memq object (list t json-null json-false))
-         (json-encode-keyword object))
-        ((stringp object)      (json-encode-string object))
-        ((keywordp object)     (json-encode-string
-                                (substring (symbol-name object) 1)))
-        ((listp object)        (json-encode-list object))
-        ((symbolp object)      (json-encode-string
-                                (symbol-name object)))
-        ((numberp object)      (json-encode-number object))
-        ((arrayp object)       (json-encode-array object))
-        ((hash-table-p object) (json-encode-hash-table object))
-        (t                     (signal 'json-error (list object)))))
+  (cond ((eq object t)          (json-encode-keyword object))
+        ((eq object json-null)  (json-encode-keyword object))
+        ((eq object json-false) (json-encode-keyword object))
+        ((stringp object)       (json-encode-string object))
+        ((keywordp object)      (json-encode-string
+                                 (substring (symbol-name object) 1)))
+        ((listp object)         (json-encode-list object))
+        ((symbolp object)       (json-encode-string
+                                 (symbol-name object)))
+        ((numberp object)       (json-encode-number object))
+        ((arrayp object)        (json-encode-array object))
+        ((hash-table-p object)  (json-encode-hash-table object))
+        (t                      (signal 'json-error (list object)))))
 
-;; Pretty printing & minimizing
+;;; Pretty printing & minimizing
 
 (defun json-pretty-print-buffer (&optional minimize)
   "Pretty-print current buffer.
@@ -769,9 +768,9 @@ json-pretty-print
 With prefix argument MINIMIZE, minimize it instead."
   (interactive "r\nP")
   (let ((json-encoding-pretty-print (null minimize))
-        ;; Distinguish an empty objects from 'null'
+        ;; Distinguish an empty object from 'null'.
         (json-null :json-null)
-        ;; Ensure that ordering is maintained
+        ;; Ensure that ordering is maintained.
         (json-object-type 'alist)
         (orig-buf (current-buffer))
         error)
@@ -800,9 +799,7 @@ json-pretty-print
                    ;; them.
                    (let ((space (buffer-substring
                                  (point)
-                                 (+ (point)
-                                    (skip-chars-forward
-                                     " \t\n" (point-max)))))
+                                 (+ (point) (skip-chars-forward " \t\n"))))
                          (json (json-read)))
                      (setq pos (point)) ; End of last good json-read.
                      (set-buffer tmp-buf)
@@ -832,14 +829,14 @@ json-pretty-print-buffer-ordered
   "Pretty-print current buffer with object keys ordered.
 With prefix argument MINIMIZE, minimize it instead."
   (interactive "P")
-  (let ((json-encoding-object-sort-predicate 'string<))
+  (let ((json-encoding-object-sort-predicate #'string<))
     (json-pretty-print-buffer minimize)))
 
 (defun json-pretty-print-ordered (begin end &optional minimize)
   "Pretty-print the region with object keys ordered.
 With prefix argument MINIMIZE, minimize it instead."
   (interactive "r\nP")
-  (let ((json-encoding-object-sort-predicate 'string<))
+  (let ((json-encoding-object-sort-predicate #'string<))
     (json-pretty-print begin end minimize)))
 
 (provide 'json)
diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 293dfaa748..42e7701af1 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -37,7 +37,6 @@
 ;;; Code:
 
 (require 'cl-lib)
-(require 'json)
 (require 'eieio)
 (eval-when-compile (require 'subr-x))
 (require 'warnings)
@@ -470,26 +469,35 @@ jsonrpc-stderr-buffer
 ;;;
 (define-error 'jsonrpc-error "jsonrpc-error")
 
-(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.")
 
-(defun jsonrpc--json-encode (object)
-  "Encode OBJECT into a JSON string."
-  (cond ((fboundp 'json-serialize) (json-serialize
-                                    object
-                                    :false-object :json-false
-                                    :null-object nil))
-        (t                         (let ((json-false :json-false)
-                                         (json-null nil))
-                                     (json-encode object)))))
+(defalias 'jsonrpc--json-encode
+  (if (fboundp 'json-serialize)
+      (lambda (object)
+        (json-serialize object
+                        :false-object :json-false
+                        :null-object nil))
+    (require 'json)
+    (defvar json-false)
+    (defvar json-null)
+    (declare-function json-encode "json" (object))
+    (lambda (object)
+      (let ((json-false :json-false)
+            (json-null nil))
+        (json-encode object))))
+  "Encode OBJECT into a JSON string.")
 
 (cl-defun jsonrpc--reply
     (connection id &key (result nil result-supplied-p) (error nil error-supplied-p))
diff --git a/lisp/progmodes/python.el b/lisp/progmodes/python.el
index 67383b3415..1ca9f01963 100644
--- a/lisp/progmodes/python.el
+++ b/lisp/progmodes/python.el
@@ -261,7 +261,6 @@
 (require 'ansi-color)
 (require 'cl-lib)
 (require 'comint)
-(require 'json)
 (require 'tramp-sh)
 
 ;; Avoid compiler warnings
@@ -2276,6 +2275,18 @@ python-shell--prompt-calculated-output-regexp
 Do not set this variable directly, instead use
 `python-shell-prompt-set-calculated-regexps'.")
 
+(defalias 'python--parse-json-array
+  (if (fboundp 'json-parse-string)
+      (lambda (string)
+        (json-parse-string string :array-type 'list))
+    (require 'json)
+    (defvar json-array-type)
+    (declare-function json-read-from-string "json" (string))
+    (lambda (string)
+      (let ((json-array-type 'list))
+        (json-read-from-string string))))
+  "Parse the JSON array in STRING into a Lisp list.")
+
 (defun python-shell-prompt-detect ()
   "Detect prompts for the current `python-shell-interpreter'.
 When prompts can be retrieved successfully from the
@@ -2324,11 +2335,11 @@ python-shell-prompt-detect
               (catch 'prompts
                 (dolist (line (split-string output "\n" t))
                   (let ((res
-                         ;; Check if current line is a valid JSON array
-                         (and (string= (substring line 0 2) "[\"")
+                         ;; Check if current line is a valid JSON array.
+                         (and (string-prefix-p "[\"" line)
                               (ignore-errors
-                                ;; Return prompts as a list, not vector
-                                (append (json-read-from-string line) nil)))))
+                                ;; Return prompts as a list.
+                                (python--parse-json-array line)))))
                     ;; The list must contain 3 strings, where the first
                     ;; is the input prompt, the second is the block
                     ;; prompt and the last one is the output prompt.  The
diff --git a/test/lisp/json-tests.el b/test/lisp/json-tests.el
index ac9706a8ae..a0e8c87c7b 100644
--- a/test/lisp/json-tests.el
+++ b/test/lisp/json-tests.el
@@ -21,11 +21,16 @@
 
 (require 'ert)
 (require 'json)
+(require 'map)
+(require 'seq)
+
+(eval-when-compile
+  (require 'cl-lib))
 
 (defmacro json-tests--with-temp-buffer (content &rest body)
   "Create a temporary buffer with CONTENT and evaluate BODY there.
 Point is moved to beginning of the buffer."
-  (declare (indent 1))
+  (declare (debug t) (indent 1))
   `(with-temp-buffer
      (insert ,content)
      (goto-char (point-min))
@@ -33,66 +38,107 @@ json-tests--with-temp-buffer
 
 ;;; Utilities
 
-(ert-deftest test-json-join ()
-  (should (equal (json-join '() ", ")  ""))
-  (should (equal (json-join '("a" "b" "c") ", ")  "a, b, c")))
-
 (ert-deftest test-json-alist-p ()
   (should (json-alist-p '()))
-  (should (json-alist-p '((a 1) (b 2) (c 3))))
-  (should (json-alist-p '((:a 1) (:b 2) (:c 3))))
-  (should (json-alist-p '(("a" 1) ("b" 2) ("c" 3))))
+  (should (json-alist-p '((()))))
+  (should (json-alist-p '((a))))
+  (should (json-alist-p '((a . 1))))
+  (should (json-alist-p '((a . 1) (b 2) (c))))
+  (should (json-alist-p '((:a) (:b 2) (:c . 3))))
+  (should (json-alist-p '(("a" . 1) ("b" 2) ("c"))))
+  (should-not (json-alist-p '(())))
+  (should-not (json-alist-p '(a)))
+  (should-not (json-alist-p '(a . 1)))
+  (should-not (json-alist-p '((a . 1) . [])))
+  (should-not (json-alist-p '((a . 1) [])))
   (should-not (json-alist-p '(:a :b :c)))
   (should-not (json-alist-p '(:a 1 :b 2 :c 3)))
-  (should-not (json-alist-p '((:a 1) (:b 2) 3))))
+  (should-not (json-alist-p '((:a 1) (:b 2) 3)))
+  (should-not (json-alist-p '((:a 1) (:b 2) ())))
+  (should-not (json-alist-p '(((a) 1) (b 2) (c 3))))
+  (should-not (json-alist-p []))
+  (should-not (json-alist-p [(a . 1)]))
+  (should-not (json-alist-p #s(hash-table))))
 
 (ert-deftest test-json-plist-p ()
   (should (json-plist-p '()))
+  (should (json-plist-p '(:a 1)))
   (should (json-plist-p '(:a 1 :b 2 :c 3)))
+  (should (json-plist-p '(:a :b)))
+  (should (json-plist-p '(:a :b :c :d)))
+  (should-not (json-plist-p '(a)))
+  (should-not (json-plist-p '(a 1)))
   (should-not (json-plist-p '(a 1 b 2 c 3)))
   (should-not (json-plist-p '("a" 1 "b" 2 "c" 3)))
+  (should-not (json-plist-p '(:a)))
   (should-not (json-plist-p '(:a :b :c)))
-  (should-not (json-plist-p '((:a 1) (:b 2) (:c 3)))))
+  (should-not (json-plist-p '(:a 1 :b 2 :c)))
+  (should-not (json-plist-p '((:a 1))))
+  (should-not (json-plist-p '((:a 1) (:b 2) (:c 3))))
+  (should-not (json-plist-p []))
+  (should-not (json-plist-p [:a 1]))
+  (should-not (json-plist-p #s(hash-table))))
 
-(ert-deftest test-json-plist-reverse ()
-  (should (equal (json--plist-reverse '()) '()))
-  (should (equal (json--plist-reverse '(:a 1)) '(:a 1)))
-  (should (equal (json--plist-reverse '(:a 1 :b 2 :c 3))
+(ert-deftest test-json-plist-nreverse ()
+  (should (equal (json--plist-nreverse '()) '()))
+  (should (equal (json--plist-nreverse (list :a 1)) '(:a 1)))
+  (should (equal (json--plist-nreverse (list :a 1 :b 2)) '(:b 2 :a 1)))
+  (should (equal (json--plist-nreverse (list :a 1 :b 2 :c 3))
                  '(:c 3 :b 2 :a 1))))
 
-(ert-deftest test-json-plist-to-alist ()
-  (should (equal (json--plist-to-alist '()) '()))
-  (should (equal (json--plist-to-alist '(:a 1)) '((:a . 1))))
-  (should (equal (json--plist-to-alist '(:a 1 :b 2 :c 3))
-                 '((:a . 1) (:b . 2) (:c . 3)))))
-
 (ert-deftest test-json-advance ()
   (json-tests--with-temp-buffer "{ \"a\": 1 }"
     (json-advance 0)
-    (should (= (point) (point-min)))
+    (should (bobp))
+    (json-advance)
+    (should (= (point) (1+ (point-min))))
+    (json-advance 0)
+    (should (= (point) (1+ (point-min))))
+    (json-advance 1)
+    (should (= (point) (+ (point-min) 2)))
     (json-advance 3)
-    (should (= (point) (+ (point-min) 3)))))
+    (should (= (point) (+ (point-min) 5)))))
 
 (ert-deftest test-json-peek ()
   (json-tests--with-temp-buffer ""
     (should (zerop (json-peek))))
   (json-tests--with-temp-buffer "{ \"a\": 1 }"
-    (should (equal (json-peek) ?{))))
+    (should (= (json-peek) ?\{))
+    (goto-char (1- (point-max)))
+    (should (= (json-peek) ?\}))
+    (json-advance)
+    (should (zerop (json-peek)))))
 
 (ert-deftest test-json-pop ()
   (json-tests--with-temp-buffer ""
     (should-error (json-pop) :type 'json-end-of-file))
   (json-tests--with-temp-buffer "{ \"a\": 1 }"
-    (should (equal (json-pop) ?{))
-    (should (= (point) (+ (point-min) 1)))))
+    (should (= (json-pop) ?\{))
+    (should (= (point) (1+ (point-min))))
+    (goto-char (1- (point-max)))
+    (should (= (json-pop) ?\}))
+    (should-error (json-pop) :type 'json-end-of-file)))
 
 (ert-deftest test-json-skip-whitespace ()
+  (json-tests--with-temp-buffer ""
+    (json-skip-whitespace)
+    (should (bobp))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "{}"
+    (json-skip-whitespace)
+    (should (bobp))
+    (json-advance)
+    (json-skip-whitespace)
+    (should (= (point) (1+ (point-min))))
+    (json-advance)
+    (json-skip-whitespace)
+    (should (eobp)))
   (json-tests--with-temp-buffer "\t\r\n\f\b { \"a\": 1 }"
     (json-skip-whitespace)
-    (should (equal (char-after) ?\f)))
+    (should (= (json-peek) ?\f)))
   (json-tests--with-temp-buffer "\t\r\n\t { \"a\": 1 }"
     (json-skip-whitespace)
-    (should (equal (char-after) ?{))))
+    (should (= (json-peek) ?\{))))
 
 ;;; Paths
 
@@ -113,59 +159,243 @@ test-json-path-to-position-with-arrays
 (ert-deftest test-json-path-to-position-no-match ()
   (let* ((json-string "{\"foo\": {\"bar\": \"baz\"}}")
          (matched-path (json-path-to-position 5 json-string)))
-    (should (null matched-path))))
+    (should-not matched-path)))
 
 ;;; Keywords
 
 (ert-deftest test-json-read-keyword ()
   (json-tests--with-temp-buffer "true"
-    (should (json-read-keyword "true")))
+    (should (eq (json-read-keyword "true") t))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "true "
+    (should (eq (json-read-keyword "true") t))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "true}"
+    (should (eq (json-read-keyword "true") t))
+    (should (= (point) (+ (point-min) 4))))
+  (json-tests--with-temp-buffer "true false"
+    (should (eq (json-read-keyword "true") t))
+    (should (= (point) (+ (point-min) 5))))
+  (json-tests--with-temp-buffer "true }"
+    (should (eq (json-read-keyword "true") t))
+    (should (= (point) (+ (point-min) 5))))
+  (json-tests--with-temp-buffer "true |"
+    (should (eq (json-read-keyword "true") t))
+    (should (= (point) (+ (point-min) 5))))
+  (json-tests--with-temp-buffer "false"
+    (let ((json-false 'false))
+      (should (eq (json-read-keyword "false") 'false)))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "null"
+    (let ((json-null 'null))
+      (should (eq (json-read-keyword "null") 'null)))
+    (should (eobp))))
+
+(ert-deftest test-json-read-keyword-invalid ()
+  (json-tests--with-temp-buffer ""
+    (should (equal (should-error (json-read-keyword ""))
+                   '(json-unknown-keyword "")))
+    (should (equal (should-error (json-read-keyword "true"))
+                   '(json-unknown-keyword ()))))
   (json-tests--with-temp-buffer "true"
-    (should-error
-     (json-read-keyword "false") :type 'json-unknown-keyword))
+    (should (equal (should-error (json-read-keyword "false"))
+                   '(json-unknown-keyword "true"))))
   (json-tests--with-temp-buffer "foo"
-    (should-error
-     (json-read-keyword "foo") :type 'json-unknown-keyword)))
+    (should (equal (should-error (json-read-keyword "foo"))
+                   '(json-unknown-keyword "foo")))
+    (should (equal (should-error (json-read-keyword "bar"))
+                   '(json-unknown-keyword "bar"))))
+  (json-tests--with-temp-buffer " true"
+    (should (equal (should-error (json-read-keyword "true"))
+                   '(json-unknown-keyword ()))))
+  (json-tests--with-temp-buffer "truefalse"
+    (should (equal (should-error (json-read-keyword "true"))
+                   '(json-unknown-keyword "truefalse"))))
+  (json-tests--with-temp-buffer "true|"
+    (should (equal (should-error (json-read-keyword "true"))
+                   '(json-unknown-keyword "true")))))
 
 (ert-deftest test-json-encode-keyword ()
   (should (equal (json-encode-keyword t) "true"))
-  (should (equal (json-encode-keyword json-false) "false"))
-  (should (equal (json-encode-keyword json-null) "null")))
+  (let ((json-false 'false))
+    (should (equal (json-encode-keyword 'false) "false"))
+    (should (equal (json-encode-keyword json-false) "false")))
+  (let ((json-null 'null))
+    (should (equal (json-encode-keyword 'null) "null"))
+    (should (equal (json-encode-keyword json-null) "null"))))
 
 ;;; Numbers
 
-(ert-deftest test-json-read-number ()
-  (json-tests--with-temp-buffer "3"
-    (should (= (json-read-number) 3)))
-  (json-tests--with-temp-buffer "-5"
-    (should (= (json-read-number) -5)))
-  (json-tests--with-temp-buffer "123.456"
-    (should (= (json-read-number) 123.456)))
-  (json-tests--with-temp-buffer "1e3"
-    (should (= (json-read-number) 1e3)))
-  (json-tests--with-temp-buffer "2e+3"
-    (should (= (json-read-number) 2e3)))
-  (json-tests--with-temp-buffer "3E3"
-    (should (= (json-read-number) 3e3)))
-  (json-tests--with-temp-buffer "1e-7"
-    (should (= (json-read-number) 1e-7)))
-  (json-tests--with-temp-buffer "abc"
-    (should-error (json-read-number) :type 'json-number-format)))
+(ert-deftest test-json-read-integer ()
+  (json-tests--with-temp-buffer "0 "
+    (should (= (json-read-number) 0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0 "
+    (should (= (json-read-number) 0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "3 "
+    (should (= (json-read-number) 3))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-10 "
+    (should (= (json-read-number) -10))
+    (should (eobp)))
+  (json-tests--with-temp-buffer (format "%d " (1+ most-positive-fixnum))
+    (should (= (json-read-number) (1+ most-positive-fixnum)))
+    (should (eobp)))
+  (json-tests--with-temp-buffer (format "%d " (1- most-negative-fixnum))
+    (should (= (json-read-number) (1- most-negative-fixnum)))
+    (should (eobp))))
+
+(ert-deftest test-json-read-fraction ()
+  (json-tests--with-temp-buffer "0.0 "
+    (should (= (json-read-number) 0.0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0.0 "
+    (should (= (json-read-number) 0.0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "0.01 "
+    (should (= (json-read-number) 0.01))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0.01 "
+    (should (= (json-read-number) -0.01))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "123.456 "
+    (should (= (json-read-number) 123.456))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-123.456 "
+    (should (= (json-read-number) -123.456))
+    (should (eobp))))
+
+(ert-deftest test-json-read-exponent ()
+  (json-tests--with-temp-buffer "0e0 "
+    (should (= (json-read-number) 0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0E0 "
+    (should (= (json-read-number) 0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0E+0 "
+    (should (= (json-read-number) 0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "0e-0 "
+    (should (= (json-read-number) 0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "12e34 "
+    (should (= (json-read-number) 12e34))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-12E34 "
+    (should (= (json-read-number) -12e34))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-12E+34 "
+    (should (= (json-read-number) -12e34))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "12e-34 "
+    (should (= (json-read-number) 12e-34))
+    (should (eobp))))
+
+(ert-deftest test-json-read-fraction-exponent ()
+  (json-tests--with-temp-buffer "0.0e0 "
+    (should (= (json-read-number) 0.0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-0.0E0 "
+    (should (= (json-read-number) 0.0e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "0.12E-0 "
+    (should (= (json-read-number) 0.12e0))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "-12.34e+56 "
+    (should (= (json-read-number) -12.34e+56))
+    (should (eobp))))
+
+(ert-deftest test-json-read-number-invalid ()
+  (cl-flet ((read (str)
+                  ;; Return error and point resulting from reading STR.
+                  (json-tests--with-temp-buffer str
+                    (cons (should-error (json-read-number)) (point)))))
+    ;; POS is where each of its STRINGS becomes invalid.
+    (pcase-dolist (`(,pos . ,strings)
+                   '((1 "" "+" "-" "." "e" "e1" "abc" "++0" "++1"
+                        "+0"  "+0.0" "+12" "+12.34" "+12.34e56"
+                        ".0" "+.0" "-.0" ".12" "+.12" "-.12"
+                        ".e0" "+.e0" "-.e0" ".0e0" "+.0e0" "-.0e0")
+                     (2 "01" "1ee1" "1e++1")
+                     (3 "-01")
+                     (4 "0.0.0" "1.1.1" "1e1e1")
+                     (5 "-0.0.0" "-1.1.1")))
+      ;; Expected error and point.
+      (let ((res `((json-number-format ,pos) . ,pos)))
+        (dolist (str strings)
+          (should (equal (read str) res)))))))
 
 (ert-deftest test-json-encode-number ()
+  (should (equal (json-encode-number 0) "0"))
+  (should (equal (json-encode-number -0) "0"))
   (should (equal (json-encode-number 3) "3"))
   (should (equal (json-encode-number -5) "-5"))
-  (should (equal (json-encode-number 123.456) "123.456")))
+  (should (equal (json-encode-number 123.456) "123.456"))
+  (let ((bignum (1+ most-positive-fixnum)))
+    (should (equal (json-encode-number bignum)
+                   (number-to-string bignum)))))
 
-;; Strings
+;;; Strings
 
 (ert-deftest test-json-read-escaped-char ()
   (json-tests--with-temp-buffer "\\\""
-    (should (equal (json-read-escaped-char) ?\"))))
+    (should (= (json-read-escaped-char) ?\"))
+    (should (eobp)))
+  (json-tests--with-temp-buffer "\\\\ "
+    (should (= (json-read-escaped-char) ?\\))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\b "
+    (should (= (json-read-escaped-char) ?\b))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\f "
+    (should (= (json-read-escaped-char) ?\f))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\n "
+    (should (= (json-read-escaped-char) ?\n))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\r "
+    (should (= (json-read-escaped-char) ?\r))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\t "
+    (should (= (json-read-escaped-char) ?\t))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\x "
+    (should (= (json-read-escaped-char) ?x))
+    (should (= (point) (+ (point-min) 2))))
+  (json-tests--with-temp-buffer "\\ud800\\uDC00 "
+    (should (= (json-read-escaped-char) #x10000))
+    (should (= (point) (+ (point-min) 12))))
+  (json-tests--with-temp-buffer "\\ud7ff\\udc00 "
+    (should (= (json-read-escaped-char) #xd7ff))
+    (should (= (point) (+ (point-min) 6))))
+  (json-tests--with-temp-buffer "\\uffff "
+    (should (= (json-read-escaped-char) #xffff))
+    (should (= (point) (+ (point-min) 6))))
+  (json-tests--with-temp-buffer "\\ufffff "
+    (should (= (json-read-escaped-char) #xffff))
+    (should (= (point) (+ (point-min) 6)))))
+
+(ert-deftest test-json-read-escaped-char-invalid ()
+  (json-tests--with-temp-buffer ""
+    (should-error (json-read-escaped-char)))
+  (json-tests--with-temp-buffer "\\"
+    (should-error (json-read-escaped-char) :type 'json-end-of-file))
+  (json-tests--with-temp-buffer "\\ufff "
+    (should (equal (should-error (json-read-escaped-char))
+                   (list 'json-string-escape (+ (point-min) 2)))))
+  (json-tests--with-temp-buffer "\\ufffg "
+    (should (equal (should-error (json-read-escaped-char))
+                   (list 'json-string-escape (+ (point-min) 2))))))
 
 (ert-deftest test-json-read-string ()
+  (json-tests--with-temp-buffer ""
+    (should-error (json-read-string)))
   (json-tests--with-temp-buffer "\"formfeed\f\""
-    (should-error (json-read-string) :type 'json-string-format))
+    (should (equal (should-error (json-read-string))
+                   '(json-string-format ?\f))))
+  (json-tests--with-temp-buffer "\"\""
+    (should (equal (json-read-string) "")))
   (json-tests--with-temp-buffer "\"foo \\\"bar\\\"\""
     (should (equal (json-read-string) "foo \"bar\"")))
   (json-tests--with-temp-buffer "\"abcαβγ\""
@@ -175,57 +405,117 @@ test-json-read-string
   ;; Bug#24784
   (json-tests--with-temp-buffer "\"\\uD834\\uDD1E\""
     (should (equal (json-read-string) "\U0001D11E")))
+  (json-tests--with-temp-buffer "f"
+    (should-error (json-read-string) :type 'json-end-of-file))
   (json-tests--with-temp-buffer "foo"
-    (should-error (json-read-string) :type 'json-string-format)))
+    (should-error (json-read-string) :type 'json-end-of-file)))
 
 (ert-deftest test-json-encode-string ()
+  (should (equal (json-encode-string "") "\"\""))
+  (should (equal (json-encode-string "a") "\"a\""))
   (should (equal (json-encode-string "foo") "\"foo\""))
   (should (equal (json-encode-string "a\n\fb") "\"a\\n\\fb\""))
   (should (equal (json-encode-string "\nasdфыв\u001f\u007ffgh\t")
                  "\"\\nasdфыв\\u001f\u007ffgh\\t\"")))
 
 (ert-deftest test-json-encode-key ()
+  (should (equal (json-encode-key "") "\"\""))
+  (should (equal (json-encode-key '##) "\"\""))
+  (should (equal (json-encode-key :) "\"\""))
   (should (equal (json-encode-key "foo") "\"foo\""))
   (should (equal (json-encode-key 'foo) "\"foo\""))
   (should (equal (json-encode-key :foo) "\"foo\""))
-  (should-error (json-encode-key 5) :type 'json-key-format)
-  (should-error (json-encode-key ["foo"]) :type 'json-key-format)
-  (should-error (json-encode-key '("foo")) :type 'json-key-format))
+  (should (equal (should-error (json-encode-key 5))
+                 '(json-key-format 5)))
+  (should (equal (should-error (json-encode-key ["foo"]))
+                 '(json-key-format ["foo"])))
+  (should (equal (should-error (json-encode-key '("foo")))
+                 '(json-key-format ("foo")))))
 
 ;;; Objects
 
 (ert-deftest test-json-new-object ()
   (let ((json-object-type 'alist))
-    (should (equal (json-new-object) '())))
+    (should-not (json-new-object)))
   (let ((json-object-type 'plist))
-    (should (equal (json-new-object) '())))
+    (should-not (json-new-object)))
   (let* ((json-object-type 'hash-table)
          (json-object (json-new-object)))
     (should (hash-table-p json-object))
-    (should (= (hash-table-count json-object) 0))))
+    (should (map-empty-p json-object))
+    (should (eq (hash-table-test json-object) #'equal))))
 
-(ert-deftest test-json-add-to-object ()
+(ert-deftest test-json-add-to-alist ()
   (let* ((json-object-type 'alist)
-         (json-key-type nil)
          (obj (json-new-object)))
-    (setq obj (json-add-to-object obj "a" 1))
-    (setq obj (json-add-to-object obj "b" 2))
-    (should (equal (assq 'a obj) '(a . 1)))
-    (should (equal (assq 'b obj) '(b . 2))))
+    (let ((json-key-type nil))
+      (setq obj (json-add-to-object obj "a" 1))
+      (setq obj (json-add-to-object obj "b" 2))
+      (should (equal (assq 'a obj) '(a . 1)))
+      (should (equal (assq 'b obj) '(b . 2))))
+    (let ((json-key-type 'symbol))
+      (setq obj (json-add-to-object obj "c" 3))
+      (setq obj (json-add-to-object obj "d" 4))
+      (should (equal (assq 'c obj) '(c . 3)))
+      (should (equal (assq 'd obj) '(d . 4))))
+    (let ((json-key-type 'keyword))
+      (setq obj (json-add-to-object obj "e" 5))
+      (setq obj (json-add-to-object obj "f" 6))
+      (should (equal (assq :e obj) '(:e . 5)))
+      (should (equal (assq :f obj) '(:f . 6))))
+    (let ((json-key-type 'string))
+      (setq obj (json-add-to-object obj "g" 7))
+      (setq obj (json-add-to-object obj "h" 8))
+      (should (equal (assoc "g" obj) '("g" . 7)))
+      (should (equal (assoc "h" obj) '("h" . 8))))))
+
+(ert-deftest test-json-add-to-plist ()
   (let* ((json-object-type 'plist)
-         (json-key-type nil)
          (obj (json-new-object)))
-    (setq obj (json-add-to-object obj "a" 1))
-    (setq obj (json-add-to-object obj "b" 2))
-    (should (= (plist-get obj :a) 1))
-    (should (= (plist-get obj :b) 2)))
+    (let ((json-key-type nil))
+      (setq obj (json-add-to-object obj "a" 1))
+      (setq obj (json-add-to-object obj "b" 2))
+      (should (= (plist-get obj :a) 1))
+      (should (= (plist-get obj :b) 2)))
+    (let ((json-key-type 'keyword))
+      (setq obj (json-add-to-object obj "c" 3))
+      (setq obj (json-add-to-object obj "d" 4))
+      (should (= (plist-get obj :c) 3))
+      (should (= (plist-get obj :d) 4)))
+    (let ((json-key-type 'symbol))
+      (setq obj (json-add-to-object obj "e" 5))
+      (setq obj (json-add-to-object obj "f" 6))
+      (should (= (plist-get obj 'e) 5))
+      (should (= (plist-get obj 'f) 6)))
+    (let ((json-key-type 'string))
+      (setq obj (json-add-to-object obj "g" 7))
+      (setq obj (json-add-to-object obj "h" 8))
+      (should (= (lax-plist-get obj "g") 7))
+      (should (= (lax-plist-get obj "h") 8)))))
+
+(ert-deftest test-json-add-to-hash-table ()
   (let* ((json-object-type 'hash-table)
-         (json-key-type nil)
          (obj (json-new-object)))
-    (setq obj (json-add-to-object obj "a" 1))
-    (setq obj (json-add-to-object obj "b" 2))
-    (should (= (gethash "a" obj) 1))
-    (should (= (gethash "b" obj) 2))))
+    (let ((json-key-type nil))
+      (setq obj (json-add-to-object obj "a" 1))
+      (setq obj (json-add-to-object obj "b" 2))
+      (should (= (gethash "a" obj) 1))
+      (should (= (gethash "b" obj) 2)))
+    (let ((json-key-type 'string))
+      (setq obj (json-add-to-object obj "c" 3))
+      (setq obj (json-add-to-object obj "d" 4))
+      (should (= (gethash "c" obj) 3))
+      (should (= (gethash "d" obj) 4)))
+    (let ((json-key-type 'symbol))
+      (setq obj (json-add-to-object obj "e" 5))
+      (setq obj (json-add-to-object obj "f" 6))
+      (should (= (gethash 'e obj) 5))
+      (should (= (gethash 'f obj) 6)))
+    (let ((json-key-type 'keyword))
+      (setq obj (json-add-to-object obj "g" 7))
+      (setq obj (json-add-to-object obj "h" 8))
+      (should (= (gethash :g obj) 7))
+      (should (= (gethash :h obj) 8)))))
 
 (ert-deftest test-json-read-object ()
   (json-tests--with-temp-buffer "{ \"a\": 1, \"b\": 2 }"
@@ -238,94 +528,384 @@ test-json-read-object
     (let* ((json-object-type 'hash-table)
            (hash-table (json-read-object)))
       (should (= (gethash "a" hash-table) 1))
-      (should (= (gethash "b" hash-table) 2))))
+      (should (= (gethash "b" hash-table) 2)))))
+
+(ert-deftest test-json-read-object-empty ()
+  (json-tests--with-temp-buffer "{}"
+    (let ((json-object-type 'alist))
+      (should-not (save-excursion (json-read-object))))
+    (let ((json-object-type 'plist))
+      (should-not (save-excursion (json-read-object))))
+    (let* ((json-object-type 'hash-table)
+           (hash-table (json-read-object)))
+      (should (hash-table-p hash-table))
+      (should (map-empty-p hash-table)))))
+
+(ert-deftest test-json-read-object-invalid ()
+  (json-tests--with-temp-buffer "{ \"a\" 1, \"b\": 2 }"
+    (should (equal (should-error (json-read-object))
+                   '(json-object-format ":" ?1))))
   (json-tests--with-temp-buffer "{ \"a\": 1 \"b\": 2 }"
-    (should-error (json-read-object) :type 'json-object-format)))
+    (should (equal (should-error (json-read-object))
+                   '(json-object-format "," ?\")))))
+
+(ert-deftest test-json-read-object-function ()
+  (let* ((pre nil)
+         (post nil)
+         (keys '("b" "a"))
+         (json-pre-element-read-function
+          (lambda (key)
+            (setq pre 'pre)
+            (should (equal key (pop keys)))))
+         (json-post-element-read-function
+          (lambda () (setq post 'post))))
+    (json-tests--with-temp-buffer "{ \"b\": 2, \"a\": 1 }"
+      (json-read-object)
+      (should (eq pre 'pre))
+      (should (eq post 'post)))))
 
 (ert-deftest test-json-encode-hash-table ()
-  (let ((hash-table (make-hash-table))
-        (json-encoding-object-sort-predicate 'string<)
+  (let ((json-encoding-object-sort-predicate nil)
         (json-encoding-pretty-print nil))
-    (puthash :a 1 hash-table)
-    (puthash :b 2 hash-table)
-    (puthash :c 3 hash-table)
-    (should (equal (json-encode hash-table)
-                   "{\"a\":1,\"b\":2,\"c\":3}"))))
+    (should (equal (json-encode-hash-table #s(hash-table)) "{}"))
+    (should (equal (json-encode-hash-table #s(hash-table data (a 1)))
+                   "{\"a\":1}"))
+    (should (member (json-encode-hash-table #s(hash-table data (b 2 a 1)))
+                    '("{\"a\":1,\"b\":2}" "{\"b\":2,\"a\":1}")))
+    (should (member (json-encode-hash-table #s(hash-table data (c 3 b 2 a 1)))
+                    '("{\"a\":1,\"b\":2,\"c\":3}"
+                      "{\"a\":1,\"c\":3,\"b\":2}"
+                      "{\"b\":2,\"a\":1,\"c\":3}"
+                      "{\"b\":2,\"c\":3,\"a\":1}"
+                      "{\"c\":3,\"a\":1,\"b\":2}"
+                      "{\"c\":3,\"b\":2,\"a\":1}")))))
 
-(ert-deftest json-encode-simple-alist ()
-  (let ((json-encoding-pretty-print nil))
-    (should (equal (json-encode '((a . 1) (b . 2)))
-                   "{\"a\":1,\"b\":2}"))))
+(ert-deftest test-json-encode-hash-table-pretty ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings nil))
+    (should (equal (json-encode-hash-table #s(hash-table)) "{}"))
+    (should (equal (json-encode-hash-table #s(hash-table data (a 1)))
+                   "{\n \"a\": 1\n}"))
+    (should (member (json-encode-hash-table #s(hash-table data (b 2 a 1)))
+                    '("{\n \"a\": 1,\n \"b\": 2\n}"
+                      "{\n \"b\": 2,\n \"a\": 1\n}")))
+    (should (member (json-encode-hash-table #s(hash-table data (c 3 b 2 a 1)))
+                    '("{\n \"a\": 1,\n \"b\": 2,\n \"c\": 3\n}"
+                      "{\n \"a\": 1,\n \"c\": 3,\n \"b\": 2\n}"
+                      "{\n \"b\": 2,\n \"a\": 1,\n \"c\": 3\n}"
+                      "{\n \"b\": 2,\n \"c\": 3,\n \"a\": 1\n}"
+                      "{\n \"c\": 3,\n \"a\": 1,\n \"b\": 2\n}"
+                      "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1\n}")))))
+
+(ert-deftest test-json-encode-hash-table-lisp-style ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings t))
+    (should (equal (json-encode-hash-table #s(hash-table)) "{}"))
+    (should (equal (json-encode-hash-table #s(hash-table data (a 1)))
+                   "{\n \"a\": 1}"))
+    (should (member (json-encode-hash-table #s(hash-table data (b 2 a 1)))
+                    '("{\n \"a\": 1,\n \"b\": 2}"
+                      "{\n \"b\": 2,\n \"a\": 1}")))
+    (should (member (json-encode-hash-table #s(hash-table data (c 3 b 2 a 1)))
+                    '("{\n \"a\": 1,\n \"b\": 2,\n \"c\": 3}"
+                      "{\n \"a\": 1,\n \"c\": 3,\n \"b\": 2}"
+                      "{\n \"b\": 2,\n \"a\": 1,\n \"c\": 3}"
+                      "{\n \"b\": 2,\n \"c\": 3,\n \"a\": 1}"
+                      "{\n \"c\": 3,\n \"a\": 1,\n \"b\": 2}"
+                      "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1}")))))
+
+(ert-deftest test-json-encode-hash-table-sort ()
+  (let ((json-encoding-object-sort-predicate #'string<)
+        (json-encoding-pretty-print nil))
+    (pcase-dolist (`(,in . ,out)
+                   '((#s(hash-table) . "{}")
+                     (#s(hash-table data (a 1)) . "{\"a\":1}")
+                     (#s(hash-table data (b 2 a 1)) . "{\"a\":1,\"b\":2}")
+                     (#s(hash-table data (c 3 b 2 a 1))
+                        . "{\"a\":1,\"b\":2,\"c\":3}")))
+      (let ((copy (map-pairs in)))
+        (should (equal (json-encode-hash-table in) out))
+        ;; Ensure sorting isn't destructive.
+        (should (seq-set-equal-p (map-pairs in) copy))))))
+
+(ert-deftest test-json-encode-alist ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print nil))
+    (should (equal (json-encode-alist ()) "{}"))
+    (should (equal (json-encode-alist '((a . 1))) "{\"a\":1}"))
+    (should (equal (json-encode-alist '((b . 2) (a . 1))) "{\"b\":2,\"a\":1}"))
+    (should (equal (json-encode-alist '((c . 3) (b . 2) (a . 1)))
+                   "{\"c\":3,\"b\":2,\"a\":1}"))))
+
+(ert-deftest test-json-encode-alist-pretty ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings nil))
+    (should (equal (json-encode-alist ()) "{}"))
+    (should (equal (json-encode-alist '((a . 1))) "{\n \"a\": 1\n}"))
+    (should (equal (json-encode-alist '((b . 2) (a . 1)))
+                   "{\n \"b\": 2,\n \"a\": 1\n}"))
+    (should (equal (json-encode-alist '((c . 3) (b . 2) (a . 1)))
+                   "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1\n}"))))
+
+(ert-deftest test-json-encode-alist-lisp-style ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings t))
+    (should (equal (json-encode-alist ()) "{}"))
+    (should (equal (json-encode-alist '((a . 1))) "{\n \"a\": 1}"))
+    (should (equal (json-encode-alist '((b . 2) (a . 1)))
+                   "{\n \"b\": 2,\n \"a\": 1}"))
+    (should (equal (json-encode-alist '((c . 3) (b . 2) (a . 1)))
+                   "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1}"))))
+
+(ert-deftest test-json-encode-alist-sort ()
+  (let ((json-encoding-object-sort-predicate #'string<)
+        (json-encoding-pretty-print nil))
+    (pcase-dolist (`(,in . ,out)
+                   '((() . "{}")
+                     (((a . 1)) . "{\"a\":1}")
+                     (((b . 2) (a . 1)) . "{\"a\":1,\"b\":2}")
+                     (((c . 3) (b . 2) (a . 1))
+                      . "{\"a\":1,\"b\":2,\"c\":3}")))
+      (let ((copy (copy-alist in)))
+        (should (equal (json-encode-alist in) out))
+        ;; Ensure sorting isn't destructive (bug#40693).
+        (should (equal in copy))))))
 
 (ert-deftest test-json-encode-plist ()
-  (let ((plist '(:a 1 :b 2))
+  (let ((json-encoding-object-sort-predicate nil)
         (json-encoding-pretty-print nil))
-    (should (equal (json-encode plist) "{\"a\":1,\"b\":2}"))))
+    (should (equal (json-encode-plist ()) "{}"))
+    (should (equal (json-encode-plist '(:a 1)) "{\"a\":1}"))
+    (should (equal (json-encode-plist '(:b 2 :a 1)) "{\"b\":2,\"a\":1}"))
+    (should (equal (json-encode-plist '(:c 3 :b 2 :a 1))
+                   "{\"c\":3,\"b\":2,\"a\":1}"))))
 
-(ert-deftest test-json-encode-plist-with-sort-predicate ()
-  (let ((plist '(:c 3 :a 1 :b 2))
-        (json-encoding-object-sort-predicate 'string<)
-        (json-encoding-pretty-print nil))
-    (should (equal (json-encode plist) "{\"a\":1,\"b\":2,\"c\":3}"))))
+(ert-deftest test-json-encode-plist-pretty ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings nil))
+    (should (equal (json-encode-plist ()) "{}"))
+    (should (equal (json-encode-plist '(:a 1)) "{\n \"a\": 1\n}"))
+    (should (equal (json-encode-plist '(:b 2 :a 1))
+                   "{\n \"b\": 2,\n \"a\": 1\n}"))
+    (should (equal (json-encode-plist '(:c 3 :b 2 :a 1))
+                   "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1\n}"))))
 
-(ert-deftest test-json-encode-alist-with-sort-predicate ()
-  (let ((alist '((:c . 3) (:a . 1) (:b . 2)))
-        (json-encoding-object-sort-predicate 'string<)
+(ert-deftest test-json-encode-plist-lisp-style ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings t))
+    (should (equal (json-encode-plist ()) "{}"))
+    (should (equal (json-encode-plist '(:a 1)) "{\n \"a\": 1}"))
+    (should (equal (json-encode-plist '(:b 2 :a 1))
+                   "{\n \"b\": 2,\n \"a\": 1}"))
+    (should (equal (json-encode-plist '(:c 3 :b 2 :a 1))
+                   "{\n \"c\": 3,\n \"b\": 2,\n \"a\": 1}"))))
+
+(ert-deftest test-json-encode-plist-sort ()
+  (let ((json-encoding-object-sort-predicate #'string<)
         (json-encoding-pretty-print nil))
-    (should (equal (json-encode alist) "{\"a\":1,\"b\":2,\"c\":3}"))))
+    (pcase-dolist (`(,in . ,out)
+                   '((() . "{}")
+                     ((:a 1) . "{\"a\":1}")
+                     ((:b 2 :a 1) . "{\"a\":1,\"b\":2}")
+                     ((:c 3 :b 2 :a 1) . "{\"a\":1,\"b\":2,\"c\":3}")))
+      (let ((copy (copy-sequence in)))
+        (should (equal (json-encode-plist in) out))
+        ;; Ensure sorting isn't destructive.
+        (should (equal in copy))))))
 
 (ert-deftest test-json-encode-list ()
-  (let ((json-encoding-pretty-print nil))
-    (should (equal (json-encode-list '(:a 1 :b 2))
-                   "{\"a\":1,\"b\":2}"))
-    (should (equal (json-encode-list '((:a . 1) (:b . 2)))
-                   "{\"a\":1,\"b\":2}"))
-    (should (equal (json-encode-list '(1 2 3 4)) "[1,2,3,4]"))))
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print nil))
+    (should (equal (json-encode-list ()) "{}"))
+    (should (equal (json-encode-list '(a)) "[\"a\"]"))
+    (should (equal (json-encode-list '(:a)) "[\"a\"]"))
+    (should (equal (json-encode-list '("a")) "[\"a\"]"))
+    (should (equal (json-encode-list '(a 1)) "[\"a\",1]"))
+    (should (equal (json-encode-list '("a" 1)) "[\"a\",1]"))
+    (should (equal (json-encode-list '(:a 1)) "{\"a\":1}"))
+    (should (equal (json-encode-list '((a . 1))) "{\"a\":1}"))
+    (should (equal (json-encode-list '((:a . 1))) "{\"a\":1}"))
+    (should (equal (json-encode-list '(:b 2 :a)) "[\"b\",2,\"a\"]"))
+    (should (equal (json-encode-list '(4 3 2 1)) "[4,3,2,1]"))
+    (should (equal (json-encode-list '(b 2 a 1)) "[\"b\",2,\"a\",1]"))
+    (should (equal (json-encode-list '(:b 2 :a 1)) "{\"b\":2,\"a\":1}"))
+    (should (equal (json-encode-list '((b . 2) (a . 1))) "{\"b\":2,\"a\":1}"))
+    (should (equal (json-encode-list '((:b . 2) (:a . 1)))
+                   "{\"b\":2,\"a\":1}"))
+    (should (equal (json-encode-list '((a) 1)) "[[\"a\"],1]"))
+    (should (equal (json-encode-list '((:a) 1)) "[[\"a\"],1]"))
+    (should (equal (json-encode-list '(("a") 1)) "[[\"a\"],1]"))
+    (should (equal (json-encode-list '((a 1) 2)) "[[\"a\",1],2]"))
+    (should (equal (json-encode-list '((:a 1) 2)) "[{\"a\":1},2]"))
+    (should (equal (json-encode-list '(((a . 1)) 2)) "[{\"a\":1},2]"))
+    (should (equal (json-encode-list '(:a 1 :b (2))) "{\"a\":1,\"b\":[2]}"))
+    (should (equal (json-encode-list '((a . 1) (b 2))) "{\"a\":1,\"b\":[2]}"))
+    (should-error (json-encode-list '(a . 1)) :type 'wrong-type-argument)
+    (should-error (json-encode-list '((a . 1) 2)) :type 'wrong-type-argument)
+    (should (equal (should-error (json-encode-list []))
+                   '(json-error [])))
+    (should (equal (should-error (json-encode-list [a]))
+                   '(json-error [a])))))
 
 ;;; Arrays
 
 (ert-deftest test-json-read-array ()
   (let ((json-array-type 'vector))
+    (json-tests--with-temp-buffer "[]"
+      (should (equal (json-read-array) [])))
+    (json-tests--with-temp-buffer "[ ]"
+      (should (equal (json-read-array) [])))
+    (json-tests--with-temp-buffer "[1]"
+      (should (equal (json-read-array) [1])))
     (json-tests--with-temp-buffer "[1, 2, \"a\", \"b\"]"
       (should (equal (json-read-array) [1 2 "a" "b"]))))
   (let ((json-array-type 'list))
+    (json-tests--with-temp-buffer "[]"
+      (should-not (json-read-array)))
+    (json-tests--with-temp-buffer "[ ]"
+      (should-not (json-read-array)))
+    (json-tests--with-temp-buffer "[1]"
+      (should (equal (json-read-array) '(1))))
     (json-tests--with-temp-buffer "[1, 2, \"a\", \"b\"]"
       (should (equal (json-read-array) '(1 2 "a" "b")))))
   (json-tests--with-temp-buffer "[1 2]"
-    (should-error (json-read-array) :type 'json-error)))
+    (should (equal (should-error (json-read-array))
+                   '(json-array-format "," ?2)))))
+
+(ert-deftest test-json-read-array-function ()
+  (let* ((pre nil)
+         (post nil)
+         (keys '(0 1))
+         (json-pre-element-read-function
+          (lambda (key)
+            (setq pre 'pre)
+            (should (equal key (pop keys)))))
+         (json-post-element-read-function
+          (lambda () (setq post 'post))))
+    (json-tests--with-temp-buffer "[1, 0]"
+      (json-read-array)
+      (should (eq pre 'pre))
+      (should (eq post 'post)))))
 
 (ert-deftest test-json-encode-array ()
-  (let ((json-encoding-pretty-print nil))
-    (should (equal (json-encode-array [1 2 "a" "b"])
-                   "[1,2,\"a\",\"b\"]"))))
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print nil))
+    (should (equal (json-encode-array ()) "[]"))
+    (should (equal (json-encode-array []) "[]"))
+    (should (equal (json-encode-array '(1)) "[1]"))
+    (should (equal (json-encode-array '[1]) "[1]"))
+    (should (equal (json-encode-array '(2 1)) "[2,1]"))
+    (should (equal (json-encode-array '[2 1]) "[2,1]"))
+    (should (equal (json-encode-array '[:b a 2 1]) "[\"b\",\"a\",2,1]"))))
+
+(ert-deftest test-json-encode-array-pretty ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings nil))
+    (should (equal (json-encode-array ()) "[]"))
+    (should (equal (json-encode-array []) "[]"))
+    (should (equal (json-encode-array '(1)) "[\n 1\n]"))
+    (should (equal (json-encode-array '[1]) "[\n 1\n]"))
+    (should (equal (json-encode-array '(2 1)) "[\n 2,\n 1\n]"))
+    (should (equal (json-encode-array '[2 1]) "[\n 2,\n 1\n]"))
+    (should (equal (json-encode-array '[:b a 2 1])
+                   "[\n \"b\",\n \"a\",\n 2,\n 1\n]"))))
+
+(ert-deftest test-json-encode-array-lisp-style ()
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print t)
+        (json-encoding-default-indentation " ")
+        (json-encoding-lisp-style-closings t))
+    (should (equal (json-encode-array ()) "[]"))
+    (should (equal (json-encode-array []) "[]"))
+    (should (equal (json-encode-array '(1)) "[\n 1]"))
+    (should (equal (json-encode-array '[1]) "[\n 1]"))
+    (should (equal (json-encode-array '(2 1)) "[\n 2,\n 1]"))
+    (should (equal (json-encode-array '[2 1]) "[\n 2,\n 1]"))
+    (should (equal (json-encode-array '[:b a 2 1])
+                   "[\n \"b\",\n \"a\",\n 2,\n 1]"))))
 
 ;;; Reader
 
 (ert-deftest test-json-read ()
-  (json-tests--with-temp-buffer "{ \"a\": 1 }"
-    ;; We don't care exactly what the return value is (that is tested
-    ;; in `test-json-read-object'), but it should parse without error.
-    (should (json-read)))
+  (pcase-dolist (`(,fn . ,contents)
+                 '((json-read-string  "\"\"" "\"a\"")
+                   (json-read-array   "[]" "[1]")
+                   (json-read-object  "{}" "{\"a\":1}")
+                   (json-read-keyword "null" "false" "true")
+                   (json-read-number
+                    "-0" "0" "1" "2" "3" "4" "5" "6" "7" "8" "9")))
+    (dolist (content contents)
+      ;; Check that leading whitespace is skipped.
+      (dolist (str (list content (concat " " content)))
+        (cl-letf* ((called nil)
+                   ((symbol-function fn)
+                    (lambda (&rest _) (setq called t))))
+          (json-tests--with-temp-buffer str
+            ;; We don't care exactly what the return value is (that is
+            ;; tested elsewhere), but it should parse without error.
+            (should (json-read))
+            (should called)))))))
+
+(ert-deftest test-json-read-invalid ()
   (json-tests--with-temp-buffer ""
     (should-error (json-read) :type 'json-end-of-file))
-  (json-tests--with-temp-buffer "xxx"
-    (let ((err (should-error (json-read) :type 'json-readtable-error)))
-      (should (equal (cdr err) '(?x))))))
+  (json-tests--with-temp-buffer " "
+    (should-error (json-read) :type 'json-end-of-file))
+  (json-tests--with-temp-buffer "x"
+    (should (equal (should-error (json-read))
+                  '(json-readtable-error ?x))))
+  (json-tests--with-temp-buffer " x"
+    (should (equal (should-error (json-read))
+                  '(json-readtable-error ?x)))))
 
 (ert-deftest test-json-read-from-string ()
-  (let ((json-string "{ \"a\": 1 }"))
-    (json-tests--with-temp-buffer json-string
-      (should (equal (json-read-from-string json-string)
+  (dolist (str '("\"\"" "\"a\"" "[]" "[1]" "{}" "{\"a\":1}"
+                 "null" "false" "true" "0" "123"))
+    (json-tests--with-temp-buffer str
+      (should (equal (json-read-from-string str)
                      (json-read))))))
 
-;;; JSON encoder
+;;; Encoder
 
 (ert-deftest test-json-encode ()
+  (should (equal (json-encode t) "true"))
+  (let ((json-null 'null))
+    (should (equal (json-encode json-null) "null")))
+  (let ((json-false 'false))
+    (should (equal (json-encode json-false) "false")))
+  (should (equal (json-encode "") "\"\""))
   (should (equal (json-encode "foo") "\"foo\""))
+  (should (equal (json-encode :) "\"\""))
+  (should (equal (json-encode :foo) "\"foo\""))
+  (should (equal (json-encode '(1)) "[1]"))
+  (should (equal (json-encode 'foo) "\"foo\""))
+  (should (equal (json-encode 0) "0"))
+  (should (equal (json-encode 123) "123"))
+  (let ((json-encoding-object-sort-predicate nil)
+        (json-encoding-pretty-print nil))
+    (should (equal (json-encode []) "[]"))
+    (should (equal (json-encode [1]) "[1]"))
+    (should (equal (json-encode #s(hash-table)) "{}"))
+    (should (equal (json-encode #s(hash-table data (a 1))) "{\"a\":1}")))
   (with-temp-buffer
-    (should-error (json-encode (current-buffer)) :type 'json-error)))
+    (should (equal (should-error (json-encode (current-buffer)))
+                   (list 'json-error (current-buffer))))))
 
-;;; Pretty-print
+;;; Pretty printing & minimizing
 
 (defun json-tests-equal-pretty-print (original &optional expected)
   "Abort current test if pretty-printing ORIGINAL does not yield EXPECTED.
@@ -351,46 +931,45 @@ test-json-pretty-print-number
   (json-tests-equal-pretty-print "0.123"))
 
 (ert-deftest test-json-pretty-print-object ()
-  ;; empty (regression test for bug#24252)
-  (json-tests-equal-pretty-print
-   "{}"
-   "{\n}")
-  ;; one pair
+  ;; Empty (regression test for bug#24252).
+  (json-tests-equal-pretty-print "{}")
+  ;; One pair.
   (json-tests-equal-pretty-print
    "{\"key\":1}"
    "{\n  \"key\": 1\n}")
-  ;; two pairs
+  ;; Two pairs.
   (json-tests-equal-pretty-print
    "{\"key1\":1,\"key2\":2}"
    "{\n  \"key1\": 1,\n  \"key2\": 2\n}")
-  ;; embedded object
+  ;; Nested object.
   (json-tests-equal-pretty-print
    "{\"foo\":{\"key\":1}}"
    "{\n  \"foo\": {\n    \"key\": 1\n  }\n}")
-  ;; embedded array
+  ;; Nested array.
   (json-tests-equal-pretty-print
    "{\"key\":[1,2]}"
    "{\n  \"key\": [\n    1,\n    2\n  ]\n}"))
 
 (ert-deftest test-json-pretty-print-array ()
-  ;; empty
+  ;; Empty.
   (json-tests-equal-pretty-print "[]")
-  ;; one item
+  ;; One item.
   (json-tests-equal-pretty-print
    "[1]"
    "[\n  1\n]")
-  ;; two items
+  ;; Two items.
   (json-tests-equal-pretty-print
    "[1,2]"
    "[\n  1,\n  2\n]")
-  ;; embedded object
+  ;; Nested object.
   (json-tests-equal-pretty-print
    "[{\"key\":1}]"
    "[\n  {\n    \"key\": 1\n  }\n]")
-  ;; embedded array
+  ;; Nested array.
   (json-tests-equal-pretty-print
    "[[1,2]]"
    "[\n  [\n    1,\n    2\n  ]\n]"))
 
 (provide 'json-tests)
+
 ;;; json-tests.el ends here
-- 
2.26.2


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


"Basil L. Contovounesios" <contovob@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:


[-- Attachment #4: json-bench.el --]
[-- Type: application/emacs-lisp, Size: 695 bytes --]

[-- Attachment #5: Type: text/plain, Size: 2339 bytes --]


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

bug#40693: 28.0.50; json-encode-alist changes alist

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: Dmitry Gutov <dgutov@yandex.ru>,  darthandrus@gmail.com,
>   40693@debbugs.gnu.org, João Távora
>  <joaotavora@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.

bug#40693: 28.0.50; json-encode-alist changes alist

[Dmitry Gutov]

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.

bug#40693: 28.0.50; json-encode-alist changes alist

[João Távora]

Hi Basil,

"Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

> "Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[João Távora]

"Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

tags 40693 fixed
close 40693 28.1
quit

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

> "Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[João Távora]

"Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

> "Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[João Távora]

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

> João Távora <joaotavora@gmail.com> writes:
>> "Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[Basil L. Contovounesios]

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

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> João Távora <joaotavora@gmail.com> writes:
>>> "Basil L. Contovounesios" <contovob@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

bug#40693: 28.0.50; json-encode-alist changes alist

[João Távora]

"Basil L. Contovounesios" <contovob@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