Fix bug with unique adds and delete_existing

AppendUnique and PrependUnique, when called with delete_existing true, had
a small logic flaw: it might remove an existing value if the value to be
added is a scalar (string, most likely) and there was a substring match.
The code needs to do an equality test, not a membership test.

Unit tests updated and got some reformatting, plus dropped a duplicate
definition of reserved_variables.

Signed-off-by: Mats Wichmann <mats@linux.com>

Author: mwichmann

CHANGES.txt

@@ -40,6 +40,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Improve wording of manpage "Functions and Environment Methods" section.
       Make doc function signature style more consistent - tweaks to AddOption,
       DefaultEnvironment and Tool,.
+    - Fix a problem with AppendUnique and PrependUnique where a value could
+      be erroneously removed due to a substring match.
 
 
 RELEASE 4.8.0 -  Sun, 07 Jul 2024 17:22:20 -0700

RELEASE.txt

@@ -46,6 +46,8 @@ FIXES
   to be avaiable. `BoolVariable`, `EnumVariable`, `ListVariable`,
   `PackageVariable` and `PathVariable` are added to `__all__`,
   so this form of import should now work again.
+- Fix a problem with AppendUnique and PrependUnique where a value could
+  be erroneously removed due to a substring match.
 
 IMPROVEMENTS
 ------------

SCons/Environment.py

@@ -1512,11 +1512,17 @@ def AppendENVPath(self, name, newpath, envname: str='ENV',
 
         self._dict[envname][name] = nv
 
-    def AppendUnique(self, delete_existing: bool=False, **kw) -> None:
-        """Append values to existing construction variables
-        in an Environment, if they're not already there.
-        If delete_existing is True, removes existing values first, so
-        values move to end.
+    def AppendUnique(self, delete_existing: bool = False, **kw) -> None:
+        """Append values uniquely to existing construction variables.
+
+        Similar to :meth:`Append`, but the result may not contain duplicates
+        of any values passed for each given key (construction variable),
+        so an existing list may need to be pruned first, however it may still
+        contain other duplicates.
+
+        If *delete_existing* is true, removes existing values first, so values
+        move to the end; otherwise (the default) values are skipped if
+        already present.
         """
         kw = copy_non_reserved_keywords(kw)
         for key, val in kw.items():
@@ -1539,12 +1545,11 @@ def AppendUnique(self, delete_existing: bool=False, **kw) -> None:
                     val = [x for x in val if x not in dk]
                 self._dict[key] = dk + val
             else:
+                # val is not a list, so presumably a scalar (likely str).
                 dk = self._dict[key]
                 if is_List(dk):
-                    # By elimination, val is not a list.  Since dk is a
-                    # list, wrap val in a list first.
                     if delete_existing:
-                        dk = list(filter(lambda x, val=val: x not in val, dk))
+                        dk = [x for x in dk if x != val]
                         self._dict[key] = dk + [val]
                     else:
                         if val not in dk:
@@ -1939,11 +1944,17 @@ def PrependENVPath(self, name, newpath, envname: str='ENV',
 
         self._dict[envname][name] = nv
 
-    def PrependUnique(self, delete_existing: bool=False, **kw) -> None:
-        """Prepend values to existing construction variables
-        in an Environment, if they're not already there.
-        If delete_existing is True, removes existing values first, so
-        values move to front.
+    def PrependUnique(self, delete_existing: bool = False, **kw) -> None:
+        """Prepend values uniquely to existing construction variables.
+
+        Similar to :meth:`Prepend`, but the result may not contain duplicates
+        of any values passed for each given key (construction variable),
+        so an existing list may need to be pruned first, however it may still
+        contain other duplicates.
+
+        If *delete_existing* is true, removes existing values first, so values
+        move to the front; otherwise (the default) values are skipped if
+        already present.
         """
         kw = copy_non_reserved_keywords(kw)
         for key, val in kw.items():
@@ -1966,12 +1977,11 @@ def PrependUnique(self, delete_existing: bool=False, **kw) -> None:
                     val = [x for x in val if x not in dk]
                 self._dict[key] = val + dk
             else:
+                # val is not a list, so presumably a scalar (likely str).
                 dk = self._dict[key]
                 if is_List(dk):
-                    # By elimination, val is not a list.  Since dk is a
-                    # list, wrap val in a list first.
                     if delete_existing:
-                        dk = [x for x in dk if x not in val]
+                        dk = [x for x in dk if x != val]
                         self._dict[key] = [val] + dk
                     else:
                         if val not in dk:

SCons/EnvironmentTests.py

@@ -1193,18 +1193,6 @@ def test_ENV(self) -> None:
 
     def test_ReservedVariables(self) -> None:
         """Test warning generation when reserved variable names are set"""
-
-        reserved_variables = [
-            'CHANGED_SOURCES',
-            'CHANGED_TARGETS',
-            'SOURCE',
-            'SOURCES',
-            'TARGET',
-            'TARGETS',
-            'UNCHANGED_SOURCES',
-            'UNCHANGED_TARGETS',
-        ]
-
         warning = SCons.Warnings.ReservedVariableWarning
         SCons.Warnings.enableWarningClass(warning)
         old = SCons.Warnings.warningAsException(1)
@@ -1759,19 +1747,26 @@ def test_AppendENVPath(self) -> None:
             ENV={'PATH': r'C:\dir\num\one;C:\dir\num\two'},
             MYENV={'MYPATH': r'C:\mydir\num\one;C:\mydir\num\two'},
         )
+
         # have to include the pathsep here so that the test will work on UNIX too.
         env1.AppendENVPath('PATH', r'C:\dir\num\two', sep=';')
         env1.AppendENVPath('PATH', r'C:\dir\num\three', sep=';')
-        env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
         assert (
             env1['ENV']['PATH'] == r'C:\dir\num\one;C:\dir\num\two;C:\dir\num\three'
         ), env1['ENV']['PATH']
 
+        # add nonexisting - at end
         env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
+        assert (
+            env1['MYENV']['MYPATH'] == r'C:\mydir\num\one;C:\mydir\num\two;C:\mydir\num\three'
+        ), env1['MYENV']['MYPATH']
+
+        # add existing with delete_existing true - moves to the end
         env1.AppendENVPath(
-            'MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';', delete_existing=1
+            'MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';', delete_existing=True
         )
-        # this should do nothing since delete_existing is 0
+        # this should do nothing since delete_existing is false (the default)
+        env1.AppendENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
         assert (
             env1['MYENV']['MYPATH'] == r'C:\mydir\num\two;C:\mydir\num\three;C:\mydir\num\one'
         ), env1['MYENV']['MYPATH']
@@ -1783,6 +1778,7 @@ def test_AppendENVPath(self) -> None:
         env1.AppendENVPath('PATH', env1.fs.Dir('sub2'), sep=';')
         assert env1['ENV']['PATH'] == p + ';sub1;sub2', env1['ENV']['PATH']
 
+
     def test_AppendUnique(self) -> None:
         """Test appending to unique values to construction variables
 
@@ -1832,34 +1828,46 @@ def test_AppendUnique(self) -> None:
         assert env['CCC1'] == 'c1', env['CCC1']
         assert env['CCC2'] == ['c2'], env['CCC2']
         assert env['DDD1'] == ['a', 'b', 'c'], env['DDD1']
-        assert env['LL1']  == [env.Literal('a literal'), env.Literal('b literal')], env['LL1']
-        assert env['LL2']  == [env.Literal('c literal'), env.Literal('b literal'), env.Literal('a literal')], [str(x) for x in env['LL2']]
+        assert env['LL1'] == [env.Literal('a literal'), \
+            env.Literal('b literal')], env['LL1']
+        assert env['LL2'] == [
+            env.Literal('c literal'),
+            env.Literal('b literal'),
+            env.Literal('a literal'),
+        ], [str(x) for x in env['LL2']]
+
+        env.AppendUnique(DDD1='b', delete_existing=True)
+        assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1']  # b moves to end
 
-        env.AppendUnique(DDD1 = 'b', delete_existing=1)
-        assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # b moves to end
-        env.AppendUnique(DDD1 = ['a','b'], delete_existing=1)
-        assert env['DDD1'] == ['c', 'a', 'b'], env['DDD1'] # a & b move to end
-        env.AppendUnique(DDD1 = ['e','f', 'e'], delete_existing=1)
-        assert env['DDD1'] == ['c', 'a', 'b', 'f', 'e'], env['DDD1'] # add last
+        env.AppendUnique(DDD1=['a', 'b'], delete_existing=True)
+        assert env['DDD1'] == ['c', 'a', 'b'], env['DDD1']  # a & b move to end
+
+        env.AppendUnique(DDD1=['e', 'f', 'e'], delete_existing=True)
+        assert env['DDD1'] == ['c', 'a', 'b', 'f', 'e'], env['DDD1']  # add last
+
+        # issue regression: substrings should not be deleted
+        env.AppendUnique(BBB4='b4.newer', delete_existing=True)
+        assert env['BBB4'] == ['b4', 'b4.new', 'b4.newer'], env['BBB4']
 
         env['CLVar'] = CLVar([])
-        env.AppendUnique(CLVar = 'bar')
+        env.AppendUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['bar'], result
 
         env['CLVar'] = CLVar(['abc'])
-        env.AppendUnique(CLVar = 'bar')
+        env.AppendUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['abc', 'bar'], result
 
         env['CLVar'] = CLVar(['bar'])
-        env.AppendUnique(CLVar = 'bar')
+        env.AppendUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['bar'], result
 
+
     def test_Clone(self) -> None:
         """Test construction environment cloning.
 
@@ -2501,18 +2509,26 @@ def test_PrependENVPath(self) -> None:
             ENV={'PATH': r'C:\dir\num\one;C:\dir\num\two'},
             MYENV={'MYPATH': r'C:\mydir\num\one;C:\mydir\num\two'},
         )
+
         # have to include the pathsep here so that the test will work on UNIX too.
         env1.PrependENVPath('PATH', r'C:\dir\num\two', sep=';')
         env1.PrependENVPath('PATH', r'C:\dir\num\three', sep=';')
         assert (
             env1['ENV']['PATH'] == r'C:\dir\num\three;C:\dir\num\two;C:\dir\num\one'
         ), env1['ENV']['PATH']
 
+
+        # add nonexisting - at front
         env1.PrependENVPath('MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';')
+        assert (
+            env1['MYENV']['MYPATH'] == r'C:\mydir\num\three;C:\mydir\num\one;C:\mydir\num\two'
+        ), env1['MYENV']['MYPATH']
+
+        # add existing - moves to the front
         env1.PrependENVPath('MYPATH', r'C:\mydir\num\one', 'MYENV', sep=';')
-        # this should do nothing since delete_existing is 0
+        # this should do nothing since delete_existing is false
         env1.PrependENVPath(
-            'MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';', delete_existing=0
+            'MYPATH', r'C:\mydir\num\three', 'MYENV', sep=';', delete_existing=False
         )
         assert (
             env1['MYENV']['MYPATH'] == r'C:\mydir\num\one;C:\mydir\num\three;C:\mydir\num\two'
@@ -2525,6 +2541,7 @@ def test_PrependENVPath(self) -> None:
         env1.PrependENVPath('PATH', env1.fs.Dir('sub2'), sep=';')
         assert env1['ENV']['PATH'] == 'sub2;sub1;' + p, env1['ENV']['PATH']
 
+
     def test_PrependUnique(self) -> None:
         """Test prepending unique values to construction variables
 
@@ -2570,32 +2587,36 @@ def test_PrependUnique(self) -> None:
         assert env['CCC2'] == ['c2'], env['CCC2']
         assert env['DDD1'] == ['a', 'b', 'c'], env['DDD1']
 
-        env.PrependUnique(DDD1 = 'b', delete_existing=1)
-        assert env['DDD1'] == ['b', 'a', 'c'], env['DDD1'] # b moves to front
-        env.PrependUnique(DDD1 = ['a','c'], delete_existing=1)
-        assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1'] # a & c move to front
-        env.PrependUnique(DDD1 = ['d','e','d'], delete_existing=1)
+        env.PrependUnique(DDD1='b', delete_existing=True)
+        assert env['DDD1'] == ['b', 'a', 'c'], env['DDD1']  # b moves to front
+        env.PrependUnique(DDD1=['a', 'c'], delete_existing=True)
+        assert env['DDD1'] == ['a', 'c', 'b'], env['DDD1']  # a & c move to front
+        env.PrependUnique(DDD1=['d', 'e', 'd'], delete_existing=True)
         assert env['DDD1'] == ['d', 'e', 'a', 'c', 'b'], env['DDD1']
 
+        # issue regression: substrings should not be deleted
+        env.PrependUnique(BBB4='b4.newer', delete_existing=True)
+        assert env['BBB4'] == ['b4.newer', 'b4.new', 'b4'], env['BBB4']
 
         env['CLVar'] = CLVar([])
-        env.PrependUnique(CLVar = 'bar')
+        env.PrependUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['bar'], result
 
         env['CLVar'] = CLVar(['abc'])
-        env.PrependUnique(CLVar = 'bar')
+        env.PrependUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['bar', 'abc'], result
 
         env['CLVar'] = CLVar(['bar'])
-        env.PrependUnique(CLVar = 'bar')
+        env.PrependUnique(CLVar='bar')
         result = env['CLVar']
         assert isinstance(result, CLVar), repr(result)
         assert result == ['bar'], result
 
+
     def test_Replace(self) -> None:
         """Test replacing construction variables in an Environment