bug#6376: 23.2; byte compile add-to-list report free variable

bug#6376: 23.2; byte compile add-to-list report free variable

[Kevin Ryde]

I made a mistake with a variable name to add-to-list and wondered if the
byte compiler might report

    (add-to-list 'nosuchvariable ...)

as a free variable, similar to the report for

    (set 'nosuchvariable ...)

The few lines below get the desired effect, but might be a bit rough.
The separate check-assign is with a view to sharing among set, setq,
set-default and maybe others.

Something specific for add-to-list would be vaguely reasonable since
it's a builtin, but it could be worth trying a general way to flag an
arg number as the name of a variable either referenced or assigned.
Something like that might also allow "function not known to be defined"
for quoted symbol args to mapcar, sort, etc.

I suppose this sort of thing might be analysed by elint instead or as
well, but the byte compiler has good information about bindings
available after macro expansion etc and it's much more often run than
elint.



(defun byte-compile-check-assign (symbol)
  (cond ((and (byte-compile-warning-enabled-p 'constants)
              (byte-compile-const-symbol-p symbol t))
         (byte-compile-warn "variable assignment to constant `%s'" symbol))

        ((and (byte-compile-warning-enabled-p 'free-vars)
              (not (memq symbol byte-compile-bound-variables))
              (not (memq symbol byte-compile-free-assignments)))
         (byte-compile-warn "assignment to free variable `%s'" symbol)
         (push symbol byte-compile-free-assignments))))

(defun byte-compile-varsym1 (form)
  ;; arg 1 is a symbol which is the name of a variable which must be found
  ;; eg. (foo 'var ...)
  (cond ((eq 'quote (car-safe (nth 1 form)))
         (let ((var (car-safe (cdr (nth 1 form)))))
           (if (symbolp var)
               (byte-compile-check-assign var))))
        ;; (add-to-list 'nil ...) reaches here as (add-to-list nil ...),
        ;; call the check so as to report nil as a constant
        ((null (nth 1 form))
         (byte-compile-check-assign nil)))
  (byte-compile-normal-call form))

(byte-defop-compiler-1 add-to-list         byte-compile-varsym1)
(byte-defop-compiler-1 add-to-ordered-list byte-compile-varsym1)



In GNU Emacs 23.2.1 (i486-pc-linux-gnu, GTK+ Version 2.20.0)
 of 2010-05-16 on raven, modified by Debian
configured using `configure  '--build' 'i486-linux-gnu' '--build' 'i486-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib' '--libexecdir=/usr/lib' '--localstatedir=/var/lib' '--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes' '--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.2/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.2/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.2/leim' '--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars' 'build_alias=i486-linux-gnu' 'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_AU
  value of $XMODIFIERS: nil
  locale-coding-system: iso-latin-1-unix
  default enable-multibyte-characters: t

bug#6376: 23.2; byte compile add-to-list report free variable

[Stefan Monnier]

> I made a mistake with a variable name to add-to-list and wondered if the
> byte compiler might report

>     (add-to-list 'nosuchvariable ...)

> as a free variable, similar to the report for

I use the code below for add-hook:

   (byte-defop-compiler-1 remove-hook byte-compile-add-hook)
   (defun byte-compile-add-hook (form)
     (let ((sym (car-safe (cdr-safe form))))
       (when (and (eq 'quote (car-safe sym))
                  (setq sym (car-safe (cdr sym)))
                  (symbolp sym))
         ;; Gross hack: We want to do the sanity checks just as we would for
         ;; a setq so we first do a setq and then pop the byte-code that was
         ;; just pushed by byte-compile-variable-ref.
         (let ((byte-compile-output byte-compile-output)
               (byte-compile-maxdepth byte-compile-maxdepth)
               (byte-compile-depth byte-compile-depth))
           (byte-compile-variable-ref 'byte-varset sym)))
       (byte-compile-normal-call form)))

so I'm in favor of such a change, but it would need to be cleaned up such
that your byte-compile-check-assign is also used for
byte-compile-variable-ref (i.e. the code should be hoisted out of
byte-compile-variable-ref).  Also, note that add-to-list is not only
a "varset" but also a "varref" (it reads the var before assigning to it).


        Stefan

bug#6376: 23.2; byte compile add-to-list report free variable

[Kevin Ryde]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
> add-hook

I was thinking of excusing add-hook and remove-hook because they can
work on an unbound variable, and might quite often do so if tying in to
something external.

> add-to-list is not only a "varset" but also a "varref"

I thought to show the message about setting in that case.  For the
unboundness check the two only differ in the wording I think.

bug#6376: 23.2; byte compile add-to-list report free variable

[Kevin Ryde]

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

I got a bit further to these few lines.  Not quite ready yet, but
getting closer.  The byte-compile-check-var is more or less a break-out
of the checks in byte-compile-variable-ref.

add-hook and remove-hook end up with checks against non-variables, but
they don't demand a bound variable.  I suppose they could helpfully also
notice undefined functions in their second arg too.  You'd be tempted to
have some sort of general arg-type description for the builtin funcs
rather than doing checks plus "normal-call" for each.


[-- Attachment #2: bytecomp-add-to-list.el --]
[-- Type: application/emacs-lisp, Size: 3933 bytes --]

bug#6376: 23.2; byte compile add-to-list report free variable

[Stefan Monnier]

> I got a bit further to these few lines.  Not quite ready yet, but
> getting closer.  The byte-compile-check-var is more or less a break-out
> of the checks in byte-compile-variable-ref.

Could you send it as a patch, please.
Which part(s) so you think still need work?


        Stefan

bug#6376: 23.2; byte compile add-to-list report free variable

[Kevin Ryde]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
> Could you send it as a patch, please.

I suppose I should get my cvs head checkout sorted so as to be able to
test on the current code.  That's likely to take a while :-(

> Which part(s) so you think still need work?

I thought to make room for checking the func arg to add-hook and
friends.  But checking the variables might be enough to start with.

bug#6376: 23.2; byte compile add-to-list report free variable

[Stefan Kangas]

Kevin Ryde <user42@zip.com.au> writes:

> I got a bit further to these few lines.  Not quite ready yet, but
> getting closer.  The byte-compile-check-var is more or less a break-out
> of the checks in byte-compile-variable-ref.
>
> add-hook and remove-hook end up with checks against non-variables, but
> they don't demand a bound variable.  I suppose they could helpfully also
> notice undefined functions in their second arg too.  You'd be tempted to
> have some sort of general arg-type description for the builtin funcs
> rather than doing checks plus "normal-call" for each.

(That was 10 years ago.)

This change looks useful to me.  Any chance you could fix it up and send
it as a patch?

> (require 'bytecomp)
>
> (defun byte-compile-check-var (symbol base-op)
>   ;; SYMBOL is the name of a variable.  BASE-OP is one of the following
>   ;; according to what's being done to the variable.
>   ;;    'byte-varref
>   ;;    'byte-varset
>   ;;    'byte-varbind
>   ;;    'varset-def    -- set, defining if not already (eg. add-hook)
>   ;; Emit a warning if SYMBOL is unbound, or if it's a constant being set or
>   ;; bound.
>   ;;
>   (cond ((and (not (symbolp symbol))
>               ;; `constants' warning includes all non-variables
>               (byte-compile-warning-enabled-p 'constants))
>          (byte-compile-warn "%s non-variable `%S'"
>                             (assoc-default base-op
>                                            '((byte-varref . "reference to")
>                                              (byte-varset . "assignment to")
>                                              (varset-def  . "assignment to")
>                                              (byte-varbind . "let-bind of"))
>                                            nil base-op)
>                             symbol))
>
>         ((and (not (eq base-op 'byte-varref))
>               (byte-compile-warning-enabled-p 'constants)
>               (byte-compile-const-symbol-p symbol t))
>          (byte-compile-warn "%s constant `%S'"
>                             (assoc-default
>                              base-op
>                              '((byte-varset . "variable assignment to")
>                                (varset-def  . "variable assignment to")
>                                (byte-varbind . "let-bind of"))
>                              nil base-op)
>                             symbol))
>
>         ((and (not (memq base-op '(varset-def byte-varbind)))
>               (byte-compile-warning-enabled-p 'free-vars)
>               (not (memq symbol byte-compile-bound-variables))
>               (not (memq symbol byte-compile-free-assignments)))
>          (byte-compile-warn "%s to free variable `%S'"
>                             (assoc-default base-op
>                                            '((byte-varref . "reference to")
>                                              (byte-varset . "assignment to"))
>                                            nil base-op)
>                             symbol)
>          (push symbol byte-compile-free-assignments))))
>
> (defun byte-compile-check-argvar (arg base-op)
>   ;; ARG is a function argument form which is supposed to evaluate to a
>   ;; symbol naming a variable.  If ARG is (quote FOO) or :foo then check
>   ;; that it's a bound variable per bytecomp-check-var.  If ARG is
>   ;; self-evaluating like nil, t, strings, etc then pass to the check too,
>   ;; to possibly report assignment to a constant.  Code like (quote nil) or
>   ;; (quote "foo") reaches this point as plain nil or t.
>   ;;
>   (cond ((eq 'quote (car-safe arg))
>          (byte-compile-check-var (car-safe (cdr arg)) base-op))
>         ((or (memq arg '(nil t))
>              ;; anything except a symbol or list is self-evaluating
>              (not (or (symbolp arg)
>                       (consp arg))))
>          (byte-compile-check-var arg base-op))))
>
> (defun byte-compile-addtolist (form)
>   ;; first arg is the name of a variable being changed, eg. (foo 'var ...)
>   (if (>= (safe-length form) 2)
>       (byte-compile-check-argvar (cadr form) 'byte-varset))
>   (byte-compile-normal-call form))
> (byte-defop-compiler-1 add-to-list         byte-compile-addtolist)
> (byte-defop-compiler-1 add-to-ordered-list byte-compile-addtolist)
>
> (defun byte-compile-addremhook (form)
>   ;; first arg is the name of a variable being changed, eg. (foo 'var ...)
>   ;; only
>   (if (>= (safe-length form) 2)
>       (byte-compile-check-argvar (cadr form) 'varset-def))
>   (byte-compile-normal-call form))
> (byte-defop-compiler-1 add-hook    byte-compile-addremhook)
> (byte-defop-compiler-1 remove-hook byte-compile-addremhook)

bug#6376: 23.2; byte compile add-to-list report free variable

[Stefan Kangas]

Stefan Kangas <stefan@marxist.se> writes:

> This change looks useful to me.  Any chance you could fix it up and send
> it as a patch?

The email bounced, so it seems like someone else will have to continue
from here.

bug#6376: 23.2; byte compile add-to-list report free variable

[Lars Ingebrigtsen]

Kevin Ryde <user42@zip.com.au> writes:

> I made a mistake with a variable name to add-to-list and wondered if the
> byte compiler might report
>
>     (add-to-list 'nosuchvariable ...)
>
> as a free variable, similar to the report for
>
>     (set 'nosuchvariable ...)

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I tried this byte-compiling such a file in Emacs 29, and I got:

Compiling file /tmp/foo.el at Mon Jan 31 17:56:17 2022
foo.el:2:15: Warning: reference to free variable ‘nosuchvariable’
foo.el:2:15: Warning: assignment to free variable ‘nosuchvariable’

So I guess it's been fixed at some point in the decade since this was
reported, and I'm therefore closing this bug report.  (If there's more
to be done, please respond to the debbugs address and we'll reopen.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#6376: 23.2; byte compile add-to-list report free variable

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I tried this byte-compiling such a file in Emacs 29, and I got:
>
> Compiling file /tmp/foo.el at Mon Jan 31 17:56:17 2022
> foo.el:2:15: Warning: reference to free variable ‘nosuchvariable’
> foo.el:2:15: Warning: assignment to free variable ‘nosuchvariable’
>
> So I guess it's been fixed at some point in the decade since this was
> reported [...]

The function got a compiler macro 2013 (bfa3acd65b), and the expansion
is either a `push' or a `setq' form (which support generating free
variable warnings), so this should have fixed this issue.

Michael.