From c977b0827d876948f2aa80af715a241b44bab35d Mon Sep 17 00:00:00 2001
From: Ian Wienand <iwienand@redhat.com>
Date: Tue, 29 Aug 2017 11:24:51 +1000
Subject: [PATCH] Add a [[ checker

This adds a basic check for [[ when using non-POSIX comparisions such
as =~, <, >.  This is a fairly common typo often not picked up until
runtime.  Since it's usually part of an "if [ $foo =~ bar ]; then"
statement it doesn't trigger "-e" and the failure just falls through.

As mentioned in the code; this is one of those things that starts
requiring a complete bash parser to get 100% right.  However, I
believe this detects the most common failures without setting off
false alarms.

Change-Id: I5478032800cdf175cb10ce25dc8b6afce6ebd0dd
---
 README.rst                        |  1 +
 bashate/bashate.py                | 41 +++++++++++++++++++++++++++++++
 bashate/messages.py               | 12 +++++++--
 bashate/tests/samples/E044_bad.sh | 41 +++++++++++++++++++++++++++++++
 bashate/tests/test_bashate.py     | 11 +++++++++
 5 files changed, 104 insertions(+), 2 deletions(-)
 create mode 100644 bashate/tests/samples/E044_bad.sh

diff --git a/README.rst b/README.rst
index 8253732..631a2d3 100644
--- a/README.rst
+++ b/README.rst
@@ -51,6 +51,7 @@ not be used
 - E041: Usage of $[ for arithmetic is deprecated for $((
 - E042: local declaration hides errors
 - E043: arithmetic compound has inconsistent return semantics
+- E044: Use [[ for =~,<,> comparisions
 
 See also
 ~~~~~~~~
diff --git a/bashate/bashate.py b/bashate/bashate.py
index 4512238..57a1caa 100755
--- a/bashate/bashate.py
+++ b/bashate/bashate.py
@@ -18,6 +18,7 @@ import argparse
 import fileinput
 import os
 import re
+import shlex
 import subprocess
 import sys
 
@@ -153,6 +154,45 @@ def check_hashbang(line, filename, report):
             report.print_error(MESSAGES['E005'].msg, line)
 
 
+def check_conditional_expression(line, report):
+    # We're really starting to push the limits of what we can do without
+    # a complete bash syntax parser here.  For example
+    # > [[ $foo =~ " [ " ]] && [[ $bar =~ " ] " ]]
+    # would be valid but mess up a simple regex matcher for "[.*]".
+    # Let alone dealing with multiple-line-spanning etc...
+    #
+    # So we'll KISS and just look for simple, one line,
+    # > if [ $foo =~ "bar" ]; then
+    # type statements, which are the vast majority of typo errors.
+    #
+    # shlex is pretty helpful in getting us something we can walk to
+    # find this pattern.  It does however have issues with
+    # unterminated quotes on multi-line strings (e.g.)
+    #
+    # foo="bar   <-- we only see this bit in "line"
+    #  baz"
+    #
+    # So we're just going to ignore parser failures here and move on.
+    # Possibly in the future we could pull such multi-line strings
+    # into "logical_line" below, and pass that here and have shlex
+    # break that up.
+    try:
+        toks = shlex.shlex(line)
+        toks.wordchars = "[]=~"
+        toks = list(toks)
+    except ValueError:
+        return
+
+    in_single_bracket = False
+    for tok in toks:
+        if tok == '[':
+            in_single_bracket = True
+        elif tok in ('=~', '<', '>') and in_single_bracket:
+            report.print_error(MESSAGES['E044'].msg, line)
+        elif tok == ']':
+            in_single_bracket = False
+
+
 def check_syntax(filename, report):
     # run the file through "bash -n" to catch basic syntax errors and
     # other warnings
@@ -364,6 +404,7 @@ class BashateRun(object):
                     check_arithmetic(line, report)
                     check_local_subshell(line, report)
                     check_bare_arithmetic(line, report)
+                    check_conditional_expression(line, report)
 
         # finished processing the file
 
diff --git a/bashate/messages.py b/bashate/messages.py
index 021b92e..994b761 100644
--- a/bashate/messages.py
+++ b/bashate/messages.py
@@ -185,8 +185,16 @@ _messages = {
         """,
         'default': 'W',
     },
-
-
+    'E044': {
+        'msg': 'Use [[ for non-POSIX comparisions',
+        'long_msg':
+        """
+        [ is the POSIX test operator, while [[ is the bash keyword
+        comparision operator.  Comparisons such as =~, < and > require
+        the use of [[.
+        """,
+        'default': 'E',
+    },
 }
 
 MESSAGES = {}
diff --git a/bashate/tests/samples/E044_bad.sh b/bashate/tests/samples/E044_bad.sh
new file mode 100644
index 0000000..4d39cbb
--- /dev/null
+++ b/bashate/tests/samples/E044_bad.sh
@@ -0,0 +1,41 @@
+# =~
+
+if [ "test" =~ "]" ]; then
+    echo "Does not work!"
+fi
+
+[ "test" =~ "string" ] && echo "Does not work!"
+
+if [[ $foo == bar || "test" =~ "[" ]]; then
+    echo "Does work!"
+fi
+
+[[ "test" =~ "string" ]] && echo "Does work"
+
+# <
+
+if [ 1 < '2' ]; then
+    echo "Does not work!"
+fi
+
+[ 1 < 2 ] && echo "Does not work!"
+
+if [[ 1 < 2 ]]; then
+    echo "Does work!"
+fi
+
+[[ 1 < 2 ]] && echo "Does work"
+
+# >
+
+if [ 1 > 2 ]; then
+    echo "Does not work!"
+fi
+
+[ 1 > 2 ] && echo "Does not work!"
+
+if [[ 1 > 2 ]]; then
+    echo "Does work!"
+fi
+
+[[ 1 > 2 ]] && echo "Does work"
diff --git a/bashate/tests/test_bashate.py b/bashate/tests/test_bashate.py
index 103db9a..0a579a4 100644
--- a/bashate/tests/test_bashate.py
+++ b/bashate/tests/test_bashate.py
@@ -270,6 +270,17 @@ class TestBashateSamples(base.TestCase):
 
         self.assert_error_found('E040', 7)
 
+    def test_sample_E044(self):
+        test_files = ['bashate/tests/samples/E044_bad.sh']
+        self.run.check_files(test_files, False)
+
+        self.assert_error_found('E044', 3)
+        self.assert_error_found('E044', 7)
+        self.assert_error_found('E044', 17)
+        self.assert_error_found('E044', 21)
+        self.assert_error_found('E044', 31)
+        self.assert_error_found('E044', 35)
+
     def test_sample_warning(self):
         # reuse a couple of the above files to make sure we turn
         # errors down to warnings if requested