Subst.py: fix bugs and improve substitution performance

[bdbaddog]

TODO:

• - try godot build
• - try wesnoth build

Bug fixes:

• ListSubber.expanded() never detected an already-expanded string (re.findall() never returns None), so the early-exit optimization added in 2019 was dead code. Fixed with a check that is only true when a value needs neither further '$' expansion nor word-splitting, guarding against appending empty values as empty words.
• scons_subst()/scons_subst_list() no longer leak a __builtins__ key into the live construction environment dictionary when substitution raises (deletion now in a try/finally).
• inspect.signature() failures on some C/builtin callables no longer crash; such callables are treated as not matching the (target, source, env, for_signature) convention.
• NameError raised during scons_subst_list() now includes the name of the unknown variable in the message.
• The overrides argument no longer mutates a caller-supplied lvars dict; removed mutable default arguments.
• Removed Literal.__neq__, a misspelled (never-invoked) __ne__; Python 3 derives != from Literal.__eq__.

Performance:

• Cache the inspect.signature() check per callable (~100x faster on that check).
• Return plain strings with no further '$' expansions directly in StringSubber.expand(), skipping a dict copy and recursive pass.
• str.partition() instead of str.split() for the recursion-guard key.

Measured on a representative command line
($CC $CCFLAGS $CPPDEFINES $GEN -c -o $TARGET $SOURCES), identical output before/after:

function	old	new	improvement
scons_subst	20.7 us	12.8 us	~38% faster
scons_subst_list	37.4 us	25.1 us	~33% faster

Adds 7 regression tests covering each fix. Full test suite passes (remaining failures are pre-existing environment-dependent tests, verified identical against unmodified HEAD).

Contributor Checklist:

• I have created a new test or updated the unit tests to cover the new/changed functionality.
• I have updated CHANGES.txt and RELEASE.txt (and read the README.rst).
• I have updated the appropriate documentation

[mwichmann]

so we just don't need the check for "attribute access"?

[bdbaddog]

The original logic was checking for { or . and then checking for { and then chopping the bookended {}'s off and doing nothing with the .

The . gets handled by not being in lvars or gvars and thus it gets eval'd

It's never used to shortcut checking lvars or gvars, so is kinda pointless here.

[mwichmann]

why get rid of startswith? And we don't need the attribute-access check?

[bdbaddog]

same comment as above about .

it's more typing? Not exactly sure, but is there any reason to use startswith instead of just checking the 1 character for a single character?

[mwichmann]

functionally no; readability-wise I think yes. I hate using indexing-slicing with just "magic numbers"; we can't avoid them with node lists where stuff[0] is magical and doesn't have an alternative, but I do like starts/endswith on strings. Not a big deal, though.

[mwichmann]

This is a long-standing checker flag - mutable default arg. Using None as a sentinel and then adding checks is the usual solution - we've changed this in a few other places. Is there an actual benefit to the change? We don't change gvars (well, we change it temporarily).

[bdbaddog]

There was an error condition where gvars or lvars could get set and not cleared and then cause issues on next call. (see the notes I posted in discord?)

[mwichmann]

Maybe there's a way to restruture this plus the above - because in at least one flow we could now remake lvars twice - once from the copy, then here from the repacking to account for the override.

[mwichmann]

Here's a proposed rejig for this:

    # 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 or overrides:
        lvars = {**lvars, **d, **overrides}

If this approach seems okay, I can make a new PR once this one is resolved - since this is essentially a new request let's not muddy this one. There's a small tweak needed in ActionTests in any case, the inconsistency there doesn't break until this proposed change makes it stricter that overrides needs to be a dict.

[bdbaddog]

AI says

Good idea — it's cleaner and saves a copy, but the snippet as written has one bug: overrides defaults to None, and {**None} raises TypeError. It needs **(overrides or {}):

      d = {}
      if 'TARGET' not in lvars:
          d = subst_dict(target, source)
      if d or overrides:
          lvars = {**lvars, **d, **(overrides or {})}

  With that fix it's strictly equal-or-better than what's there now:

  - Both apply (TARGET missing and overrides given): current code copies twice (lvars.copy() + update, then {**lvars, **overrides}); yours builds one merged dict.
  - Only one applies: same single copy either way.
  - Neither applies: no copy either way.
  - Semantics are identical, including overrides winning over subst_dict keys (same precedence order).

  One side note: subst_dict() always returns a non-empty dict (it always sets TARGET/TARGETS/SOURCE/SOURCES, even to NullNodesList), so the if d: truthiness check — both in the current code and in your version — is effectively always true when the 'TARGET' not in
  lvars branch runs. Your if d or overrides: keeps it as a harmless guard.

[mwichmann]

yeah, I know it needs more scaffolding at the top. I didn't say it was a complete PR, though it works in my copy here.

[mwichmann]

Oh, didn't read this all. I'd rather put the overrides stuff at the top of the function, though this works as well. With the other entry-point checks:

if overrides is None:
    overrides = {}

There's a small hange needed to correct an error in ActionTests.py if we do it my way though, as its local subst functions default overrides to False (i.e., bool instead of dict) before calling on to scons_subst.

[mwichmann]

same comments as for scons_subst

[mwichmann]

This new version would apply equally to scons_subst_list.

[bdbaddog]

Done..