Skip to content

Commit

Permalink
Add the feature swift.werror.<diagnostic-name> to selectively allow…
Browse files Browse the repository at this point in the history
… certain warnings from the Swift compiler to be treated as errors.

The feature can be specified multiple times, with different diagnostic names.

The prefix `swift.werror...` is meant to evoke the `-Werror` Clang flag; `swift.warning_as_error...` and other variations felt too verbose.

This works by using the `-debug-diagnostic-names` flag to print the symbolic name of the diagnostic at the end of the message, in square brackets. Then the worker process intercepts stderr and looks for diagnostics with matching names, and modifies the message and exit code if there's a match.

PiperOrigin-RevId: 632470858
  • Loading branch information
allevato authored and swiple-rules-gardener committed May 10, 2024
1 parent 542ecfe commit 66a0137
Show file tree
Hide file tree
Showing 10 changed files with 168 additions and 4 deletions.
5 changes: 5 additions & 0 deletions swift/internal/compiling.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ load(
"get_cc_feature_configuration",
"is_feature_enabled",
"upcoming_and_experimental_features",
"warnings_as_errors_from_features",
)
load(":module_maps.bzl", "write_module_map")
load(":toolchain_utils.bzl", "SWIFT_TOOLCHAIN_TYPE")
Expand Down Expand Up @@ -477,6 +478,9 @@ def compile(
upcoming_features, experimental_features = upcoming_and_experimental_features(
feature_configuration = feature_configuration,
)
warnings_as_errors = warnings_as_errors_from_features(
feature_configuration = feature_configuration,
)
prerequisites = struct(
additional_inputs = additional_inputs,
always_include_headers = is_feature_enabled(
Expand All @@ -501,6 +505,7 @@ def compile(
transitive_swiftmodules = transitive_swiftmodules,
upcoming_features = upcoming_features,
user_compile_flags = copts,
warnings_as_errors = warnings_as_errors,
# Merge the compile outputs into the prerequisites.
**struct_fields(compile_outputs)
)
Expand Down
18 changes: 18 additions & 0 deletions swift/internal/features.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,24 @@ def upcoming_and_experimental_features(feature_configuration):

return (upcoming, experimental)

def warnings_as_errors_from_features(feature_configuration):
"""Extracts the diagnostic IDs to treat as errors in post-processing.
Args:
feature_configuration: The Swift feature configuration.
Returns:
The `list` of diagnostic IDs to treat as errors.
"""
prefix = "swift.werror."
warnings_as_errors = []

for feature in feature_configuration._enabled_features:
if feature.startswith(prefix):
warnings_as_errors.append(feature[len(prefix):])

return warnings_as_errors

def _check_allowlists(
*,
allowlists,
Expand Down
3 changes: 2 additions & 1 deletion swift/toolchains/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ bzl_library(
"//swift/toolchains/config:all_actions_config",
"//swift/toolchains/config:compile_config",
"//swift/toolchains/config:compile_module_interface_config",
"//swift/toolchains/config:modulewrap_config",
"//swift/toolchains/config:default_warnings_as_errors",
"//swift/toolchains/config:symbol_graph_config",
"//swift/toolchains/config:tool_config",
"@bazel_skylib//lib:paths",
Expand All @@ -47,6 +47,7 @@ bzl_library(
"//swift/toolchains/config:all_actions_config",
"//swift/toolchains/config:compile_config",
"//swift/toolchains/config:compile_module_interface_config",
"//swift/toolchains/config:default_warnings_as_errors",
"//swift/toolchains/config:symbol_graph_config",
"//swift/toolchains/config:tool_config",
"@bazel_skylib//lib:dicts",
Expand Down
5 changes: 5 additions & 0 deletions swift/toolchains/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ bzl_library(
],
)

bzl_library(
name = "default_warnings_as_errors",
srcs = ["default_warnings_as_errors.bzl"],
)

bzl_library(
name = "modulewrap_config",
srcs = ["modulewrap_config.bzl"],
Expand Down
16 changes: 16 additions & 0 deletions swift/toolchains/config/compile_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -856,6 +856,15 @@ def compile_action_configs(
SWIFT_FEATURE__SUPPORTS_V6,
],
),
ActionConfigInfo(
actions = [
SWIFT_ACTION_COMPILE,
],
configurators = [
add_arg("-debug-diagnostic-names"),
_warnings_as_errors_configurator,
],
),
]

# NOTE: The positions of these action configs in the list are important,
Expand Down Expand Up @@ -1637,6 +1646,13 @@ def _upcoming_and_experimental_features_configurator(prerequisites, args):
before_each = "-enable-experimental-feature",
)

def _warnings_as_errors_configurator(prerequisites, args):
"""Adds upcoming and experimental features to the command line."""
args.add_all(
prerequisites.warnings_as_errors,
format_each = "-Xwrapped-swift=-warning-as-error=%s",
)

def _additional_inputs_configurator(prerequisites, _args):
"""Propagates additional input files to the action.
Expand Down
29 changes: 29 additions & 0 deletions swift/toolchains/config/default_warnings_as_errors.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Copyright 2024 The Bazel Authors. All rights reserved.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

"""Manages the default list of warnings that are upgraded to warnings."""

visibility([
"@build_bazel_rules_swift//swift/toolchains/...",
])

_WARNINGS_AS_ERRORS_IDENTIFIERS = [
]

def default_warnings_as_errors_features():
"""Returns features to upgrade warnings to errors."""
return [
"swift.werror.{}".format(id)
for id in _WARNINGS_AS_ERRORS_IDENTIFIERS
]
5 changes: 5 additions & 0 deletions swift/toolchains/xcode_swift_toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,10 @@ load(
"@build_bazel_rules_swift//swift/toolchains/config:compile_module_interface_config.bzl",
"compile_module_interface_action_configs",
)
load(
"@build_bazel_rules_swift//swift/toolchains/config:default_warnings_as_errors.bzl",
"default_warnings_as_errors_features",
)
load(
"@build_bazel_rules_swift//swift/toolchains/config:symbol_graph_config.bzl",
"symbol_graph_action_configs",
Expand Down Expand Up @@ -644,6 +648,7 @@ def _xcode_swift_toolchain_impl(ctx):
ctx = ctx,
target_triple = target_triple,
))
requested_features.extend(default_warnings_as_errors_features())

if _is_xcode_at_least_version(xcode_config, "14.3"):
requested_features.append(SWIFT_FEATURE__SUPPORTS_UPCOMING_FEATURES)
Expand Down
1 change: 1 addition & 0 deletions tools/worker/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ cc_library(
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/strings",
"@com_googlesource_code_re2//:re2",
],
)

Expand Down
79 changes: 76 additions & 3 deletions tools/worker/swift_runner.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@
#include <fstream>
#include <functional>
#include <memory>
#include <optional>
#include <ostream>
#include <sstream>
#include <string>
#include <utility>
#include <vector>
Expand All @@ -38,6 +40,7 @@
#include "tools/common/path_utils.h"
#include "tools/common/process.h"
#include "tools/common/temp_file.h"
#include "re2/re2.h"

namespace bazel_rules_swift {

Expand Down Expand Up @@ -206,9 +209,13 @@ int SwiftRunner::Run(std::ostream &stderr_stream, bool stdout_to_stderr) {
}
}

// Spawn the originally requested job with its full argument list.
exit_code =
SpawnJob(tool_args_, args_, &job_env_, stderr_stream, stdout_to_stderr);
// Spawn the originally requested job with its full argument list. Capture
// stderr in a string stream, which we post-process to upgrade warnings to
// errors if requested.
std::ostringstream captured_stderr_stream;
exit_code = SpawnJob(tool_args_, args_, &job_env_, captured_stderr_stream,
stdout_to_stderr);
ProcessDiagnostics(captured_stderr_stream.str(), stderr_stream, exit_code);
if (exit_code != 0) {
return exit_code;
}
Expand Down Expand Up @@ -344,6 +351,11 @@ bool SwiftRunner::ProcessArgument(
return true;
}

if (absl::ConsumePrefix(&trimmed_arg, "-warning-as-error=")) {
warnings_as_errors_.insert(std::string(trimmed_arg));
return true;
}

// TODO(allevato): Report that an unknown wrapper arg was found and give
// the caller a way to exit gracefully.
return true;
Expand Down Expand Up @@ -477,4 +489,65 @@ int SwiftRunner::PerformLayeringCheck(std::ostream &stderr_stream,
return 0;
}

void SwiftRunner::ProcessDiagnostics(absl::string_view stderr_output,
std::ostream &stderr_stream,
int &exit_code) const {
if (stderr_output.empty()) {
// Nothing to do if there was no output.
return;
}

// Match the "warning: " prefix on a message, also capturing the preceding
// ANSI color sequence if present.
RE2 warning_pattern("((\\x1b\\[(?:\\d+)(?:;\\d+)*m)?warning:\\s)");
// When `-debug-diagnostic-names` is enabled, names are printed as identifiers
// in square brackets, either at the end of the string or followed by a
// semicolon (for wrapped diagnostics). Nothing guarantees this for the
// wrapped case -- it is just observed convention -- but it is sufficient
// while the compiler doesn't give us a more proper way to detect these.
RE2 diagnostic_name_pattern("\\[([_A-Za-z][_A-Za-z0-9]*)\\](;|$)");

for (absl::string_view line : absl::StrSplit(stderr_output, '\n')) {
std::unique_ptr<std::string> modified_line;

absl::string_view warning_label;
std::optional<absl::string_view> ansi_sequence;
if (RE2::PartialMatch(line, warning_pattern, &warning_label,
&ansi_sequence)) {
absl::string_view diagnostic_name;
absl::string_view line_cursor = line;

// Search the diagnostic line for all possible diagnostic names surrounded
// by square brackets.
while (RE2::FindAndConsume(&line_cursor, diagnostic_name_pattern,
&diagnostic_name)) {
if (warnings_as_errors_.contains(diagnostic_name)) {
modified_line = std::make_unique<std::string>(line);
std::ostringstream error_label;
if (ansi_sequence.has_value()) {
error_label << Color(Color::kBoldRed);
}
error_label << "error (upgraded from warning): ";
modified_line->replace(warning_label.data() - line.data(),
warning_label.length(), error_label.str());
if (exit_code == 0) {
exit_code = 1;
}

// In the event that there are multiple diagnostics on the same line
// (this is the case, for example, with "this is an error in Swift 6"
// messages), we can stop after we find the first match; the whole
// line will become an error.
break;
}
}
}
if (modified_line) {
stderr_stream << *modified_line << std::endl;
} else {
stderr_stream << line << std::endl;
}
}
}

} // namespace bazel_rules_swift
11 changes: 11 additions & 0 deletions tools/worker/swift_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <vector>

#include "absl/container/flat_hash_map.h"
#include "absl/container/flat_hash_set.h"
#include "absl/strings/string_view.h"
#include "tools/common/bazel_substitutions.h"
#include "tools/common/temp_file.h"
Expand Down Expand Up @@ -122,6 +123,12 @@ class SwiftRunner {
// declared in the build graph.
int PerformLayeringCheck(std::ostream &stderr_stream, bool stdout_to_stderr);

// Upgrade any of the requested warnings to errors and then print all of the
// diagnostics to the given stream. Updates the exit code if necessary (to
// turn a previously successful compilation into a failing one).
void ProcessDiagnostics(absl::string_view stderr_output,
std::ostream &stderr_stream, int &exit_code) const;

// A mapping of Bazel placeholder strings to the actual paths that should be
// substituted for them. Supports Xcode resolution on Apple OSes.
bazel_rules_swift::BazelPlaceholderSubstitutions
Expand Down Expand Up @@ -179,6 +186,10 @@ class SwiftRunner {

// The name of the module currently being compiled.
std::string module_name_;

// A set containing the diagnostic IDs that should be upgraded from warnings
// to errors by the worker.
absl::flat_hash_set<std::string> warnings_as_errors_;
};

} // namespace bazel_rules_swift
Expand Down

0 comments on commit 66a0137

Please sign in to comment.