diff --git a/tools/code_format/check_format.py b/tools/code_format/check_format.py index 351414da436b0..4d8df4bf9a02a 100755 --- a/tools/code_format/check_format.py +++ b/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>(?!;)") +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) diff --git a/tools/code_format/check_format_test_helper.py b/tools/code_format/check_format_test_helper.py index acf2cd9f87001..4cd12ccf18da8 100755 --- a/tools/code_format/check_format_test_helper.py +++ b/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,9 +50,9 @@ 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 @@ -57,9 +60,10 @@ def fixFileHelper(filename, extra_input_files=None): # 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 diff --git a/tools/testdata/check_format/cpp_std.cc b/tools/testdata/check_format/cpp_std.cc index 8342a139f26a3..ee4b0510eb561 100644 --- a/tools/testdata/check_format/cpp_std.cc +++ b/tools/testdata/check_format/cpp_std.cc @@ -4,7 +4,7 @@ namespace Envoy { -std::unique_ptr to_be_fix = absl::make_unique(0); +auto to_be_fix = absl::make_unique(0); // Awesome stuff goes here. diff --git a/tools/testdata/check_format/cpp_std.cc.gold b/tools/testdata/check_format/cpp_std.cc.gold index 8414a46360327..7f9457b3a3fc2 100644 --- a/tools/testdata/check_format/cpp_std.cc.gold +++ b/tools/testdata/check_format/cpp_std.cc.gold @@ -4,7 +4,7 @@ namespace Envoy { -std::unique_ptr to_be_fix = std::make_unique(0); +auto to_be_fix = std::make_unique(0); // Awesome stuff goes here. diff --git a/tools/testdata/check_format/non_type_alias_allowed_type.cc b/tools/testdata/check_format/non_type_alias_allowed_type.cc new file mode 100644 index 0000000000000..aa70841829276 --- /dev/null +++ b/tools/testdata/check_format/non_type_alias_allowed_type.cc @@ -0,0 +1,11 @@ +#include +#include +#include + +namespace Envoy { + +void a(std::unique_ptr, std::shared_ptr, + absl::optional>, + absl::optional>>) {} + +} // namespace Envoy diff --git a/tools/testdata/check_format/non_type_alias_optional_ref.cc b/tools/testdata/check_format/non_type_alias_optional_ref.cc new file mode 100644 index 0000000000000..df57aaa64adb8 --- /dev/null +++ b/tools/testdata/check_format/non_type_alias_optional_ref.cc @@ -0,0 +1,11 @@ +#include + +#include "server/connection_handler_impl.h" + +namespace Envoy { + +absl::optional> a() { + return nullptr; +} + +} // namespace Envoy diff --git a/tools/testdata/check_format/non_type_alias_smart_ptr.cc b/tools/testdata/check_format/non_type_alias_smart_ptr.cc new file mode 100644 index 0000000000000..6bf4739a8a893 --- /dev/null +++ b/tools/testdata/check_format/non_type_alias_smart_ptr.cc @@ -0,0 +1,9 @@ +#include + +#include "envoy/network/connection.h" + +namespace Envoy { + +std::unique_ptr a() { return nullptr; } + +} // namespace Envoy diff --git a/tools/testdata/check_format/non_type_alias_types.cc b/tools/testdata/check_format/non_type_alias_types.cc new file mode 100644 index 0000000000000..6802be663b2fc --- /dev/null +++ b/tools/testdata/check_format/non_type_alias_types.cc @@ -0,0 +1,10 @@ +#include + +namespace Envoy { +void a(std::unique_ptr, std::unique_ptr, std::shared_ptr, + std::shared_ptr, absl::optional>, + absl::optional>, std::unique_ptr>, + std::unique_ptr>, std::shared_ptr>, std::shared_ptr>, + absl::optional>>, + absl::optional>>) {} +} // namespace Envoy \ No newline at end of file diff --git a/tools/testdata/check_format/non_type_alias_types.cc.gold b/tools/testdata/check_format/non_type_alias_types.cc.gold new file mode 100644 index 0000000000000..a73b8290a39d8 --- /dev/null +++ b/tools/testdata/check_format/non_type_alias_types.cc.gold @@ -0,0 +1,7 @@ +#include + +namespace Envoy { +void a(AAAPtr, AAAConstPtr, BBBSharedPtr, BBBConstSharedPtr, CCCOptRef, + CCCOptConstRef, DDDPtr, DDDConstPtr, FFFSharedPtr, + FFFConstSharedPtr, HHHOptRef, HHHOptConstRef) {} +} // namespace Envoy \ No newline at end of file diff --git a/tools/testdata/check_format/using_type_alias.cc b/tools/testdata/check_format/using_type_alias.cc new file mode 100644 index 0000000000000..50b4a3ab4a522 --- /dev/null +++ b/tools/testdata/check_format/using_type_alias.cc @@ -0,0 +1,17 @@ +#include + +namespace Envoy { +namespace Network { +class Connection; + +using ConnectionPtr = std::unique_ptr; +typedef std::unique_ptr ConnectionPtr; + +template using EdfSchedulerPtr = std::unique_ptr>; + +class A { + using ConnectionSharedPtr = std::shared_ptr; + using ConnectionOptRef = absl::optional>; +}; +} // namespace Network +} // namespace Envoy