Subst.py: fix bugs and improve substitution performance

CHANGES.txt

@@ -44,6 +44,32 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       new contributors. (Using Gemini AI)
     - Fix Appveyor scripting to install unavailable python versions when needed and use them
       for testing.
+    - Subst: Fixed ListSubber.expanded(), which never detected an already-expanded
+      string (dead code since its 2019 introduction), so fully-expanded values are
+      no longer recursively re-processed during scons_subst_list().
+    - Subst: scons_subst() and scons_subst_list() no longer leak a __builtins__
+      key into the construction environment's dictionary if an exception is
+      raised during substitution.
+    - Subst: the result of the inspect.signature() check for callable
+      construction variables is now cached per callable, speeding up expansion
+      of function-valued variables. Callables whose signature cannot be
+      determined (some C/builtin callables) are now treated as not matching
+      the (target, source, env, for_signature) convention instead of raising.
+    - Subst: variable values which are plain strings with no further '$'
+      expansions are now returned directly, skipping an unneeded dict copy
+      and recursive substitution pass. Combined, the substitution speedups
+      measured on a representative command line
+      ('$CC $CCFLAGS $CPPDEFINES $GEN -c -o $TARGET $SOURCES'):
+                            old         new       improvement
+        scons_subst         20.7 us     12.8 us   ~38% faster
+        scons_subst_list    37.4 us     25.1 us   ~33% faster
+    - Subst: a NameError raised during scons_subst_list() now includes the
+      name of the unknown variable in the error message.
+    - Subst: the overrides argument to scons_subst()/scons_subst_list() no
+      longer mutates a caller-supplied lvars dictionary; also removed mutable
+      default arguments. Removed Literal.__neq__, a misspelled (and therefore
+      never-invoked) version of __ne__; Python derives inequality from
+      Literal.__eq__.
 
   From Prabhu S. Khalsa:
     - Fix typo in preface
@@ -54,6 +80,9 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
 
   From Mats Wichmann:
     - Introduce some unit tests for the file locking utility routines
+    - Subst: Improved variable substitution by consolidating dictionary
+      merging operations, reducing unnecessary copy operations when no
+      TARGET/SOURCE variables or overrides need to be applied.
     - Reduce unneeded computation of overrides. The Mkdir builder used an
       unknown argument ('explain') on creation, causing it to be considered
       an override. Also, if override dict is empty, don't even call the

RELEASE.txt

@@ -63,6 +63,10 @@ IMPROVEMENTS
   an override. Also, if override dict is empty, don't even call the
   Override factory function.
 
+- Subst: Improved variable substitution by consolidating dictionary
+  merging operations, reducing unnecessary copy operations when no
+  TARGET/SOURCE variables or overrides need to be applied.
+
 - Test runner reworked to use Python logging; the portion of the test suite
   which tests the runner was adjusted to match.
 

SCons/Subst.py

@@ -27,6 +27,7 @@
 
 import re
 from collections import UserList, UserString
+from functools import lru_cache
 from inspect import signature, Parameter
 
 import SCons.Errors
@@ -45,7 +46,7 @@
 
 def SetAllowableExceptions(*excepts) -> None:
     global AllowableExceptions
-    AllowableExceptions = [_f for _f in excepts if _f]
+    AllowableExceptions = tuple(_f for _f in excepts if _f)
 
 
 def raise_exception(exception, target, s):
@@ -84,9 +85,6 @@ def __eq__(self, other) -> bool:
             return False
         return self.lstr == other.lstr
 
-    def __neq__(self, other) -> bool:
-        return not self.__eq__(other)
-
     def __hash__(self) -> int:
         return hash(self.lstr)
 
@@ -340,6 +338,49 @@ def get_src_subst_proxy(node):
 
 _callable_args_set = {'target', 'source', 'env', 'for_signature'}
 
+
+def _check_callable_subst_args(s) -> bool:
+    """Check if callable *s* matches the subst calling convention.
+
+    A callable construction variable must be callable with exactly the
+    arguments (target, source, env, for_signature); any additional
+    parameters must have default values (which also allows
+    functools.partial objects to work).
+    """
+    try:
+        params = signature(s).parameters.items()
+    except (ValueError, TypeError):
+        # signature() can fail on some C/builtin callables; treat them
+        # as not matching our calling convention.
+        return False
+    return {
+        k for k, v in params if k in _callable_args_set or v.default is Parameter.empty
+    } == _callable_args_set
+
+
+_callable_subst_args_cache: dict = {}
+
+
+def _callable_matches_subst_args(s) -> bool:
+    """Cached version of :func:`_check_callable_subst_args`.
+
+    Inspecting a signature is expensive and the same callables are
+    expanded over and over (once or more per target), so cache the
+    result per callable.  The cache holds strong references, but these
+    callables are construction-variable values which normally live as
+    long as the build anyway.
+    """
+    try:
+        return _callable_subst_args_cache[s]
+    except KeyError:
+        pass
+    except TypeError:
+        # unhashable callable: do the check each time
+        return _check_callable_subst_args(s)
+    result = _callable_subst_args_cache[s] = _check_callable_subst_args(s)
+    return result
+
+
 class StringSubber:
     """A class to construct the results of a scons_subst() call.
 
@@ -382,9 +423,8 @@ def expand(self, s, lvars):
                 return s
             else:
                 key = s[1:]
-                if key[0] == '{' or '.' in key:
-                    if key[0] == '{':
-                        key = key[1:-1]
+                if key[0] == '{':
+                    key = key[1:-1]
 
                 # Store for error messages if we fail to expand the
                 # value
@@ -409,20 +449,18 @@ def expand(self, s, lvars):
                 elif s is None:
                      return ''
 
+                # A plain string with no more expansions needs no
+                # further processing, so skip the copy/recursion below.
+                if isinstance(s, str) and '$' not in s:
+                    return s
+
                 # Before re-expanding the result, handle
                 # recursive expansion by copying the local
                 # variable dictionary and overwriting a null
                 # string for the value of the variable name
                 # we just expanded.
-                #
-                # This could potentially be optimized by only
-                # copying lvars when s contains more expansions,
-                # but lvars is usually supposed to be pretty
-                # small, and deeply nested variable expansions
-                # are probably more the exception than the norm,
-                # so it should be tolerable for now.
                 lv = lvars.copy()
-                var = key.split('.')[0]
+                var = key.partition('.')[0]
                 lv[var] = ''
                 return self.substitute(s, lv)
         elif is_Sequence(s):
@@ -436,8 +474,7 @@ def func(l, conv=self.conv, substitute=self.substitute, lvars=lvars):
             # string if called on, so we make an exception in this condition for Null class
             # Also allow callables where the only non default valued args match the expected defaults
             # this should also allow functools.partial's to work.
-            if isinstance(s, SCons.Util.Null) or {k for k, v in signature(s).parameters.items() if
-                                                  k in _callable_args_set or v.default == Parameter.empty} == _callable_args_set:
+            if isinstance(s, SCons.Util.Null) or _callable_matches_subst_args(s):
 
                 s = s(target=lvars['TARGETS'],
                      source=lvars['SOURCES'],
@@ -526,12 +563,19 @@ def expanded(self, s) -> bool:
         method is used to determine if a string is already fully
         expanded and if so exit the loop early to prevent these
         recursive calls.
+
+        Only a string without any ``$`` (no further expansion) and
+        without any whitespace (no word-splitting needed) can safely
+        be appended directly as a single word.
         """
         if not is_String(s) or isinstance(s, CmdStringHolder):
             return False
 
         s = str(s)  # in case it's a UserString
-        return _separate_args.findall(s) is None
+        if not s:
+            # an empty string must not be appended as an empty word
+            return False
+        return _unexpandable.search(s) is None
 
     def expand(self, s, lvars, within_list):
         """Expand a single "token" as necessary, appending the
@@ -561,9 +605,8 @@ def expand(self, s, lvars, within_list):
                 self.close_strip('$)')
             else:
                 key = s[1:]
-                if key.startswith('{') or '.' in key:
-                    if key.startswith('{'):
-                        key = key[1:-1]
+                if key[0] == '{':
+                    key = key[1:-1]
 
                 # Store for error messages if we fail to expand the
                 # value
@@ -584,7 +627,7 @@ def expand(self, s, lvars, within_list):
                          raise_exception(e, lvars['TARGETS'], old_s)
 
                 if s is None and NameError not in AllowableExceptions:
-                     raise_exception(NameError(), lvars['TARGETS'], old_s)
+                     raise_exception(NameError(key), lvars['TARGETS'], old_s)
                 elif s is None:
                      return
 
@@ -600,7 +643,7 @@ def expand(self, s, lvars, within_list):
                 # string for the value of the variable name
                 # we just expanded.
                 lv = lvars.copy()
-                var = key.split('.')[0]
+                var = key.partition('.')[0]
                 lv[var] = ''
                 self.substitute(s, lv, 0)
                 self.this_word()
@@ -614,8 +657,7 @@ def expand(self, s, lvars, within_list):
             # string if called on, so we make an exception in this condition for Null class
             # Also allow callables where the only non default valued args match the expected defaults
             # this should also allow functools.partial's to work.
-            if isinstance(s, SCons.Util.Null) or {k for k, v in signature(s).parameters.items() if
-                                                  k in _callable_args_set or v.default == Parameter.empty} == _callable_args_set:
+            if isinstance(s, SCons.Util.Null) or _callable_matches_subst_args(s):
 
                 s = s(target=lvars['TARGETS'],
                      source=lvars['SOURCES'],
@@ -815,12 +857,16 @@ def _remove_list(list):
 _dollar_exps = re.compile(r'(%s)' % _dollar_exps_str)
 _separate_args = re.compile(r'(%s|\s+|[^\s$]+|\$)' % _dollar_exps_str)
 
+# Matches strings which may need further expansion ('$') or
+# word-splitting (whitespace); see ListSubber.expanded().
+_unexpandable = re.compile(r'[\s$]')
+
 # This regular expression is used to replace strings of multiple white
 # space characters in the string result from the scons_subst() function.
 _space_sep = re.compile(r'[\t ]+(?![^{]*})')
 
 
-def scons_subst(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars={}, lvars={}, conv=None, overrides: dict | None = None):
+def scons_subst(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars=None, lvars=None, conv=None, overrides: dict | None = None):
     """Expand a string or list containing construction variable
     substitutions.
 
@@ -832,6 +878,11 @@ def scons_subst(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars={
     if (isinstance(strSubst, str) and '$' not in strSubst) or isinstance(strSubst, CmdStringHolder):
         return strSubst
 
+    if gvars is None:
+        gvars = {}
+    if lvars is None:
+        lvars = {}
+
     if conv is None:
         conv = _strconv[mode]
 
@@ -844,31 +895,31 @@ def scons_subst(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars={
     # If we dropped that behavior (or found another way to cover it),
     # we could get rid of this call completely and just rely on the
     # Executor setting the variables.
+    # Build any special TARGET/SOURCE vars and apply overrides.
+    # Only copy the caller's lvars once if we need to modify it.
+    d = {}
     if 'TARGET' not in lvars:
         d = subst_dict(target, source)
-        if d:
-            lvars = lvars.copy()
-            lvars.update(d)
-
-    # Allow last ditch chance to override lvars
-    if overrides:
-        lvars.update(overrides)
+    if d or overrides:
+        lvars = {**lvars, **d, **(overrides or {})}
 
     # We're (most likely) going to eval() things.  If Python doesn't
     # find a __builtins__ value in the global dictionary used for eval(),
     # it copies the current global values for you.  Avoid this by
     # setting it explicitly and then deleting, so we don't pollute the
     # construction environment Dictionary(ies) that are typically used
-    # for expansion.
+    # for expansion.  gvars is usually the live env dict, so make sure
+    # the deletion happens even if substitution raises.
     gvars['__builtins__'] = __builtins__
 
     ss = StringSubber(env, mode, conv, gvars)
-    result = ss.substitute(strSubst, lvars)
-
     try:
-        del gvars['__builtins__']
-    except KeyError:
-        pass
+        result = ss.substitute(strSubst, lvars)
+    finally:
+        try:
+            del gvars['__builtins__']
+        except KeyError:
+            pass
 
     res = result
     if is_String(result):
@@ -902,14 +953,19 @@ def scons_subst(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars={
 
     return result
 
-def scons_subst_list(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars={}, lvars={}, conv=None, overrides: dict | None = None):
+def scons_subst_list(strSubst, env, mode=SUBST_RAW, target=None, source=None, gvars=None, lvars=None, conv=None, overrides: dict | None = None):
     """Substitute construction variables in a string (or list or other
     object) and separate the arguments into a command list.
 
     The companion scons_subst() function (above) handles basic
     substitutions within strings, so see that function instead
     if that's what you're looking for.
     """
+    if gvars is None:
+        gvars = {}
+    if lvars is None:
+        lvars = {}
+
     if conv is None:
         conv = _strconv[mode]
 
@@ -928,25 +984,28 @@ def scons_subst_list(strSubst, env, mode=SUBST_RAW, target=None, source=None, gv
             lvars = lvars.copy()
             lvars.update(d)
 
-    # Allow caller to specify last ditch override of lvars
+    # Allow caller to specify last ditch override of lvars; don't
+    # mutate the caller's dictionary doing so.
     if overrides:
-        lvars.update(overrides)
+        lvars = {**lvars, **overrides}
 
     # We're (most likely) going to eval() things.  If Python doesn't
     # find a __builtins__ value in the global dictionary used for eval(),
     # it copies the current global values for you.  Avoid this by
     # setting it explicitly and then deleting, so we don't pollute the
     # construction environment Dictionary(ies) that are typically used
-    # for expansion.
+    # for expansion.  gvars is usually the live env dict, so make sure
+    # the deletion happens even if substitution raises.
     gvars['__builtins__'] = __builtins__
 
     ls = ListSubber(env, mode, conv, gvars)
-    ls.substitute(strSubst, lvars, 0)
-
     try:
-        del gvars['__builtins__']
-    except KeyError:
-        pass
+        ls.substitute(strSubst, lvars, 0)
+    finally:
+        try:
+            del gvars['__builtins__']
+        except KeyError:
+            pass
 
     return ls.data