format: add checks for type aliases

tools/code_format/check_format.py

@@ -175,6 +175,21 @@
 }
 # yapf: enable
 
+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 .* = .*;|typedef .* .*;)")
+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)})")
+
 
 # Map a line transformation function across each line of a file,
 # writing the result lines as requested.
@@ -348,6 +363,10 @@ def allowlistedForGrpcInit(file_path):
   return file_path in GRPC_INIT_ALLOWLIST
 
 
+def whitelistedForNonTypeAlias(name):
+  return NON_TYPE_ALIAS_ALLOWED_TYPE_REGEX.match(name)
+
+
 def allowlistedForUnpackTo(file_path):
   return file_path.startswith("./test") or file_path in [
       "./source/common/protobuf/utility.cc", "./source/common/protobuf/utility.h"
@@ -510,6 +529,71 @@ def reportError(message):
   return error_messages
 
 
+def replaceWithTypeAlias(line):
+
+  def replaceSmartPtr(match):
+    kind = match.group(1)
+    name = match.group(2)
+    const = "const " in name
+    if const:
+      name = name.replace("const ", "")
+    if whitelistedForNonTypeAlias(name):
+      return match.group()
+
+    with_type_param = "<" in name
+
+    if kind == "unique_ptr" and not const and not with_type_param:
+      return f"{name}Ptr"
+    elif kind == "unique_ptr" and const and not with_type_param:
+      return f"{name}ConstPtr"
+    elif kind == "unique_ptr" and not const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}Ptr<{splitted[1]}"
+    elif kind == "unique_ptr" and const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}ConstPtr<{splitted[1]}"
+    elif kind == "shared_ptr" and not const and not with_type_param:
+      return f"{name}SharedPtr"
+    elif kind == "shared_ptr" and const and not with_type_param:
+      return f"{name}ConstSharedPtr"
+    elif kind == "shared_ptr" and not const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}SharedPtr<{splitted[1]}"
+    elif kind == "shared_ptr" and const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}ConstSharedPtr<{splitted[1]}"
+    else:
+      return match.group()
+
+  def replaceOptionalRef(match):
+    name = match.group(1)
+    const = "const " in name
+    if const:
+      name = name.replace("const ", "")
+    if whitelistedForNonTypeAlias(name):
+      return match.group()
+
+    with_type_param = "<" in name
+
+    if not const and not with_type_param:
+      return f"{name}OptRef"
+    elif const and not with_type_param:
+      return f"{name}OptConstRef"
+    elif not const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}OptRef<{splitted[1]}"
+    elif const and with_type_param:
+      splitted = name.split("<")
+      return f"{splitted[0]}OptConstRef<{splitted[1]}"
+    else:
+      return match.group()
+
+  line = SMART_PTR_REGEX.sub(replaceSmartPtr, line)
+  line = OPTIONAL_REF_REGEX.sub(replaceOptionalRef, line)
+
+  return line
+
+
 DOT_MULTI_SPACE_REGEX = re.compile("\\. +")
 
 
@@ -529,6 +613,9 @@ def fixSourceLine(line, line_number):
   for invalid_construct, valid_construct in LIBCXX_REPLACEMENTS.items():
     line = line.replace(invalid_construct, valid_construct)
 
+  if aggressive and not USING_TYPE_ALIAS_REGEX.search(line):
+    line = replaceWithTypeAlias(line)
+
   return line
 
 
@@ -715,6 +802,17 @@ def checkSourceLine(line, file_path, reportError):
         reportError("Don't call grpc_init() or grpc_shutdown() directly, instantiate " +
                     "Grpc::GoogleGrpcContext. See #8282")
 
+  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 checkBuildLine(line, file_path, reportError):
   if "@bazel_tools" in line and not (isSkylarkFile(file_path) or file_path.startswith("./bazel/") or
@@ -989,6 +1087,9 @@ def checkErrorMessages(error_messages):
                       type=str,
                       default=",".join(common.includeDirOrder()),
                       help="specify the header block include directory order.")
+  parser.add_argument("--aggressive",
+                      action='store_true',
+                      help="specify if it fixes formats making risky changes.")
   args = parser.parse_args()
 
   operation_type = args.operation_type
@@ -1006,6 +1107,7 @@ def checkErrorMessages(error_messages):
       "./tools/clang_tools",
   ]
   include_dir_order = args.include_dir_order
+  aggressive = args.aggressive
   if args.add_excluded_prefixes:
     EXCLUDED_PREFIXES += tuple(args.add_excluded_prefixes)
 

tools/code_format/check_format_test_helper.py

@@ -25,8 +25,11 @@
 # Runs the 'check_format' operation, on the specified file, printing
 # the comamnd run and the status code as well as the stdout, and returning
 # all of that to the caller.
-def runCheckFormat(operation, filename):
-  command = check_format + " " + operation + " " + filename
+def runCheckFormat(operation, filename, options=None):
+  command = f"{check_format} {operation} {filename} "
+  if options is not None:
+    command += f"{' '.join(options)} "
+
   status, stdout, stderr = runCommand(command)
   return (command, status, stdout + stderr)
 
@@ -47,19 +50,20 @@ def getInputFile(filename, extra_input_files=None):
 # Attempts to fix file, returning a 4-tuple: the command, input file name,
 # output filename, captured stdout as an array of lines, and the error status
 # code.
-def fixFileHelper(filename, extra_input_files=None):
+def fixFileHelper(filename, extra_input_files=None, options=None):
   command, status, stdout = runCheckFormat(
-      "fix", getInputFile(filename, extra_input_files=extra_input_files))
+      "fix", getInputFile(filename, extra_input_files=extra_input_files), options)
   infile = os.path.join(src, filename)
   return command, infile, filename, status, stdout
 
 
 # Attempts to fix a file, returning the status code and the generated output.
 # If the fix was successful, the diff is returned as a string-array. If the file
 # was not fixable, the error-messages are returned as a string-array.
-def fixFileExpectingSuccess(file, extra_input_files=None):
+def fixFileExpectingSuccess(file, extra_input_files=None, options=None):
   command, infile, outfile, status, stdout = fixFileHelper(file,
-                                                           extra_input_files=extra_input_files)
+                                                           extra_input_files=extra_input_files,
+                                                           options=options)
   if status != 0:
     print("FAILED: " + infile)
     emitStdoutAsError(stdout)
@@ -110,11 +114,11 @@ def checkFileExpectingError(filename, expected_substring, extra_input_files=None
   return expectError(filename, status, stdout, expected_substring)
 
 
-def checkAndFixError(filename, expected_substring, extra_input_files=None):
+def checkAndFixError(filename, expected_substring, extra_input_files=None, options=None):
   errors = checkFileExpectingError(filename,
                                    expected_substring,
                                    extra_input_files=extra_input_files)
-  errors += fixFileExpectingSuccess(filename, extra_input_files=extra_input_files)
+  errors += fixFileExpectingSuccess(filename, extra_input_files=extra_input_files, options=options)
   return errors
 
 
@@ -216,6 +220,11 @@ def runChecks():
       "grpc_shutdown.cc",
       "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " +
       "See #8282")
+  errors += checkUnfixableError("non_type_alias_smart_ptr.cc",
+                                "Use type alias for 'Network::Connection' instead. See STYLE.md")
+  errors += checkUnfixableError(
+      "non_type_alias_optional_ref.cc",
+      "Use type alias for 'ConnectionHandlerImpl::ActiveTcpListener' instead. See STYLE.md")
   errors += checkUnfixableError("clang_format_double_off.cc", "clang-format nested off")
   errors += checkUnfixableError("clang_format_trailing_off.cc", "clang-format remains off")
   errors += checkUnfixableError("clang_format_double_on.cc", "clang-format nested on")
@@ -269,10 +278,18 @@ def runChecks():
   errors += checkAndFixError(
       "cpp_std.cc",
       "term absl::make_unique< should be replaced with standard library term std::make_unique<")
+  errors += checkAndFixError("non_type_alias_types.cc",
+                             "Use type alias for",
+                             options=[
+                                 "--aggressive",
+                             ])
 
   errors += checkFileExpectingOK("real_time_source_override.cc")
   errors += checkFileExpectingOK("time_system_wait_for.cc")
   errors += checkFileExpectingOK("clang_format_off.cc")
+  errors += checkFileExpectingOK("using_type_alias.cc")
+  errors += checkFileExpectingOK("non_type_alias_allowed_type.cc")
+
   return errors
 
 

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.
 

tools/testdata/check_format/non_type_alias_allowed_type.cc

@@ -0,0 +1,11 @@
+#include <memory>
+#include <string>
+#include <vector>
+
+namespace Envoy {
+
+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>>>) {}
+
+} // namespace Envoy

tools/testdata/check_format/non_type_alias_optional_ref.cc

@@ -0,0 +1,11 @@
+#include <memory>
+
+#include "server/connection_handler_impl.h"
+
+namespace Envoy {
+
+absl::optional<std::reference_wrapper<ConnectionHandlerImpl::ActiveTcpListener>> a() {
+  return nullptr;
+}
+
+} // namespace Envoy

tools/testdata/check_format/non_type_alias_smart_ptr.cc

@@ -0,0 +1,9 @@
+#include <memory>
+
+#include "envoy/network/connection.h"
+
+namespace Envoy {
+
+std::unique_ptr<Network::Connection> a() { return nullptr; }
+
+} // namespace Envoy

tools/testdata/check_format/non_type_alias_types.cc

@@ -0,0 +1,10 @@
+#include <memory>
+
+namespace Envoy {
+void a(std::unique_ptr<AAA>, std::unique_ptr<const AAA>, std::shared_ptr<BBB>,
+       std::shared_ptr<const BBB>, absl::optional<std::reference_wrapper<CCC>>,
+       absl::optional<std::reference_wrapper<const CCC>>, std::unique_ptr<DDD<EEE>>,
+       std::unique_ptr<const DDD<EEE>>, std::shared_ptr<FFF<GGG>>, std::shared_ptr<const FFF<GGG>>,
+       absl::optional<std::reference_wrapper<HHH<III>>>,
+       absl::optional<std::reference_wrapper<const HHH<III>>>) {}
+} // namespace Envoy

tools/testdata/check_format/non_type_alias_types.cc.gold

@@ -0,0 +1,7 @@
+#include <memory>
+
+namespace Envoy {
+void a(AAAPtr, AAAConstPtr, BBBSharedPtr, BBBConstSharedPtr, CCCOptRef,
+       CCCOptConstRef, DDDPtr<EEE>, DDDConstPtr<EEE>, FFFSharedPtr<GGG>,
+       FFFConstSharedPtr<GGG>, HHHOptRef<III>, HHHOptConstRef<III>) {}
+} // namespace Envoy

tools/testdata/check_format/using_type_alias.cc

@@ -0,0 +1,17 @@
+#include <memory>
+
+namespace Envoy {
+namespace Network {
+class Connection;
+
+using ConnectionPtr = std::unique_ptr<Connection>;
+typedef std::unique_ptr<Connection> ConnectionPtr;
+
+template <class C> using EdfSchedulerPtr = std::unique_ptr<EdfScheduler<C>>;
+
+class A {
+  using ConnectionSharedPtr = std::shared_ptr<Connection>;
+  using ConnectionOptRef = absl::optional<std::reference_wrapper<Connection>>;
+};
+} // namespace Network
+} // namespace Envoy