bug#34233: 27.0.50; defvar in comment

bug#34233: 27.0.50; defvar in comment

[Sebastian Miele]

;; Start ./src/emacs -Q, put this into *scratch*, do not remove this
;; "(defvar x)" in this comment, and C-x C-e the following two
;; expressions in turn:
;;
(setq lexical-binding t)
(let ((x 42)) (lambda () x))
;;
;; Value: (closure (x t) nil x)
;; Expected: (closure ((x . 42) t) nil x)



In GNU Emacs 27.0.50 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.4)
 of 2019-01-28 built on tisch
Repository revision: d81aa8516ef6df79d6d602d4f732d8a65bf1677c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Arch Linux

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
Making completion list...
Mark set
next-line: End of buffer [4 times]
Mark set
t
(closure (x t) nil x)
Quit
Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 XDBE XIM THREADS LIBSYSTEMD JSON
CANNOT_DUMP LCMS2 GMP

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs time-date mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader cl-loaddefs cl-lib sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils elec-pair
mule-util tooltip eldoc electric uniquify ediff-hook vc-hooks
lisp-float-type mwheel term/x-win x-win term/common-win x-dnd tool-bar
dnd fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame 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 minibuffer
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 dbusbind
inotify lcms2 dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 45259 7117)
 (symbols 48 5860 1)
 (strings 32 14881 1022)
 (string-bytes 1 490862)
 (vectors 16 9474)
 (vector-slots 8 117324 9510)
 (floats 8 19 155)
 (intervals 56 279 0)
 (buffers 992 12))

bug#34233: 27.0.50; defvar in comment

[Lars Ingebrigtsen]

Sebastian Miele <sebastian.miele@gmail.com> writes:

> ;; Start ./src/emacs -Q, put this into *scratch*, do not remove this
> ;; "(defvar x)" in this comment, and C-x C-e the following two
> ;; expressions in turn:
> ;;
> (setq lexical-binding t)
> (let ((x 42)) (lambda () x))
> ;;
> ;; Value: (closure (x t) nil x)
> ;; Expected: (closure ((x . 42) t) nil x)

I can confirm that this bug is still present.

Stefan, do you know what's going on here?  That a commented-out (defvar)
before the eval-ed form has any effect is rather surprising...

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

bug#34233: 27.0.50; defvar in comment

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Sebastian Miele <sebastian.miele@gmail.com> writes:
>
> > ;; Start ./src/emacs -Q, put this into *scratch*, do not remove this
> > ;; "(defvar x)" in this comment, and C-x C-e the following two
> > ;; expressions in turn:
> > ;;
> > (setq lexical-binding t)
> > (let ((x 42)) (lambda () x))
> > ;;
> > ;; Value: (closure (x t) nil x)
> > ;; Expected: (closure ((x . 42) t) nil x)
>
> I can confirm that this bug is still present.
>
> Stefan, do you know what's going on here?  That a commented-out (defvar)
> before the eval-ed form has any effect is rather surprising...

This is `eval-sexp-add-defvars' not checking if defvars are really code.

Michael.

bug#34233: 27.0.50; defvar in comment

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> This is `eval-sexp-add-defvars' not checking if defvars are really code.

So is this the correct solution?

diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
index cb1b17b447..0e99cb9ac0 100644
--- a/lisp/progmodes/elisp-mode.el
+++ b/lisp/progmodes/elisp-mode.el
@@ -1179,7 +1179,13 @@ eval-sexp-add-defvars
           (let ((var (intern (match-string 1))))
             (and (not (special-variable-p var))
                  (save-excursion
-                   (zerop (car (syntax-ppss (match-beginning 0)))))
+                   (let ((syntax (syntax-ppss (match-beginning 0))))
+                     ;; Top-level.
+                     (and (zerop (car syntax))
+                          ;; Not in a comment.
+                          (null (nth 4 syntax))
+                          ;; Not in a string.
+                          (null (nth 3 syntax)))))
                  (push var vars))))
         `(progn ,@(mapcar (lambda (v) `(defvar ,v)) vars) ,exp)))))
 
(Those ppss things really should have accessors.  :-))

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

bug#34233: 27.0.50; defvar in comment

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Michael Heerdegen <michael_heerdegen@web.de> writes:
>
>> This is `eval-sexp-add-defvars' not checking if defvars are really code.
>
> So is this the correct solution?
>
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> index cb1b17b447..0e99cb9ac0 100644
> --- a/lisp/progmodes/elisp-mode.el
> +++ b/lisp/progmodes/elisp-mode.el
> @@ -1179,7 +1179,13 @@ eval-sexp-add-defvars
>            (let ((var (intern (match-string 1))))
>              (and (not (special-variable-p var))
>                   (save-excursion
> -                   (zerop (car (syntax-ppss (match-beginning 0)))))
> +                   (let ((syntax (syntax-ppss (match-beginning 0))))
> +                     ;; Top-level.
> +                     (and (zerop (car syntax))
> +                          ;; Not in a comment.
> +                          (null (nth 4 syntax))
> +                          ;; Not in a string.
> +                          (null (nth 3 syntax)))))
>                   (push var vars))))
>          `(progn ,@(mapcar (lambda (v) `(defvar ,v)) vars) ,exp)))))
>  
> (Those ppss things really should have accessors.  :-))

There is already a function for this particular case:
syntax-ppss-toplevel-pos.

bug#34233: 27.0.50; defvar in comment

[Lars Ingebrigtsen]

Noam Postavsky <npostavs@gmail.com> writes:

>> +                     ;; Top-level.
>> +                     (and (zerop (car syntax))
>> +                          ;; Not in a comment.
>> +                          (null (nth 4 syntax))
>> +                          ;; Not in a string.
>> +                          (null (nth 3 syntax)))))
>>                   (push var vars))))
>>          `(progn ,@(mapcar (lambda (v) `(defvar ,v)) vars) ,exp)))))

[...]

> There is already a function for this particular case:
> syntax-ppss-toplevel-pos.

Are you referring to just the (car syntax) bit or the nth 4/3 bit (which
are the same as nth 8, apparently?  (And which I've now committed.)
Feel free to touch it up if you want to, but I couldn't convince myself
that they were equivalent...

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

bug#34233: 27.0.50; defvar in comment

[Noam Postavsky]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> There is already a function for this particular case:
>> syntax-ppss-toplevel-pos.
>
> Are you referring to just the (car syntax) bit or the nth 4/3 bit (which
> are the same as nth 8, apparently?  (And which I've now committed.)
> Feel free to touch it up if you want to, but I couldn't convince myself
> that they were equivalent...

Okay, done.

5d713c89e9 2019-07-13T21:25:10-04:00 "Use syntax-ppss-toplevel-pos for defvar search (Bug#34233)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5d713c89e923fbc3192719792090cc40327b13a2

bug#34233: 27.0.50; defvar in comment

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So is this the correct solution?
>
> diff --git a/lisp/progmodes/elisp-mode.el b/lisp/progmodes/elisp-mode.el
> index cb1b17b447..0e99cb9ac0 100644
> --- a/lisp/progmodes/elisp-mode.el
> +++ b/lisp/progmodes/elisp-mode.el
> @@ -1179,7 +1179,13 @@ eval-sexp-add-defvars
>            (let ((var (intern (match-string 1))))
>              (and (not (special-variable-p var))
>                   (save-excursion
> -                   (zerop (car (syntax-ppss (match-beginning 0)))))
> +                   (let ((syntax (syntax-ppss (match-beginning 0))))
> +                     ;; Top-level.
> +                     (and (zerop (car syntax))
> +                          ;; Not in a comment.
> +                          (null (nth 4 syntax))
> +                          ;; Not in a string.
> +                          (null (nth 3 syntax)))))
>                   (push var vars))))
>          `(progn ,@(mapcar (lambda (v) `(defvar ,v)) vars) ,exp)))))

Yes, I think so, plus Noam's suggestion.

Michael.