tools: fixing fix format check for exceptions (#32022)

Fixing a couple of exceptions while in the code.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

source/common/rds/rds_route_config_subscription.cc

@@ -85,13 +85,13 @@ absl::Status RdsRouteConfigSubscription::onConfigUpdate(
     stats_.config_reload_.inc();
     stats_.config_reload_time_ms_.set(DateUtil::nowToMilliseconds(factory_context_.timeSource()));
 
-    THROW_IF_NOT_OK(beforeProviderUpdate(noop_init_manager, resume_rds));
+    RETURN_IF_NOT_OK(beforeProviderUpdate(noop_init_manager, resume_rds));
 
     ENVOY_LOG(debug, "rds: loading new configuration: config_name={} hash={}", route_config_name_,
               config_update_info_->configHash());
 
     if (route_config_provider_ != nullptr) {
-      THROW_IF_NOT_OK(route_config_provider_->onConfigUpdate());
+      RETURN_IF_NOT_OK(route_config_provider_->onConfigUpdate());
     }
 
     afterProviderUpdate();

source/common/router/vhds.cc

@@ -100,7 +100,7 @@ absl::Status VhdsSubscription::onConfigUpdate(
               config_update_info_->protobufConfigurationCast().name(),
               config_update_info_->configHash());
     if (route_config_provider_ != nullptr) {
-      THROW_IF_NOT_OK(route_config_provider_->onConfigUpdate());
+      RETURN_IF_NOT_OK(route_config_provider_->onConfigUpdate());
     }
   }
 

source/common/secret/sds_api.cc

@@ -120,7 +120,7 @@ absl::Status SdsApi::onConfigUpdate(const std::vector<Config::DecodedResourceRef
           // Watch for directory instead of file. This allows users to do atomic renames
           // on directory level (e.g. Kubernetes secret update).
           const auto result_or_error = api_.fileSystem().splitPathFromFilename(filename);
-          THROW_IF_STATUS_NOT_OK(result_or_error, throw);
+          RETURN_IF_STATUS_NOT_OK(result_or_error);
           watcher_->addWatch(absl::StrCat(result_or_error.value().directory_, "/"),
                              Filesystem::Watcher::Events::MovedTo,
                              [this](uint32_t) { onWatchUpdate(); });

tools/code_format/check_format.py

@@ -393,11 +393,19 @@ def allow_listed_for_raw_try(self, file_path):
         return file_path in self.config.paths["raw_try"]["include"]
 
     def deny_listed_for_exceptions(self, file_path):
-        # Returns true when it is a non test header file or the file_path is in DENYLIST or
-        # it is under tools/testdata subdirectory.
-        return (file_path.endswith('.h') and not file_path.startswith("./test/") and not file_path in self.config.paths["exception"]["include"]) \
-            or (file_path.endswith('.cc') and file_path.startswith("./source/") and not file_path.startswith("./source/extensions/") and not file_path in self.config.paths["exception"]["include"]) \
-            or self.is_in_subdir(file_path, 'tools/testdata')
+        # Returns if this file is deny listed for exceptions.
+        # Header files are strongly discouraged from throwing exceptions, both for
+        # core and exception code. Source files are also discouraged but
+        # extensions source files are currently exempt from checks as many factory creation
+        # calls do not yet support StatusOr.
+        return ((
+            file_path.endswith('.h') and file_path.startswith("./source/")
+            and not file_path.startswith("./source/extensions/")
+            and not file_path in self.config.paths["exception"]["include"]) or (
+                file_path.endswith('.cc') and file_path.startswith("./source/")
+                and not file_path.startswith("./source/extensions/")
+                and not file_path in self.config.paths["exception"]["include"])
+                or self.is_in_subdir(file_path, 'tools/testdata'))
 
     def allow_listed_for_build_urls(self, file_path):
         return file_path in self.config.paths["build_urls"]["include"]
@@ -785,14 +793,19 @@ def check_source_line(self, line, file_path, report_error):
                 "Don't call memcpy() directly; use safeMemcpy, safeMemcpyUnsafeSrc, safeMemcpyUnsafeDst or MemBlockBuilder instead."
             )
 
-        if self.deny_listed_for_exceptions(file_path):
+        def has_non_comment_throw(line):
             # Skpping cases where 'throw' is a substring of a symbol like in "foothrowBar".
             if "throw" in line.split():
                 comment_match = self.config.re["comment"].search(line)
                 if comment_match is None or comment_match.start(0) > line.find("throw"):
-                    report_error(
-                        "Don't introduce throws into exception-free files, use error "
-                        + "statuses instead.")
+                    return True
+            return False
+
+        if self.deny_listed_for_exceptions(file_path):
+            if has_non_comment_throw(line) or "THROW" in line or "throwExceptionOrPanic" in line:
+                report_error(
+                    "Don't introduce throws into exception-free files, use error "
+                    "statuses instead.")
 
     def check_build_line(self, line, file_path, report_error):
         if "@bazel_tools" in line and not (self.is_starlark_file(file_path)

tools/code_format/config.yaml

@@ -161,6 +161,12 @@ paths:
     - source/server/hot_restarting_base.cc
     - source/server/hot_restart_impl.cc
     - source/server/ssl_context_manager.cc
+    - source/server/config_validation/cluster_manager.cc
+    - source/common/upstream/health_discovery_service.cc
+    - source/common/secret/sds_api.h
+    - source/common/secret/sds_api.cc
+    - source/common/router/router.cc
+    - source/common/config/config_provider_impl.h
 
   # Only one C++ file should instantiate grpc_init
   grpc_init: