format: add checks for type alias

support/hooks/pre-push

@@ -14,7 +14,7 @@ DUMMY_SHA=0000000000000000000000000000000000000000
 echo "Running pre-push check; to skip this step use 'push --no-verify'"
 
 while read LOCAL_REF LOCAL_SHA REMOTE_REF REMOTE_SHA
-do
+do    
   if [ "$LOCAL_SHA" = $DUMMY_SHA ]
   then
     # Branch deleted. Do nothing.
@@ -52,6 +52,20 @@ do
     # our `relpath` helper.
     SCRIPT_DIR="$(dirname "$(realpath "$0")")/../../tools"
 
+    git diff $RANGE -- '*.cc' '*.h' | grep -v -E '^\+\+\+' | grep -E '^\+' | sed -e 's/^\+//g' | while IFS='' read -r line
+    do
+      if [ -z "$line" ]
+      then
+        continue
+      fi
+
+      echo -ne "  Checking format for $line - "
+      "$SCRIPT_DIR"/code_format/check_line_format.py "$line"
+      if [[ $? -ne 0 ]]; then
+        exit 1
+      fi
+    done
+
     # TODO(hausdorff): We should have a more graceful failure story when the
     # user does not have all the tools set up correctly. This script assumes
     # `$CLANG_FORMAT` and `$BUILDIFY` are defined, or that the default values it

tools/code_format/check_format.py

@@ -168,6 +168,7 @@
   "extensions/filters/network/common",
   "extensions/filters/network/common/redis",
 }
+
 # yapf: enable
 
 

tools/code_format/check_format_test_helper.py

@@ -19,6 +19,7 @@
 tools = os.path.dirname(curr_dir)
 src = os.path.join(tools, 'testdata', 'check_format')
 check_format = sys.executable + " " + os.path.join(curr_dir, 'check_format.py')
+check_line_format = sys.executable + " " + os.path.join(curr_dir, 'check_line_format.py')
 errors = 0
 
 
@@ -31,6 +32,12 @@ def runCheckFormat(operation, filename):
   return (command, status, stdout + stderr)
 
 
+def runCheckLineFormat(line):
+  command = f"{check_line_format} \"{line}\""
+  status, stdout, stderr = runCommand(command)
+  return (command, status, stdout + stderr)
+
+
 def getInputFile(filename, extra_input_files=None):
   files_to_copy = [filename]
   if extra_input_files is not None:
@@ -99,6 +106,20 @@ def expectError(filename, status, stdout, expected_substring):
   return 1
 
 
+def expectLineError(line, status, stdout, expected_error):
+  if status == 0:
+    logging.error(f"{line}: Expected failure `{expected_error}`, but succeeded")
+    return 1
+
+  for line in stdout:
+    if expected_error in line:
+      return 0
+
+  logging.error(f"{line}: Could not find '{expected_error}' in:\n")
+  emitStdoutAsError(stdout)
+  return 1
+
+
 def fixFileExpectingFailure(filename, expected_substring):
   command, infile, outfile, status, stdout = fixFileHelper(filename)
   return expectError(filename, status, stdout, expected_substring)
@@ -110,6 +131,11 @@ def checkFileExpectingError(filename, expected_substring, extra_input_files=None
   return expectError(filename, status, stdout, expected_substring)
 
 
+def checkLineExpectingError(line, expected_error):
+  command, status, stdout = runCheckLineFormat(line)
+  return expectLineError(line, status, stdout, expected_error)
+
+
 def checkAndFixError(filename, expected_substring, extra_input_files=None):
   errors = checkFileExpectingError(filename,
                                    expected_substring,
@@ -138,6 +164,10 @@ def checkUnfixableError(filename, expected_substring):
   return errors
 
 
+def checkUnfixableLineError(line, expected_error):
+  return checkLineExpectingError(line, expected_error)
+
+
 def checkFileExpectingOK(filename):
   command, status, stdout = runCheckFormat("check", getInputFile(filename))
   if status != 0:
@@ -146,6 +176,15 @@ def checkFileExpectingOK(filename):
   return status + fixFileExpectingNoChange(filename)
 
 
+def checkLineExpectingOK(line):
+  command, status, stdout = runCheckLineFormat(line)
+  if status != 0:
+    logging.error(f"Expected '{line}' to have no errors; status={status}, output:\n")
+    emitStdoutAsError(stdout)
+
+  return status
+
+
 def runChecks():
   errors = 0
 
@@ -271,6 +310,25 @@ def runChecks():
   errors += checkFileExpectingOK("real_time_source_override.cc")
   errors += checkFileExpectingOK("time_system_wait_for.cc")
   errors += checkFileExpectingOK("clang_format_off.cc")
+
+  errors += checkUnfixableLineError(
+      "std::unique_ptr<Network::Connection> a() { return nullptr; }",
+      "Use type alias for 'Network::Connection' instead. See STYLE.md")
+  errors += checkUnfixableLineError(
+      ("absl::optional<std::reference_wrapper<ConnectionHandlerImpl::ActiveTcpListener>> a() {"
+       "    return nullptr;"
+       "}"), "Use type alias for 'ConnectionHandlerImpl::ActiveTcpListener' instead. See STYLE.md")
+
+  errors += checkLineExpectingOK(
+      ("using ConnectionPtr = std::unique_ptr<Connection>;"
+       "class A {"
+       "using ConnectionSharedPtr = std::shared_ptr<Connection>;"
+       "using ConnectionOptRef = absl::optional<std::reference_wrapper<Connection>>;"
+       "};"))
+  errors += checkLineExpectingOK(("void a(std::unique_ptr<int>, std::shared_ptr<std::string>,"
+                                  "absl::optional<std::reference_wrapper<char[]>>,"
+                                  "absl::optional<std::reference_wrapper<std::vector<int>>>) {}"))
+
   return errors
 
 

tools/code_format/check_line_format.py

@@ -0,0 +1,78 @@
+#!/usr/bin/env python3
+
+import argparse
+import re
+import sys
+
+NON_TYPE_ALIAS_ALLOWED_TYPES = {
+    "^(?![A-Z].*).*$",
+    "^(.*::){,1}(StrictMock<|NiceMock<).*$",
+    "^(.*::){,1}(Test|Mock|Fake).*$",
+    "^Protobuf.*::.*$",
+    "^[A-Z]$",
+    "^.*, .*",
+    r"^.*\[\]$",
+}
+
+USING_TYPE_ALIAS_REGEX = re.compile("using .* = .*;")
+SMART_PTR_REGEX = re.compile("std::(unique_ptr|shared_ptr)<(.*?)>(?!;)")
+OPTIONAL_REF_REGEX = re.compile("absl::optional<std::reference_wrapper<(.*?)>>(?!;)")
+NON_TYPE_ALIAS_ALLOWED_TYPE_REGEX = re.compile(f"({'|'.join(NON_TYPE_ALIAS_ALLOWED_TYPES)})")
+
+
+def whitelistedForNonTypeAlias(name):
+  return NON_TYPE_ALIAS_ALLOWED_TYPE_REGEX.match(name)
+
+
+def checkSourceLine(line, reportError):
+  if not USING_TYPE_ALIAS_REGEX.search(line):
+    smart_ptrs = SMART_PTR_REGEX.finditer(line)
+    for smart_ptr in smart_ptrs:
+      if not whitelistedForNonTypeAlias(smart_ptr.group(2)):
+        reportError(f"Use type alias for '{smart_ptr.group(2)}' instead. See STYLE.md")
+
+    optional_refs = OPTIONAL_REF_REGEX.finditer(line)
+    for optional_ref in optional_refs:
+      if not whitelistedForNonTypeAlias(optional_ref.group(1)):
+        reportError(f"Use type alias for '{optional_ref.group(1)}' instead. See STYLE.md")
+
+
+def printError(error):
+  print(f"ERROR: {error}")
+
+
+def checkFormat(line):
+  error_messages = []
+
+  def reportError(message):
+    error_messages.append(f"'{line}': {message}")
+
+  checkSourceLine(line, reportError)
+
+  return error_messages
+
+
+def checkErrors(errors):
+  if errors:
+    for e in errors:
+      printError(e)
+    return True
+
+  return False
+
+
+if __name__ == "__main__":
+  parser = argparse.ArgumentParser(description="Check line format.")
+  parser.add_argument("target_line", type=str, help="specify the line to check the format of")
+
+  args = parser.parse_args()
+
+  target_line = args.target_line
+
+  errors = checkFormat(target_line)
+
+  if checkErrors(errors):
+    printError("check format failed.")
+    sys.exit(1)
+
+  print("PASS")

tools/testdata/check_format/cpp_std.cc

@@ -4,7 +4,7 @@
 
 namespace Envoy {
 
-std::unique_ptr<int> to_be_fix = absl::make_unique<int>(0);
+auto to_be_fix = absl::make_unique<int>(0);
 
 // Awesome stuff goes here.
 

tools/testdata/check_format/cpp_std.cc.gold

@@ -4,7 +4,7 @@
 
 namespace Envoy {
 
-std::unique_ptr<int> to_be_fix = std::make_unique<int>(0);
+auto to_be_fix = std::make_unique<int>(0);
 
 // Awesome stuff goes here.