Re: [Emacs-diffs] master 5f72004: Revert "Fix command repetition with lexical-binding (Bug#29334)"

Re: [Emacs-diffs] master 5f72004: Revert "Fix command repetition with lexical-binding (Bug#29334)"

[Stefan Monnier]

> --- a/src/callint.c
> +++ b/src/callint.c
> @@ -357,9 +357,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
>        /* Compute the arg values using the user's expression.  */
>        specs = Feval (specs,
>   		     CONSP (funval) && EQ (Qclosure, XCAR (funval))
> -                     ? CAR_SAFE (XCDR (funval))
> -                     : COMPILEDP (funval) && INTEGERP (AREF (funval, COMPILED_ARGLIST))
> -                     ? Qt : Qnil);
> +		     ? CAR_SAFE (XCDR (funval)) : Qnil);
>        if (events != num_input_events || !NILP (record_flag))
>  	{
>  	  /* We should record this command on the command history.  */

Why?  This looks wrong.


        Stefan

Re: [Emacs-diffs] master 5f72004: Revert "Fix command repetition with lexical-binding (Bug#29334)"

[Noam Postavsky]

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

On Sun, Jan 7, 2018 at 10:36 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> --- a/src/callint.c
>> +++ b/src/callint.c
>> @@ -357,9 +357,7 @@ invoke it.  If KEYS is omitted or nil, the return value of
>>        /* Compute the arg values using the user's expression.  */
>>        specs = Feval (specs,
>>                    CONSP (funval) && EQ (Qclosure, XCAR (funval))
>> -                     ? CAR_SAFE (XCDR (funval))
>> -                     : COMPILEDP (funval) && INTEGERP (AREF (funval, COMPILED_ARGLIST))
>> -                     ? Qt : Qnil);
>> +                  ? CAR_SAFE (XCDR (funval)) : Qnil);

> Why?  This looks wrong.

Hmm, I just reverted to the previous code without actually thinking
about it. But after checking this more closely, I find that passing
non-nil LEXICAL to `eval' doesn't really do anything for compiled
functions anyway: the "lexicalness" has already been applied when
compiling. E.g., the above change has no effect on the attached tests.

[-- Attachment #2: callint-tests.el --]
[-- Type: application/octet-stream, Size: 2374 bytes --]

;;; callint-tests.el --- tests for src/callint.c -*- lexical-binding: t -*-

;; Copyright (C) 2018 Free Software Foundation, Inc.

;; This file is part of GNU Emacs.

;; GNU Emacs is free software: you can redistribute it and/or modify
;; it under the terms of the GNU General Public License as published by
;; the Free Software Foundation, either version 3 of the License, or
;; (at your option) any later version.

;; GNU Emacs is distributed in the hope that it will be useful,
;; but WITHOUT ANY WARRANTY; without even the implied warranty of
;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
;; GNU General Public License for more details.

;; You should have received a copy of the GNU General Public License
;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.

;;; Commentary:

;;; Code:

(require 'ert)
(require 'bytecomp)


;; NOTE: We're purposely checking the dynamic value of the symbol
;; `callint-tests-x' (it should be void), which names a *lexical*
;; variable.

(ert-deftest callint-interactive-form-binding-1 ()
  "Test that interactive form evaluation uses lexical binding."
  (let ((fun
         (lambda (a b)
           (interactive
            (list (bound-and-true-p callint-tests-x)
                  (funcall (let* ((callint-tests-x 42))
                             (lambda () callint-tests-x)))))
           (list a b))))
    (ert-info ("interp")
      (should (equal (call-interactively fun)
                     '(nil 42))))
    (setq fun (byte-compile fun))
    (ert-info ("compiled")
      (should (equal (call-interactively fun)
                    '(nil 42))))))

(ert-deftest callint-interactive-form-binding-2 ()
  "Test that interactive form evaluation uses lexical binding."
  ;; Outer lexical environment is not preserved inside interactive
  ;; forms, so this test fails.
  :expected-result :failed
  (let ((fun
         (let ((callint-tests-x 42))
           (lambda (a b)
             (interactive (list (bound-and-true-p callint-tests-x)
                                callint-tests-x))
             (list a b callint-tests-x)))))
    (ert-info ("interp")
      (should (equal (call-interactively fun)
                     '(nil 42 42))))
    (ert-info ("compiled")
      (should (equal (call-interactively (byte-compile fun))
                    '(nil 42 42))))))

;;; callint-tests.el ends here

Re: [Emacs-diffs] master 5f72004: Revert "Fix command repetition with lexical-binding (Bug#29334)"

[Stefan Monnier]

> Hmm, I just reverted to the previous code without actually thinking
> about it. But after checking this more closely, I find that passing
> non-nil LEXICAL to `eval' doesn't really do anything for compiled
> functions anyway: the "lexicalness" has already been applied when
> compiling. E.g., the above change has no effect on the attached tests.

If the interactive spec has been compiled (as was always the case until
now), then indeed it shouldn't make a difference.  But if the byte-code
object was built by hand (or with the change you intended to install)
the interactive spec could be non-compiled, in which case the LEXICAL
arg to `eval` can have an effect.

So yes, it's not super important, but I think this part of your patch
should not have been reverted.


        Stefan