Try and detect common problems in lexical code

Try and detect common problems in lexical code

[Stefan Monnier]

One of the most common problem with the use of lexical binding seems to
be when we do something like:

    (let ((byte-compile-dest-file-function …))
      (byte-compile ...))

because the file that defines `byte-compile-dest-file-function` might be
loaded after the let binding is created (i.e. in response to the call
to `byte-compile`).

We already try to handle the same problem with dynamic bindings (where
the problem was that `byte-compile-dest-file-function` revert to being
unbound once we exit the `let`; problem we fixed by making `defvar`
affect the `default-toplevel-value` rather than the `default-value`),

The patch below does something similar for the lexical case.  It's only
*similar* because for the dynamic scoping case we found a solution that
tries to guess the real intent and hence makes the code work, whereas
the patch below only detects the problem and signals an error.  Maybe we
could actually try to also guess the intent and fix the problem on the
spot, but that seems more delicate, so I haven't tried to go there yet.

Another difference is that this probably has a more noticeable
performance impact: the dynamic case needs to walk the specpdl stack
only when the var already has a value when we get to `defvar` (which is
the less usual case), whereas here we have to walk the the specpdl stack
when the var does not yet have a value (which is the usual case), and on
top of that, our walk is more expensive.

WDYT?


        Stefan


diff --git a/src/eval.c b/src/eval.c
index 4559a0e1f6..40ad0a2136 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -688,6 +688,46 @@ default_toplevel_binding (Lisp_Object symbol)
   return binding;
 }
 
+/* Look for a lexical-binding of SYMBOL somewhere up the stack.
+   This will only find bindings created with interpreted code, since once
+   compiled names of lexical variables are basically gone anyway.  */
+static bool
+lexbound_p (Lisp_Object symbol)
+{
+  union specbinding *binding = NULL;
+  union specbinding *pdl = specpdl_ptr;
+  while (pdl > specpdl)
+    {
+      switch ((--pdl)->kind)
+	{
+	case SPECPDL_LET_DEFAULT:
+	case SPECPDL_LET:
+	  if (EQ (specpdl_symbol (pdl), Qinternal_interpreter_environment))
+	    {
+	      Lisp_Object env = specpdl_old_value (pdl);
+	      if (CONSP (env) && !NILP (Fassq (symbol, env)))
+	        return true;
+	    }
+	  break;
+
+	case SPECPDL_UNWIND:
+	case SPECPDL_UNWIND_ARRAY:
+	case SPECPDL_UNWIND_PTR:
+	case SPECPDL_UNWIND_INT:
+	case SPECPDL_UNWIND_INTMAX:
+	case SPECPDL_UNWIND_EXCURSION:
+	case SPECPDL_UNWIND_VOID:
+	case SPECPDL_BACKTRACE:
+	case SPECPDL_LET_LOCAL:
+	  break;
+
+	default:
+	  emacs_abort ();
+	}
+    }
+  return false;
+}
+
 DEFUN ("default-toplevel-value", Fdefault_toplevel_value, Sdefault_toplevel_value, 1, 1, 0,
        doc: /* Return SYMBOL's toplevel default value.
 "Toplevel" means outside of any let binding.  */)
@@ -723,6 +763,15 @@ DEFUN ("internal--define-uninitialized-variable",
 value.  */)
   (Lisp_Object symbol, Lisp_Object doc)
 {
+  if (!XSYMBOL (symbol)->u.s.declared_special
+      && lexbound_p (symbol))
+    /* This test tries to catch the situation where we do
+       (let ((<foo-var> ...)) ...(<foo-function> ...)....)
+       and where the `foo` package only gets loaded when <foo-function>
+       is called, so the outer `let` incorrectly made the binding lexical
+       because the <foo-var> wasn't defined yet at that point.  */
+    error ("Defining as dynamic an already lexical var");
+
   XSYMBOL (symbol)->u.s.declared_special = true;
   if (!NILP (doc))
     {

Re: Try and detect common problems in lexical code

[Clément Pit-Claudel]

Well, that was fast! :)

> Another difference is that this probably has a more noticeable
> performance impact

Shouldn't the cost be essentially 0 when (defvar) happens "at the toplevel", within an empty specdpl stack?  And isn't that the common case (a defvar is a toplevel form in a file loaded by a `require')?

Clément.

Re: Try and detect common problems in lexical code

[Stefan Monnier]

>> Another difference is that this probably has a more noticeable
>> performance impact
> Shouldn't the cost be essentially 0 when (defvar) happens "at the toplevel",
> within an empty specdpl stack?  And isn't that the common case (a defvar is
> a toplevel form in a file loaded by a `require')?

specpdl holds all the current dynamic let-bindings, unwind_protects (and
their variants), and the call backtrace info, so it's never really empty
(e.g. `load` itself installs a handful of unwind_protects and
let-bindings on the specpdl stack around the execution of the actual
loaded code).

With the use of lexical binding (especially after byte-compilation), the
specpdl is typically less deep, indeed.  And at the top-level of loading
a file it's usually not terribly deep (unless it's an autoload that
happens in the middle of a deepish recursion).  So there's a chance the cost
won't be significant.

I just wanted to be clear that there is a possibility that the cost can
be above the noise-level in some cases (e.g. files with many defvars
that get (auto)loaded while we're in the middle of a deepish recursion).
I haven't bothered to measure it at all, tho.


        Stefan

Re: Try and detect common problems in lexical code

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > because the file that defines `byte-compile-dest-file-function` might be
  > loaded after the let binding is created (i.e. in response to the call
  > to `byte-compile`).

ISTR that in lexically-scoped languages you usually have to load
an interface spec for a program before you can call it.
Maybe that is the right direction to go, for this.

Maybe it is possible to have a function 'lexical-require'
which is like 'require' except that, instead of _loading_ the
file require, it only prepares to call things in that file.

-- 
Dr Richard Stallman
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Try and detect common problems in lexical code

[Clément Pit-Claudel]

On 18/03/2020 20.00, Stefan Monnier wrote:
>>> Another difference is that this probably has a more noticeable
>>> performance impact
>> Shouldn't the cost be essentially 0 when (defvar) happens "at the toplevel",
>> within an empty specdpl stack?  And isn't that the common case (a defvar is
>> a toplevel form in a file loaded by a `require')?
> 
> specpdl holds all the current dynamic let-bindings, unwind_protects (and
> their variants), and the call backtrace info, so it's never really empty
> (e.g. `load` itself installs a handful of unwind_protects and
> let-bindings on the specpdl stack around the execution of the actual
> loaded code).
> 
> With the use of lexical binding (especially after byte-compilation), the
> specpdl is typically less deep, indeed.  And at the top-level of loading
> a file it's usually not terribly deep (unless it's an autoload that
> happens in the middle of a deepish recursion).  So there's a chance the cost
> won't be significant.
> 
> I just wanted to be clear that there is a possibility that the cost can
> be above the noise-level in some cases (e.g. files with many defvars
> that get (auto)loaded while we're in the middle of a deepish recursion).
> I haven't bothered to measure it at all, tho.

Btw, we already do something like this in defvaralias, to make sure that the variable being aliased isn't already let-bound.