Suppress illegal construction variables.

9 files changed, 149 insertions, 13 deletions

diff --git a/doc/man/scons.1 b/doc/man/scons.1
index 056b31e..53c6cb0 100644
--- a/doc/man/scons.1
+++ b/doc/man/scons.1
@@ -31,7 +31,7 @@
 .RE
 .fi
 ..
-.TH SCONS 1 "April 2003"
+.TH SCONS 1 "May 2003"
 .SH NAME
 scons \- a software construction tool
 .SH SYNOPSIS
@@ -2013,12 +2013,18 @@ env.SourceCode('no_source.c', None)
 .\"        env["CCCOM"] = "$CC $CFLAGS -o $TARGET $SOURCES
 .\"    Default:
 .\"        (I dunno what this is ;-)
-A construction environment has an associated dictionary of construction
-variables that are used by built-in or user-supplied build rules. A number
-of useful construction variables are automatically defined by scons for
-each supported platform, and additional construction variables can be defined
-by the user. The following is a list of the automatically defined construction
-variables:
+A construction environment has an associated dictionary of
+.I construction variables
+that are used by built-in or user-supplied build rules.
+Construction variables must follow the same rules for
+Python identifiers:
+the initial character must be an underscore or letter,
+followed by any number of underscores, letters, or digits.
+
+A number of useful construction variables are automatically defined by
+scons for each supported platform, and additional construction variables
+can be defined by the user. The following is a list of the automatically
+defined construction variables:
 
 .IP AR
 The static library archiver.
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")
diff --git a/test/bad-variables.py b/test/bad-variables.py
new file mode 100644
index 0000000..59fc184
--- /dev/null
+++ b/test/bad-variables.py
@@ -0,0 +1,60 @@
+#!/usr/bin/env python
+#
+# __COPYRIGHT__
+#
+# Permission is hereby granted, free of charge, to any person obtaining
+# a copy of this software and associated documentation files (the
+# "Software"), to deal in the Software without restriction, including
+# without limitation the rights to use, copy, modify, merge, publish,
+# distribute, sublicense, and/or sell copies of the Software, and to
+# permit persons to whom the Software is furnished to do so, subject to
+# the following conditions:
+#
+# The above copyright notice and this permission notice shall be included
+# in all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
+# KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE
+# WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+# NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+# LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+# OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
+# WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+#
+
+__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
+
+"""
+Test that setting illegal construction variables fails in ways that are
+useful to the user.
+"""
+
+import TestSCons
+
+test = TestSCons.TestSCons()
+
+test.write('SConstruct', """\
+env = Environment()
+env['foo-bar'] = 1
+""")
+
+test.run(arguments = '.', status = 2, stderr="""
+scons: *** Illegal construction variable `foo-bar'
+File "SConstruct", line 2, in ?
+""")
+
+test.write('SConstruct', """\
+SConscript('SConscript')
+""")
+
+test.write('SConscript', """\
+env = Environment()
+env['foo(bar)'] = 1
+""")
+
+test.run(arguments = '.', status = 2, stderr="""
+scons: *** Illegal construction variable `foo(bar)'
+File "SConscript", line 2, in ?
+""")
+
+test.pass_test()