From de0f60472b5943275960b9ef4037374587064dab Mon Sep 17 00:00:00 2001 From: Tony Allevato Date: Mon, 16 Aug 2021 10:58:32 -0700 Subject: [PATCH] Add a way for `swift_interop_hint` to suppress the module for targets that generate them by default, like `objc_library`. PiperOrigin-RevId: 391087374 --- swift/BUILD | 8 +++ swift/internal/swift_clang_module_aspect.bzl | 13 +++++ swift/internal/swift_interop_hint.bzl | 42 +++++++++++++++ test/fixtures/BUILD | 1 + test/fixtures/common.bzl | 25 +++++++++ test/fixtures/interop_hints/BUILD | 43 +++++++++++++++- test/fixtures/interop_hints/Empty.swift | 1 + test/interop_hints_tests.bzl | 18 +++++++ test/rules/provider_test.bzl | 54 +++++++++++++++++--- 9 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 test/fixtures/interop_hints/Empty.swift diff --git a/swift/BUILD b/swift/BUILD index 0fc45f747..02757a30b 100644 --- a/swift/BUILD +++ b/swift/BUILD @@ -48,6 +48,14 @@ swift_interop_hint( visibility = ["//visibility:public"], ) +# An aspect hint that suppresses generation of a module for a non-Swift target +# that would otherwise generate one by default, like an `objc_library`. +swift_interop_hint( + name = "no_module", + suppressed = True, + visibility = ["//visibility:public"], +) + # User settable flag that specifies additional Swift copts on a per-swiftmodule basis. per_module_swiftcopt_flag( name = "per_module_swiftcopt", diff --git a/swift/internal/swift_clang_module_aspect.bzl b/swift/internal/swift_clang_module_aspect.bzl index 34ec60640..1b9b1bbc8 100644 --- a/swift/internal/swift_clang_module_aspect.bzl +++ b/swift/internal/swift_clang_module_aspect.bzl @@ -70,6 +70,10 @@ implementation to supply an additional set of fixed features that should always be enabled when the aspect processes that target; for example, a rule can request that `swift.emit_c_module` always be enabled for its targets even if it is not explicitly enabled in the toolchain or on the target directly. +""", + "suppressed": """\ +A `bool` indicating whether the module that the aspect would create for the +target should instead be suppressed. """, "swift_infos": """\ A list of `SwiftInfo` providers from dependencies of the target, which will be @@ -92,6 +96,7 @@ def create_swift_interop_info( module_map = None, module_name = None, requested_features = [], + suppressed = False, swift_infos = [], unsupported_features = []): """Returns a provider that lets a target expose C/Objective-C APIs to Swift. @@ -139,6 +144,8 @@ def create_swift_interop_info( `swift.emit_c_module` always be enabled for its targets even if it is not explicitly enabled in the toolchain or on the target directly. + suppressed: A `bool` indicating whether the module that the aspect would + create for the target should instead be suppressed. swift_infos: A list of `SwiftInfo` providers from dependencies, which will be merged with the new `SwiftInfo` created by the aspect. unsupported_features: A list of features (empty by default) that should @@ -163,6 +170,7 @@ def create_swift_interop_info( module_map = module_map, module_name = module_name, requested_features = requested_features, + suppressed = suppressed, swift_infos = swift_infos, unsupported_features = unsupported_features, ) @@ -599,6 +607,11 @@ def _swift_clang_module_aspect_impl(target, aspect_ctx): interop_info, swift_infos = _find_swift_interop_info(target, aspect_ctx) if interop_info: + # If the module should be suppressed, return immediately and propagate + # nothing (not even transitive dependencies). + if interop_info.suppressed: + return [] + module_map_file = interop_info.module_map module_name = ( interop_info.module_name or derive_module_name(target.label) diff --git a/swift/internal/swift_interop_hint.bzl b/swift/internal/swift_interop_hint.bzl index e9db0f51f..b325623df 100644 --- a/swift/internal/swift_interop_hint.bzl +++ b/swift/internal/swift_interop_hint.bzl @@ -22,6 +22,7 @@ def _swift_interop_hint_impl(ctx): return swift_common.create_swift_interop_info( module_map = ctx.file.module_map, module_name = ctx.attr.module_name, + suppressed = ctx.attr.suppressed, ) swift_interop_hint = rule( @@ -48,6 +49,14 @@ The name that will be used to import the hinted module into Swift. If left unspecified, the module name will be computed based on the hinted target's build label, by stripping the leading `//` and replacing `/`, `:`, and other non-identifier characters with underscores. +""", + mandatory = False, + ), + "suppressed": attr.bool( + default = False, + doc = """\ +If `True`, the hinted target should suppress any module that it would otherwise +generate. """, mandatory = False, ), @@ -136,6 +145,39 @@ swift_interop_hint( module_name = "CSomeLib", ) ``` + +#### Suppressing a module + +As mentioned above, `objc_library` and other Objective-C targets generate +modules by default, without an explicit hint, for convenience. In some +situations, this behavior may not be desirable. For example, an `objc_library` +might contain only Objective-C++ code in its headers that would not be possible +to import into Swift at all. + +When building with implicit modules, this is not typically an issue because the +module map would only be used if Swift code tried to import it (although it does +create useless actions and compiler inputs during the build). When building with +explicit modules, however, Bazel needs to know which targets represent modules +that it can compile and which do not. + +In these cases, there is no need to declare an instance of `swift_interop_hint`. +A canonical one that suppresses module generation has been provided in +`@build_bazel_rules_swift//swift:no_module`. Simply add it to the `aspect_hints` of +the target whose module you wish to suppress: + +```build +# //my/project/BUILD +objc_library( + name = "somelib", + srcs = ["somelib.mm"], + hdrs = ["somelib.h"], + aspect_hints = ["@build_bazel_rules_swift//swift:no_module"], +) +``` + +When this `objc_library` is a dependency of a Swift target, no module map or +explicit module will be generated for it, nor will any Swift information from +its transitive dependencies be propagated. """, implementation = _swift_interop_hint_impl, ) diff --git a/test/fixtures/BUILD b/test/fixtures/BUILD index 20f8c53cd..af47b67a5 100644 --- a/test/fixtures/BUILD +++ b/test/fixtures/BUILD @@ -4,4 +4,5 @@ bzl_library( name = "starlark_tests_bzls", srcs = glob(["*.bzl"]), visibility = ["//test:__pkg__"], + deps = ["//swift"], ) diff --git a/test/fixtures/common.bzl b/test/fixtures/common.bzl index 7fdf657fa..472ada8ea 100644 --- a/test/fixtures/common.bzl +++ b/test/fixtures/common.bzl @@ -14,9 +14,34 @@ """Common build definitions used by test fixtures.""" +load( + "@build_bazel_rules_swift//swift:swift.bzl", + "SwiftInfo", + "swift_clang_module_aspect", +) + # Common tags that prevent the test fixtures from actually being built (i.e., # their actions executed) when running `bazel test` to do analysis testing. FIXTURE_TAGS = [ "manual", "notap", ] + +def _forward_swift_info_from_swift_clang_module_aspect_impl(ctx): + if SwiftInfo in ctx.attr.target: + return [ctx.attr.target[SwiftInfo]] + return [] + +forward_swift_info_from_swift_clang_module_aspect = rule( + attrs = { + "target": attr.label( + aspects = [swift_clang_module_aspect], + mandatory = True, + ), + }, + doc = """\ +Applies `swift_clang_module_aspect` to the given target and forwards the +`SwiftInfo` provider that it attaches to the target, if any. +""", + implementation = _forward_swift_info_from_swift_clang_module_aspect_impl, +) diff --git a/test/fixtures/interop_hints/BUILD b/test/fixtures/interop_hints/BUILD index 2b33cb36f..227433ec6 100644 --- a/test/fixtures/interop_hints/BUILD +++ b/test/fixtures/interop_hints/BUILD @@ -3,7 +3,11 @@ load( "swift_interop_hint", "swift_library", ) -load("//test/fixtures:common.bzl", "FIXTURE_TAGS") +load( + "//test/fixtures:common.bzl", + "FIXTURE_TAGS", + "forward_swift_info_from_swift_clang_module_aspect", +) package( default_visibility = ["//test:__subpackages__"], @@ -80,3 +84,40 @@ swift_interop_hint( module_map = "module.modulemap", tags = FIXTURE_TAGS, ) + +forward_swift_info_from_swift_clang_module_aspect( + name = "objc_library_suppressed", + target = ":objc_library_suppressed_lib", +) + +objc_library( + name = "objc_library_suppressed_lib", + hdrs = ["header1.h"], + aspect_hints = [":suppress_hint"], + tags = FIXTURE_TAGS, +) + +forward_swift_info_from_swift_clang_module_aspect( + name = "objc_library_with_swift_dep_suppressed", + target = ":objc_library_with_swift_dep_suppressed_lib", +) + +objc_library( + name = "objc_library_with_swift_dep_suppressed_lib", + hdrs = ["header1.h"], + aspect_hints = [":suppress_hint"], + tags = FIXTURE_TAGS, + deps = [":empty_lib"], +) + +swift_library( + name = "empty_lib", + srcs = ["Empty.swift"], + tags = FIXTURE_TAGS, +) + +swift_interop_hint( + name = "suppress_hint", + suppressed = True, + tags = FIXTURE_TAGS, +) diff --git a/test/fixtures/interop_hints/Empty.swift b/test/fixtures/interop_hints/Empty.swift new file mode 100644 index 000000000..bd9ec079d --- /dev/null +++ b/test/fixtures/interop_hints/Empty.swift @@ -0,0 +1 @@ +// Intentionally empty. diff --git a/test/interop_hints_tests.bzl b/test/interop_hints_tests.bzl index 31d6d5633..2a40f06f1 100644 --- a/test/interop_hints_tests.bzl +++ b/test/interop_hints_tests.bzl @@ -63,6 +63,24 @@ def interop_hints_test_suite(): target_under_test = "@build_bazel_rules_swift//test/fixtures/interop_hints:invalid_swift", ) + # Verify that an `objc_library` hinted to suppress its module does not + # propagate a `SwiftInfo` provider at all. + provider_test( + name = "{}_objc_library_module_suppressed".format(name), + does_not_propagate_provider = "SwiftInfo", + tags = [name], + target_under_test = "@build_bazel_rules_swift//test/fixtures/interop_hints:objc_library_suppressed", + ) + + # Verify that an `objc_library` hinted to suppress its module does not + # propagate a `SwiftInfo` provider even if it has Swift dependencies. + provider_test( + name = "{}_objc_library_module_with_swift_dep_suppressed".format(name), + does_not_propagate_provider = "SwiftInfo", + tags = [name], + target_under_test = "@build_bazel_rules_swift//test/fixtures/interop_hints:objc_library_with_swift_dep_suppressed", + ) + native.test_suite( name = name, tags = [name], diff --git a/test/rules/provider_test.bzl b/test/rules/provider_test.bzl index a6e68cfa3..19bdede3d 100644 --- a/test/rules/provider_test.bzl +++ b/test/rules/provider_test.bzl @@ -160,11 +160,6 @@ def _lookup_provider_by_name(env, target, provider_name): if provider in target: return target[provider] - - unittest.fail( - env, - "Target '{}' did not provide '{}'.".format(target.label, provider_name), - ) return None def _field_access_description(target, provider, field): @@ -322,12 +317,41 @@ def _provider_test_impl(ctx): if types.is_list(target_under_test): target_under_test = target_under_test[0] + provider_name = ctx.attr.does_not_propagate_provider + if provider_name: + provider = _lookup_provider_by_name( + env, + target_under_test, + provider_name, + ) + if provider: + unittest.fail( + env, + "Expected {} to not propagate '{}', but it did: {}".format( + target_under_test.label, + provider_name, + provider, + ), + ) + return analysistest.end(env) + provider_name = ctx.attr.provider + field = ctx.attr.field + if not provider_name or not field: + fail("Either 'does_not_propagate_provider' must be specified, or " + + "both 'provider' and 'field' must be specified.") + provider = _lookup_provider_by_name(env, target_under_test, provider_name) if not provider: + unittest.fail( + env, + "Target '{}' did not provide '{}'.".format( + target_under_test.label, + provider_name, + ), + ) return analysistest.end(env) - field = ctx.attr.field actual = _evaluate_field(env, provider, field) if actual == _EVALUATE_FIELD_FAILED: return analysistest.end(env) @@ -363,6 +387,20 @@ def make_provider_test_rule(config_settings = {}): return analysistest.make( _provider_test_impl, attrs = { + "does_not_propagate_provider": attr.string( + mandatory = False, + doc = """\ +The name of a provider that is expected to not be propagated by the target under +test. + +Currently, only the following providers are recognized: + +* `CcInfo` +* `DefaultInfo` +* `SwiftInfo` +* `apple_common.Objc` +""", + ), "expected_files": attr.string_list( mandatory = False, doc = """\ @@ -384,7 +422,7 @@ configuration details, such as output directories for generated files. """, ), "field": attr.string( - mandatory = True, + mandatory = False, doc = """\ The field name or dotted field path of the provider that should be tested. @@ -398,7 +436,7 @@ the next component. """, ), "provider": attr.string( - mandatory = True, + mandatory = False, doc = """\ The name of the provider expected to be propagated by the target under test, and on which the field will be checked.