Suppress illegal construction variables.

7 files changed, 76 insertions, 6 deletions

diff --git a/src/CHANGES.txt b/src/CHANGES.txt
index 0cbe2c3..79178f5 100644
--- a/src/CHANGES.txt
+++ b/src/CHANGES.txt
@@ -12,6 +12,9 @@ RELEASE 0.15 - XXX
 
   From Steven Knight:
 
+  - SCons now enforces (with an error) that construction variables
+    must have the same form as valid Python identifiers.
+
 
 
 RELEASE 0.14 - Wed, 21 May 2003 05:16:32 -0500
diff --git a/src/engine/SCons/Environment.py b/src/engine/SCons/Environment.py
index f317556..ba5e279 100644
--- a/src/engine/SCons/Environment.py
+++ b/src/engine/SCons/Environment.py
@@ -314,10 +314,7 @@ class Environment:
 	return dlist
 
     def __setitem__(self, key, value):
-        if key == 'TARGET' or \
-           key == 'TARGETS' or \
-           key == 'SOURCE' or \
-           key == 'SOURCES':
+        if key in ['TARGET', 'TARGETS', 'SOURCE', 'SOURCES']:
             SCons.Warnings.warn(SCons.Warnings.ReservedVariableWarning,
                                 "Ignoring attempt to set reserved variable `%s'" % key)
         elif key == 'BUILDERS':
@@ -329,6 +326,8 @@ class Environment:
                 self._dict[key] = BuilderDict(kwbd, self)
             self._dict[key].update(value)
         else:
+            if not SCons.Util.is_valid_construction_var(key):
+                raise SCons.Errors.UserError, "Illegal construction variable `%s'" % key
             self._dict[key] = value
 
     def __getitem__(self, key):
diff --git a/src/engine/SCons/EnvironmentTests.py b/src/engine/SCons/EnvironmentTests.py
index 6473bdd..aed3a9e 100644
--- a/src/engine/SCons/EnvironmentTests.py
+++ b/src/engine/SCons/EnvironmentTests.py
@@ -374,6 +374,22 @@ class EnvironmentTestCase(unittest.TestCase):
         finally:
             SCons.Warnings.warningAsException(old)
 
+    def test_IllegalVariables(self):
+        """Test that use of illegal variables raises an exception"""
+        env = Environment()
+        def test_it(var, env=env):
+            exc_caught = None
+            try:
+                env[var] = 1
+            except SCons.Errors.UserError:
+                exc_caught = 1
+            assert exc_caught, "did not catch UserError for '%s'" % var
+        env['aaa'] = 1
+        assert env['aaa'] == 1, env['aaa']
+        test_it('foo/bar')
+        test_it('foo.bar')
+        test_it('foo-bar')
+
     def test_Replace(self):
         """Test replacing construction variables in an Environment
 
diff --git a/src/engine/SCons/Options.py b/src/engine/SCons/Options.py
index 2aae5fc..9d315b8 100644
--- a/src/engine/SCons/Options.py
+++ b/src/engine/SCons/Options.py
@@ -67,6 +67,9 @@ class Options:
                     putting it in the environment.
         """
 
+        if not SCons.Util.is_valid_construction_var(key):
+            raise SCons.Errors.UserError, "Illegal Options.Add() key `%s'" % key
+
         class Option:
             pass
 
diff --git a/src/engine/SCons/OptionsTests.py b/src/engine/SCons/OptionsTests.py
index 491845e..93705c0 100644
--- a/src/engine/SCons/OptionsTests.py
+++ b/src/engine/SCons/OptionsTests.py
@@ -79,6 +79,17 @@ class OptionsTestCase(unittest.TestCase):
         assert o.default == "42"
         o.validater(o.key, o.converter(o.default), {})
 
+        def test_it(var, opts=opts):
+            exc_caught = None
+            try:
+                opts.Add(var)
+            except SCons.Errors.UserError:
+                exc_caught = 1
+            assert exc_caught, "did not catch UserError for '%s'" % var
+        test_it('foo/bar')
+        test_it('foo-bar')
+        test_it('foo.bar')
+
     def test_Update(self):
 
         test = TestSCons.TestSCons()
diff --git a/src/engine/SCons/Util.py b/src/engine/SCons/Util.py
index 3b97907..2ebe0d9 100644
--- a/src/engine/SCons/Util.py
+++ b/src/engine/SCons/Util.py
@@ -181,14 +181,21 @@ class NodeList(UserList.UserList):
     def is_literal(self):
         return 1
 
-_env_var = re.compile(r'^\$([_a-zA-Z]\w*|{[_a-zA-Z]\w*})$')
+_valid_var = re.compile(r'[_a-zA-Z]\w*$')
+_get_env_var = re.compile(r'^\$([_a-zA-Z]\w*|{[_a-zA-Z]\w*})$')
+
+def is_valid_construction_var(varstr):
+    """Return if the specified string is a legitimate construction
+    variable.
+    """
+    return _valid_var.match(varstr)
 
 def get_environment_var(varstr):
     """Given a string, first determine if it looks like a reference
     to a single environment variable, like "$FOO" or "${FOO}".
     If so, return that variable with no decorations ("FOO").
     If not, return None."""
-    mo=_env_var.match(to_String(varstr))
+    mo=_get_env_var.match(to_String(varstr))
     if mo:
         var = mo.group(1)
         if var[0] == '{':
diff --git a/src/engine/SCons/UtilTests.py b/src/engine/SCons/UtilTests.py
index 84655a9..7f5f166 100644
--- a/src/engine/SCons/UtilTests.py
+++ b/src/engine/SCons/UtilTests.py
@@ -571,6 +571,37 @@ class UtilTestCase(unittest.TestCase):
             wi = WhereIs('xxx', path = forward_slash, pathext = '.EXE')
             assert string.lower(wi) == string.lower(test.workpath(sub3_xxx_exe)), wi
 
+    def test_is_valid_construction_var(self):
+        """Testing is_valid_construction_var()"""
+        r = is_valid_construction_var("_a")
+        assert not r is None, r
+        r = is_valid_construction_var("z_")
+        assert not r is None, r
+        r = is_valid_construction_var("X_")
+        assert not r is None, r
+        r = is_valid_construction_var("2a")
+        assert r is None, r
+        r = is_valid_construction_var("a2_")
+        assert not r is None, r
+        r = is_valid_construction_var("/")
+        assert r is None, r
+        r = is_valid_construction_var("_/")
+        assert r is None, r
+        r = is_valid_construction_var("a/")
+        assert r is None, r
+        r = is_valid_construction_var(".b")
+        assert r is None, r
+        r = is_valid_construction_var("_.b")
+        assert r is None, r
+        r = is_valid_construction_var("b1._")
+        assert r is None, r
+        r = is_valid_construction_var("-b")
+        assert r is None, r
+        r = is_valid_construction_var("_-b")
+        assert r is None, r
+        r = is_valid_construction_var("b1-_")
+        assert r is None, r
+
     def test_get_env_var(self):
         """Testing get_environment_var()."""
         assert get_environment_var("$FOO") == "FOO", get_environment_var("$FOO")