bug#46534: Lexical change in bindat breaks weechat.el

[Stefan Monnier]

> Indeed, I can confirm this fixes the test file as well as weechat.el proper, 
> thanks!

Thanks, closing for now, tho I'd love to see a better fix
(e.g. changing the system so it doesn't rely on `eval` and dynamic
scoping so much).


        Stefan


> Sent from ProtonMail mobile
>
> -------- Original Message --------
> On 15 Feb 2021, 15:50, Stefan Monnier < monnier@iro.umontreal.ca> wrote:
>
>  [ Hi Kim, long time no see.
>  I'd appreciate your opinion on this issue with bindat.el. ]
>
>  > (defconst minrepro--str-spec
>  > '((len u32)
>  > (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
>  > (bindat-get-field struct 'len)
>  > 4)))
>  > ;; Hack for signed/unsigned problems
>  > (if (<= len 0) 0 len))))))
>
>  Hmm... the doc of bindat.el does not include `struct` among the vars you
>  can use in `eval`.
>
>  OTOH, a variable which you can use is `last` and it indeed contains
>  exactly the info you need from `struct`, so you can rewrite the above to:
>
>  (defconst minrepro--str-spec
>  '((len u32)
>  (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
>  last 4)))
>  ;; Hack for signed/unsigned problems
>  (if (<= len 0) 0 len))))))
>
>  > (defconst minrepro--message-spec
>  > '((length u32)
>  > (compression u8)
>  > (id struct minrepro--str-spec)
>  > (data vec (eval (let ((l (- (bindat-get-field struct 'length)
>  > 4 ;length
>  > 1 ;compression
>  > (+ 4 (length (bindat-get-field struct 'id 'val))))))
>  > l)))))
>
>  This one OTOH can't just use `last` since that only gives us the `id`
>  field but not the `length` field :-(
>
>  I can't see any way to do what you want given the documentation found in
>  the `Commentary:` of `bindat.el`, so I guess we do need to extend the
>  documented functionality.
>
>  I installed the patch below, for now. It fixes the problem in your test
>  case and hopefully in other cases as well. Please confirm.
>
>  Stefan
>
>  diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
>  index 0d9ba57d66..bf01347ae0 100644
>  --- a/lisp/emacs-lisp/bindat.el
>  +++ b/lisp/emacs-lisp/bindat.el
>  @@ -26,7 +26,7 @@
>  ;; Packing and unpacking of (binary) data structures.
>  ;;
>  ;; The data formats used in binary files and network protocols are
>  -;; often structed data which can be described by a C-style structure
>  +;; often structured data which can be described by a C-style structure
>  ;; such as the one shown below. Using the bindat package, decoding
>  ;; and encoding binary data formats like these is made simple using a
>  ;; structure specification which closely resembles the C style
>  @@ -135,7 +135,8 @@
>  ;; | ( [FIELD] repeat COUNT ITEM... )
>
>  ;; -- In (eval EXPR), the value of the last field is available in
>  -;; the dynamically bound variable `last'.
>  +;; the dynamically bound variable `last' and all the previous
>  +;; ones in the variable `struct'.
>
>  ;; TYPE ::= ( eval EXPR ) -- interpret result as TYPE
>  ;; | u8 | byte -- length 1
>  @@ -191,7 +192,7 @@
>  ;;; Code:
>
>  ;; Helper functions for structure unpacking.
>  -;; Relies on dynamic binding of BINDAT-RAW and BINDAT-IDX
>  +;; Relies on dynamic binding of `bindat-raw' and `bindat-idx'.
>
>  (defvar bindat-raw)
>  (defvar bindat-idx)
>  @@ -276,8 +277,8 @@ bindat--unpack-item
>  (t nil)))
>
>  (defun bindat--unpack-group (spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  (let (struct last)
>  (while spec
>  (let* ((item (car spec))
>  @@ -378,9 +379,9 @@ bindat--fixed-length-alist
>  (ip . 4)))
>
>  (defun bindat--length-group (struct spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  - (let (last)
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  + (let ((struct struct) last)
>  (while spec
>  (let* ((item (car spec))
>  (field (car item))
>  @@ -544,9 +545,9 @@ bindat--pack-item
>  (setq bindat-idx (+ bindat-idx len)))))
>
>  (defun bindat--pack-group (struct spec)
>  - (with-suppressed-warnings ((lexical last))
>  - (defvar last))
>  - (let (last)
>  + (with-suppressed-warnings ((lexical struct last))
>  + (defvar struct) (defvar last))
>  + (let ((struct struct) last)
>  (while spec
>  (let* ((item (car spec))
>  (field (car item))