Auto merge of #12736 - aneeshusa:prohibit-backticks-in-shell-scripts, r=Wafflespeanut

Add more shell script lints

<!-- Please describe your changes on the following line: -->

The "$(some_command arg1 arg2)" form is preferred to the
`some_command arg1 arg2` form because it nests unambiguously.
Add a lint for this to tidy.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12736)
<!-- Reviewable:end -->

python/tidy/servo_tidy/tidy.py

@@ -318,19 +318,23 @@ def check_shell(file_name, lines):
     shebang = "#!/usr/bin/env bash"
     required_options = {"set -o errexit", "set -o nounset", "set -o pipefail"}
 
+    did_shebang_check = False
+
     if len(lines) == 0:
         yield (0, 'script is an empty file')
-    else:
-        if lines[0].rstrip() != shebang:
-            yield (1, 'script does not have shebang "{}"'.format(shebang))
+        return
 
-        for idx in range(1, len(lines)):
-            stripped = lines[idx].rstrip()
+    if lines[0].rstrip() != shebang:
+        yield (1, 'script does not have shebang "{}"'.format(shebang))
 
-            # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
-            if lines[idx].startswith("#") or stripped == "":
-                continue
-            elif stripped in required_options:
+    for idx in range(1, len(lines)):
+        stripped = lines[idx].rstrip()
+        # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
+        if lines[idx].startswith("#") or stripped == "":
+            continue
+
+        if not did_shebang_check:
+            if stripped in required_options:
                 required_options.remove(stripped)
             else:
                 # The first non-comment, non-whitespace, non-option line is the first "real" line of the script.
@@ -338,7 +342,20 @@ def check_shell(file_name, lines):
                 if len(required_options) != 0:
                     formatted = ['"{}"'.format(opt) for opt in required_options]
                     yield (idx + 1, "script is missing options {}".format(", ".join(formatted)))
-                    break
+                did_shebang_check = True
+
+        if "`" in stripped:
+            yield (idx + 1, "script should not use backticks for command substitution")
+
+        if " [ " in stripped or stripped.startswith("[ "):
+            yield (idx + 1, "script should use `[[` instead of `[` for conditional testing")
+
+        for dollar in re.finditer('\$', stripped):
+            next_idx = dollar.end()
+            if next_idx < len(stripped):
+                next_char = stripped[next_idx]
+                if not (next_char == '{' or next_char == '('):
+                    yield(idx + 1, "variable substitutions should use the full \"${VAR}\" form")
 
 
 def check_rust(file_name, lines):

python/tidy/servo_tidy_tests/shell_tidy.sh

@@ -4,4 +4,11 @@
 
 set -o nounset
 
+# Talking about some `concept in backticks` # shouldn't trigger
 echo "hello world"
+some_var=`echo "command substitution"`
+another_var="$some_var"
+if [ -z "${some_var}" ]; then
+  echo "should have used [["
+fi
+[ -z "${another_var}" ]

python/tidy/servo_tidy_tests/test_tidy.py

@@ -52,6 +52,10 @@ class CheckTidiness(unittest.TestCase):
         errors = tidy.collect_errors_for_files(iterFile('shell_tidy.sh'), [], [tidy.check_shell], print_text=False)
         self.assertEqual('script does not have shebang "#!/usr/bin/env bash"', errors.next()[2])
         self.assertEqual('script is missing options "set -o errexit", "set -o pipefail"', errors.next()[2])
+        self.assertEqual('script should not use backticks for command substitution', errors.next()[2])
+        self.assertEqual('variable substitutions should use the full \"${VAR}\" form', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_rust(self):