diff --git a/swift/internal/compiling.bzl b/swift/internal/compiling.bzl index 0447e73bc..4245192d4 100644 --- a/swift/internal/compiling.bzl +++ b/swift/internal/compiling.bzl @@ -719,6 +719,18 @@ def compile( for dep_module_context in dep_swift_info.direct_modules: direct_module_names.append(dep_module_context.name) + # Excludes implicitly added deps + unused_check_module_name_groups = [] + for dep_swift_info in swift_infos + private_swift_infos: + direct_module_names_for_dep = [ + dep_module_context.name + for dep_module_context in dep_swift_info.direct_modules + ] + if direct_module_names_for_dep: + unused_check_module_name_groups.append( + ",".join(direct_module_names_for_dep), + ) + validate_system_modules = is_feature_enabled( feature_configuration = feature_configuration, feature_name = SWIFT_FEATURE_USE_C_MODULES, @@ -750,6 +762,7 @@ def compile( deps_modules_file = deps_modules_file, direct_module_names = direct_module_names, transitive_module_names = transitive_module_names, + unused_check_module_name_groups = unused_check_module_name_groups, ) else: deps_modules_file = None @@ -1932,7 +1945,8 @@ def _write_deps_modules_file( actions, deps_modules_file, direct_module_names, - transitive_module_names): + transitive_module_names, + unused_check_module_name_groups): """Writes a file containing the module names of direct dependencies. This file is used by the Swift worker process to perform layering checks. @@ -1949,11 +1963,15 @@ def _write_deps_modules_file( dependencies of the code being compiled. transitive_module_names: The list of names of modules in the target's transitive dependency graph. + unused_check_module_name_groups: A list of comma-separated module name + groups. Each group represents the direct modules provided by one + user-declared dependency and is used to detect unused dependencies. """ deps_mapping = actions.args() deps_mapping.set_param_file_format("multiline") deps_mapping.add_all(direct_module_names, format_each = "direct:%s") deps_mapping.add_all(transitive_module_names, format_each = "transitive:%s") + deps_mapping.add_all(unused_check_module_name_groups, format_each = "unused-check:%s") actions.write( content = deps_mapping, diff --git a/swift/internal/feature_names.bzl b/swift/internal/feature_names.bzl index a63c2bff3..5445ae1e5 100644 --- a/swift/internal/feature_names.bzl +++ b/swift/internal/feature_names.bzl @@ -128,7 +128,8 @@ SWIFT_FEATURE_MODULAR_INDEXING = "swift.modular_indexing" SWIFT_FEATURE_LAYERING_CHECK_FOR_C_DEPS = "swift.layering_check_for_c_deps" # If enabled, an error will be emitted when compiling Swift code if it imports -# any module that is not listed among the direct dependencies of the target. +# any module that is not listed among the direct dependencies of the target, or +# if it declares a direct dependency that is never imported. SWIFT_FEATURE_LAYERING_CHECK_SWIFT = "swift.layering_check_swift" # If enabled with `swift.layering_check_swift`, Swift layering checks will also diff --git a/test/fixtures/layering_check/BUILD b/test/fixtures/layering_check/BUILD index 771aee356..15f73116f 100644 --- a/test/fixtures/layering_check/BUILD +++ b/test/fixtures/layering_check/BUILD @@ -103,6 +103,14 @@ swift_library( deps = [":DirectDependency"], ) +swift_library( + name = "unused_dependency", + srcs = ["UnusedDependency.swift"], + module_name = "UnusedDependency", + tags = FIXTURE_TAGS, + deps = [":DirectDependency"], +) + transition_binary( name = "layering_violation_explicit_modules", tags = FIXTURE_TAGS, diff --git a/test/fixtures/layering_check/UnusedDependency.swift b/test/fixtures/layering_check/UnusedDependency.swift new file mode 100644 index 000000000..540c9e064 --- /dev/null +++ b/test/fixtures/layering_check/UnusedDependency.swift @@ -0,0 +1,3 @@ +public func unusedDependencyValue() -> String { + return "unused" +} diff --git a/test/fixtures/layering_check/layering_check_failure_test.sh b/test/fixtures/layering_check/layering_check_failure_test.sh index bad2eee59..55bdc6322 100755 --- a/test/fixtures/layering_check/layering_check_failure_test.sh +++ b/test/fixtures/layering_check/layering_check_failure_test.sh @@ -30,6 +30,29 @@ check_failure() { done } +check_unused_failure() { + local target="$1" + local expected_module="${2:-DirectDependency}" + + if "$bazel" build "$target" &>"$log"; then + cat "$log" + echo "Expected $target to fail to build" >&2 + exit 1 + fi + + for expected in \ + "Unused dependencies in" \ + "$target" \ + "$expected_module" \ + "Please remove the unused 'deps'"; do + if ! grep -Fq "$expected" "$log"; then + cat "$log" + echo "Expected failure log for $target to contain: $expected" >&2 + exit 1 + fi + done +} + check_failure \ "//test/fixtures/layering_check:foundation_consumer_violation_precompiled_modules" \ "//test/fixtures/layering_check:foundation_consumer" \ @@ -41,3 +64,4 @@ check_failure \ check_failure \ "//test/fixtures/layering_check:layering_violation_explicit_modules_default_precompiled_modules" \ "//test/fixtures/layering_check:layering_violation" +check_unused_failure "//test/fixtures/layering_check:unused_dependency" diff --git a/tools/worker/swift_runner.cc b/tools/worker/swift_runner.cc index 71ca5e023..56b5e5e2e 100644 --- a/tools/worker/swift_runner.cc +++ b/tools/worker/swift_runner.cc @@ -20,10 +20,12 @@ #include #include #include +#include #include "absl/container/btree_set.h" #include "absl/container/flat_hash_set.h" #include "absl/strings/match.h" +#include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" #include "absl/strings/substitute.h" @@ -305,6 +307,7 @@ void PrintVerboseInvocation(const std::vector& args, struct LayeringCheckModules { absl::btree_set direct_modules; + std::vector> direct_module_groups_to_check; absl::btree_set transitive_modules; }; @@ -321,6 +324,17 @@ LayeringCheckModules ReadLayeringCheckModules(absl::string_view path) { modules.transitive_modules.insert(std::string(value)); } else if (absl::ConsumePrefix(&value, "transitive:")) { modules.transitive_modules.insert(std::string(value)); + } else if (absl::ConsumePrefix(&value, "unused-check:")) { + absl::btree_set direct_module_group; + for (absl::string_view module_name : absl::StrSplit(value, ',')) { + if (!module_name.empty()) { + direct_module_group.insert(std::string(module_name)); + } + } + if (!direct_module_group.empty()) { + modules.direct_module_groups_to_check.push_back( + std::move(direct_module_group)); + } } else { // Compatibility with the original file format, which contained one // direct module name per line. @@ -407,8 +421,10 @@ bool SkipLayeringCheckIncompatibleArgs(std::vector::iterator& it) { // modules. static const absl::flat_hash_set kModulesIgnorableForLayeringCheck = { - "Builtin", "Swift", "SwiftOnoneSupport", - "_Backtracing", "_Concurrency", "_StringProcessing", + "Builtin", "Swift", + "SwiftOnoneSupport", "SwiftShims", + "_Backtracing", "_Concurrency", + "_StringProcessing", "_SwiftConcurrencyShims", }; // Returns true if the module can be ignored for the purposes of layering check @@ -894,26 +910,56 @@ int SwiftRunner::PerformLayeringCheck(std::ostream& stderr_stream, // Use a `btree_set` so that the output is automatically sorted // lexicographically. absl::btree_set missing_deps; + absl::btree_set imported_modules; std::ifstream imported_modules_stream(imported_modules_path); std::string module_name; + auto diagnostic_module_name = [this](const std::string& module_name) { + // Swift's `-emit-imported-modules` output reports resolved aliased module + // names. Map them back to the names users write in source before reporting + // diagnostics. + if (auto alias_and_source_name = alias_to_source_mapping_.find(module_name); + alias_and_source_name != alias_to_source_mapping_.end()) { + return alias_and_source_name->second; + } + return module_name; + }; + while (std::getline(imported_modules_stream, module_name)) { + imported_modules.insert(module_name); + // A module can import itself when the Swift module has an underlying Clang // module, such as with `@_exported import X` in a Swift overlay for X. if (module_name != module_name_ && !IsModuleIgnorableForLayeringCheck(module_name) && layering_check_modules.transitive_modules.contains(module_name) && !layering_check_modules.direct_modules.contains(module_name)) { - // Swift's `-emit-imported-modules` output reports resolved aliased module - // names. Map them back to the names users write in source before - // reporting missing deps. - if (auto alias_and_source_name = - alias_to_source_mapping_.find(module_name); - alias_and_source_name != alias_to_source_mapping_.end()) { - missing_deps.insert(alias_and_source_name->second); - } else { - missing_deps.insert(module_name); + missing_deps.insert(diagnostic_module_name(module_name)); + } + } + + absl::btree_set unused_deps; + for (const auto& direct_module_group : + layering_check_modules.direct_module_groups_to_check) { + bool has_checkable_module = false; + bool has_imported_module = false; + absl::btree_set unused_modules_in_group; + for (const std::string& module_name : direct_module_group) { + if (module_name == module_name_ || + IsModuleIgnorableForLayeringCheck(module_name)) { + continue; + } + + has_checkable_module = true; + unused_modules_in_group.insert(diagnostic_module_name(module_name)); + if (imported_modules.contains(module_name)) { + has_imported_module = true; } } + + if (has_checkable_module && !has_imported_module) { + unused_deps.insert(unused_modules_in_group.begin(), + unused_modules_in_group.end()); + } } if (!missing_deps.empty()) { @@ -934,6 +980,29 @@ int SwiftRunner::PerformLayeringCheck(std::ostream& stderr_stream, WithColor(stderr_stream, Color::kBold) << "Please add the correct 'deps' to " << target_label_ << " to import those modules." << std::endl; + } + + if (!unused_deps.empty()) { + stderr_stream << std::endl; + WithColor(stderr_stream, Color::kBoldRed) << "error: "; + WithColor(stderr_stream, Color::kBold) << "Unused dependencies in "; + WithColor(stderr_stream, Color::kBoldGreen) << target_label_ << std::endl; + stderr_stream + << "The following modules are provided by direct dependencies, but " + << "they were not imported:" << std::endl + << std::endl; + + for (const std::string& module_name : unused_deps) { + stderr_stream << " " << module_name << std::endl; + } + stderr_stream << std::endl; + + WithColor(stderr_stream, Color::kBold) + << "Please remove the unused 'deps' from " << target_label_ << "." + << std::endl; + } + + if (!missing_deps.empty() || !unused_deps.empty()) { return 1; }