Detect setting locals with subshell commands

ianw · ianw · commit 1d949d6e3efe · 2015-10-06T20:30:04.000+11:00
The return status of "local" is always 0, so something like

 local foo=$(failing command)

doesn't trigger a failure under "set -e".  Missing "failing command"'s
failure has been a source of problems within devstack.

This is added as a warning

Change-Id: Ia0957b47187c3dcadd46154b17022c4213781112
diff --git a/README.rst b/README.rst
@@ -40,12 +40,14 @@ unrolling that is kind of "interesting"
 - E012: heredoc didn't end before EOF
 - E020: Function declaration not in format ``^function name {$``
 
-Obsolete and deprecated syntax
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Obsolete, deprecated or unsafe syntax
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-Rules to identify obsolete and deprecated syntax that should not be used
+Rules to identify obsolete, deprecated or unsafe syntax that should
+not be used
 
 - E041: Usage of $[ for arithmetic is deprecated for $((
+- E042: local declaration hides errors
 
 See also
 ~~~~~~~~
diff --git a/bashate/bashate.py b/bashate/bashate.py
@@ -98,6 +98,12 @@ def check_arithmetic(line, report):
         report.print_error(MESSAGES['E041'].msg, line)
 
 
+def check_local_subshell(line, report):
+    if line.lstrip().startswith('local ') and \
+       ("=$(" in line or "=`" in line):
+        report.print_error(MESSAGES['E042'].msg, line)
+
+
 def check_hashbang(line, filename, report):
     # this check only runs on the first line
     #  maybe this should check for shell?
@@ -249,6 +255,7 @@ def check_files(self, files, verbose):
                 check_if_then(logical_line, report)
                 check_function_decl(logical_line, report)
                 check_arithmetic(logical_line, report)
+                check_local_subshell(logical_line, report)
 
                 prev_line = logical_line
                 prev_lineno = fileinput.filelineno()
diff --git a/bashate/messages.py b/bashate/messages.py
@@ -154,6 +154,16 @@ def msg(self):
         """,
         'default': 'E'
     },
+    'E042': {
+        'msg': 'local declaration hides errors',
+        'long_msg':
+        """
+        The return value of "local" is always 0; errors in subshells
+        used for declaration are thus hidden and will not trigger "set -e".
+        """,
+        'default': 'W',
+    },
+
 }
 
 MESSAGES = {}
diff --git a/bashate/tests/samples/E042_bad.sh b/bashate/tests/samples/E042_bad.sh
@@ -0,0 +1,10 @@
+function hello {
+    local foo=$(ls)
+}
+
+function hello_too {
+    local foo=`ls`
+}
+
+hello
+hello_too
diff --git a/bashate/tests/test_bashate.py b/bashate/tests/test_bashate.py
@@ -196,6 +196,14 @@ def test_sample_E041(self):
 
         self.assert_error_found('E041', 4)
 
+    def test_sample_E042(self):
+        test_files = ['bashate/tests/samples/E042_bad.sh']
+        self.run.register_errors('E042')
+        self.run.check_files(test_files, False)
+
+        self.assert_error_found('E042', 2)
+        self.assert_error_found('E042', 6)
+
     def test_sample_for_loops(self):
         test_files = ['bashate/tests/samples/for_loops.sh']
         self.run.check_files(test_files, False)
