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