bug#60311: json-available-p: make dynamically correct for Windows

bug#60311: json-available-p: make dynamically correct for Windows

[Mattias Engdegård]

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

The implementation of `json-available-p`,

  (and (fboundp 'json-serialize)
       (condition-case nil
           (json-serialize t)
         (:success t)
         (json-unavailable nil))))

probably isn't quite right on Windows: `json-serialize` is pure so it will be called at compile time and the result, "true", used in the code (actually not even that since the result is never used). Thus, if libjansson could not be loaded during actual Emacs use (as opposed to when Emacs was built), this would never be detected and json-available-p would still return t.

The patch below should take care of this. Would someone please help testing it on Windows?


[-- Attachment #2: json-available-p.diff --]
[-- Type: application/octet-stream, Size: 4818 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index fff4c88ccf..ce5ac14f5f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -6905,11 +6905,11 @@ internal--format-docstring-line
 
 (defun json-available-p ()
   "Return non-nil if Emacs has libjansson support."
-  (and (fboundp 'json-serialize)
-       (condition-case nil
-           (json-serialize t)
-         (:success t)
-         (json-unavailable nil))))
+  (declare (side-effect-free error-free))
+  (and (eval-when-compile (fboundp 'json-serialize))
+       ;; If `json--available-p' is present, we need to call it at run-time.
+       (or (not (eval-when-compile (fboundp 'json--available-p)))
+           (json--available-p))))
 
 (defun ensure-list (object)
   "Return OBJECT as a list.
diff --git a/src/json.c b/src/json.c
index cdcc11358e..d2105bc27b 100644
--- a/src/json.c
+++ b/src/json.c
@@ -555,6 +555,38 @@ json_parse_args (ptrdiff_t nargs,
   }
 }
 
+#ifdef WINDOWSNT
+static bool
+json_available_p (void)
+{
+  if (json_initialized)
+    return true;
+  json_initialized = init_json_functions ();
+  Lisp_Object status = json_initialized ? Qt : Qnil;
+  Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
+  return json_initialized;
+}
+#endif
+
+static void
+ensure_json_available (void)
+{
+#ifdef WINDOWSNT
+  if (!json_available_p ())
+    Fsignal (Qjson_unavailable,
+	     list1 (build_unibyte_string ("jansson library not found")));
+#endif
+}
+
+#ifdef WINDOWSNT
+DEFUN ("json--available-p", Fjson__available_p, Sjson__available_p, 0, 0, NULL,
+       doc: /* Whether libjansson is available (internal).  */)
+  (void)
+{
+  return json_available_p () ? Qt : Qnil;
+}
+#endif
+
 DEFUN ("json-serialize", Fjson_serialize, Sjson_serialize, 1, MANY,
        NULL,
        doc: /* Return the JSON representation of OBJECT as a string.
@@ -585,19 +617,7 @@ DEFUN ("json-serialize", Fjson_serialize, Sjson_serialize, 1, MANY,
      (ptrdiff_t nargs, Lisp_Object *args)
 {
   specpdl_ref count = SPECPDL_INDEX ();
-
-#ifdef WINDOWSNT
-  if (!json_initialized)
-    {
-      Lisp_Object status;
-      json_initialized = init_json_functions ();
-      status = json_initialized ? Qt : Qnil;
-      Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
-    }
-  if (!json_initialized)
-    Fsignal (Qjson_unavailable,
-	     list1 (build_unibyte_string ("jansson library not found")));
-#endif
+  ensure_json_available ();
 
   struct json_configuration conf =
     {json_object_hashtable, json_array_array, QCnull, QCfalse};
@@ -694,19 +714,7 @@ DEFUN ("json-insert", Fjson_insert, Sjson_insert, 1, MANY,
      (ptrdiff_t nargs, Lisp_Object *args)
 {
   specpdl_ref count = SPECPDL_INDEX ();
-
-#ifdef WINDOWSNT
-  if (!json_initialized)
-    {
-      Lisp_Object status;
-      json_initialized = init_json_functions ();
-      status = json_initialized ? Qt : Qnil;
-      Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
-    }
-  if (!json_initialized)
-    Fsignal (Qjson_unavailable,
-	     list1 (build_unibyte_string ("jansson library not found")));
-#endif
+  ensure_json_available ();
 
   struct json_configuration conf =
     {json_object_hashtable, json_array_array, QCnull, QCfalse};
@@ -951,19 +959,7 @@ DEFUN ("json-parse-string", Fjson_parse_string, Sjson_parse_string, 1, MANY,
   (ptrdiff_t nargs, Lisp_Object *args)
 {
   specpdl_ref count = SPECPDL_INDEX ();
-
-#ifdef WINDOWSNT
-  if (!json_initialized)
-    {
-      Lisp_Object status;
-      json_initialized = init_json_functions ();
-      status = json_initialized ? Qt : Qnil;
-      Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
-    }
-  if (!json_initialized)
-    Fsignal (Qjson_unavailable,
-	     list1 (build_unibyte_string ("jansson library not found")));
-#endif
+  ensure_json_available ();
 
   Lisp_Object string = args[0];
   CHECK_STRING (string);
@@ -1048,19 +1044,7 @@ DEFUN ("json-parse-buffer", Fjson_parse_buffer, Sjson_parse_buffer,
      (ptrdiff_t nargs, Lisp_Object *args)
 {
   specpdl_ref count = SPECPDL_INDEX ();
-
-#ifdef WINDOWSNT
-  if (!json_initialized)
-    {
-      Lisp_Object status;
-      json_initialized = init_json_functions ();
-      status = json_initialized ? Qt : Qnil;
-      Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
-    }
-  if (!json_initialized)
-    Fsignal (Qjson_unavailable,
-	     list1 (build_unibyte_string ("jansson library not found")));
-#endif
+  ensure_json_available ();
 
   struct json_configuration conf =
     {json_object_hashtable, json_array_array, QCnull, QCfalse};
@@ -1137,6 +1121,9 @@ syms_of_json (void)
   DEFSYM (Qplist, "plist");
   DEFSYM (Qarray, "array");
 
+#ifdef WINDOWSNT
+  defsubr (&Sjson__available_p);
+#endif
   defsubr (&Sjson_serialize);
   defsubr (&Sjson_insert);
   defsubr (&Sjson_parse_string);

bug#60311: json-available-p: make dynamically correct for Windows

[Mattias Engdegård]

By the way, I'm entirely to blame for this flaw: a compiler bug only recently fixed (8bb8cc5b49) prevented the emission of a warning that would have alerted the author of `json-available-p` to the issue at hand.

bug#60311: json-available-p: make dynamically correct for Windows

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 25 Dec 2022 15:32:06 +0100
> 
> The implementation of `json-available-p`,
> 
>   (and (fboundp 'json-serialize)
>        (condition-case nil
>            (json-serialize t)
>          (:success t)
>          (json-unavailable nil))))
> 
> probably isn't quite right on Windows: `json-serialize` is pure so it will be called at compile time and the result, "true", used in the code (actually not even that since the result is never used). Thus, if libjansson could not be loaded during actual Emacs use (as opposed to when Emacs was built), this would never be detected and json-available-p would still return t.

Yes, you are right.  But please come up with a smaller changeset which
only changes what strictly needs to be changed.  Or if you want, I can
do this myself.

AFAIU, we just need a C implementation of json-available-p, since
doing this in Lisp doesn't work.  So that's the change I expect,
nothing more, nothing less.  Like we do with other optional libraries
and features, there's more than enough examples in the codebase.

Thanks.

bug#60311: json-available-p: make dynamically correct for Windows

[Eli Zaretskii]

> Cc: 60311@debbugs.gnu.org
> Date: Sun, 25 Dec 2022 17:40:47 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> AFAIU, we just need a C implementation of json-available-p, since
> doing this in Lisp doesn't work.

This came out too ambiguous.  So here's a more accurate request:
please make your json_available_p use the same code as the first part
of the below:

  #ifdef WINDOWSNT
    if (!json_initialized)
      {
	Lisp_Object status;
	json_initialized = init_json_functions ();
	status = json_initialized ? Qt : Qnil;
	Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
      }
    if (!json_initialized)
      Fsignal (Qjson_unavailable,
	       list1 (build_unibyte_string ("jansson library not found")));
  #endif

and your ensure_json_available use the second part of the above.
There's no reason to reshuffle the code that works.

Also, please make ensure_json_available specific to WINDOWSNT, so that
we don't pay an extra function call on Posix platforms.

And finally, json--available-p should be defined on all platforms that
compile json.c, not just on WINDOWSNT; it should return t on Posix
platforms.

Thanks.

bug#60311: json-available-p: make dynamically correct for Windows

[Eli Zaretskii]

> Cc: 60311@debbugs.gnu.org
> Date: Sun, 25 Dec 2022 18:24:54 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> This came out too ambiguous.  So here's a more accurate request:
> please make your json_available_p use the same code as the first part
> of the below:
> 
>   #ifdef WINDOWSNT
>     if (!json_initialized)
>       {
> 	Lisp_Object status;
> 	json_initialized = init_json_functions ();
> 	status = json_initialized ? Qt : Qnil;
> 	Vlibrary_cache = Fcons (Fcons (Qjson, status), Vlibrary_cache);
>       }
>     if (!json_initialized)
>       Fsignal (Qjson_unavailable,
> 	       list1 (build_unibyte_string ("jansson library not found")));
>   #endif
> 
> and your ensure_json_available use the second part of the above.
> There's no reason to reshuffle the code that works.
> 
> Also, please make ensure_json_available specific to WINDOWSNT, so that
> we don't pay an extra function call on Posix platforms.
> 
> And finally, json--available-p should be defined on all platforms that
> compile json.c, not just on WINDOWSNT; it should return t on Posix
> platforms.

The above is for the emacs-29 branch.  I think on master we should
compile json.c on all platforms, even if libjansson is not being
linked against, and then have json--available-p always fboundp, so
that test at run time will not be needed.

bug#60311: json-available-p: make dynamically correct for Windows

[Mattias Engdegård]

25 dec. 2022 kl. 16.40 skrev Eli Zaretskii <eliz@gnu.org>:

> Yes, you are right.  But please come up with a smaller changeset which
> only changes what strictly needs to be changed.  Or if you want, I can
> do this myself.

Since you have very specific ideas about how to go about it it's better that you make the change yourself. After all, attempts to remote-control someone else this way rarely make either party very happy.
As long as it gets done I'm not very particular about it myself.

>> +  (and (eval-when-compile (fboundp 'json-serialize))
>> +       ;; If `json--available-p' is present, we need to call it at run-time.
>> +       (or (not (eval-when-compile (fboundp 'json--available-p)))
>> +           (json--available-p))))
> 
> Btw, I don't understand this use of eval-when-compile here.  Can you
> explain why should we care at compile time whether these functions are
> fboundp?

Because we can: we already know the answers when subr.el is compiled, giving us constant nil or t values that will simplify the function to a constant or a call to json--available-p.

> why not just
> 
>  (and (fboundp 'json--available-p)
>       (json--available-p))

Calling `fboundp` at run time is wasteful, and that patch used the presence of json--available-p to indicate whether it needs to be called at run time (on Windows only, but it's good to keep that platform-dependency in one place).

It doesn't matter much if json-available-p is implemented in Lisp or C; even performance-wise it's a wash.

bug#60311: json-available-p: make dynamically correct for Windows

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Mon, 26 Dec 2022 13:11:21 +0100
> Cc: 60311@debbugs.gnu.org
> 
> 25 dec. 2022 kl. 16.40 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Yes, you are right.  But please come up with a smaller changeset which
> > only changes what strictly needs to be changed.  Or if you want, I can
> > do this myself.
> 
> Since you have very specific ideas about how to go about it it's better that you make the change yourself.

Done.

> > why not just
> > 
> >  (and (fboundp 'json--available-p)
> >       (json--available-p))
> 
> Calling `fboundp` at run time is wasteful, and that patch used the presence of json--available-p to indicate whether it needs to be called at run time (on Windows only, but it's good to keep that platform-dependency in one place).

Yes, but using eval-when-compile precludes defining json--available-p
at run time, something that I don't like doing in Emacs, since it's
IMO against the Emacs's spirit of being interpreter based.

So I went with the simpler definition that doesn't assume functions
are only defined at byte-compile time.

Thanks, I'm now closing the bug.

bug#60311: json-available-p: make dynamically correct for Windows

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Sun, 25 Dec 2022 15:32:06 +0100
> 
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -6905,11 +6905,11 @@ internal--format-docstring-line
>  
>  (defun json-available-p ()
>    "Return non-nil if Emacs has libjansson support."
> -  (and (fboundp 'json-serialize)
> -       (condition-case nil
> -           (json-serialize t)
> -         (:success t)
> -         (json-unavailable nil))))
> +  (declare (side-effect-free error-free))
> +  (and (eval-when-compile (fboundp 'json-serialize))
> +       ;; If `json--available-p' is present, we need to call it at run-time.
> +       (or (not (eval-when-compile (fboundp 'json--available-p)))
> +           (json--available-p))))

Btw, I don't understand this use of eval-when-compile here.  Can you
explain why should we care at compile time whether these functions are
fboundp?

IOW, why not just

  (and (fboundp 'json--available-p)
       (json--available-p))

?