Only disable check_includes_strict optimization for exempted includers Instead of disabling the hard_deps propagation optimization across the entire build whenever `check_includes_strict_exceptions` is non-empty, only disable it for targets that match an exception rule as the includer. Index `includer` LabelPatterns in an `unordered_set<LabelPattern>` and implement `HasExceptionFrom()` by probing exact target matches, directory matches, and ancestor recursive directory matches. Bug: 556959580 Change-Id: I9f65ed5611355cec26e403e3b910bc65eccdde16 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/27860 Reviewed-by: Matt Stark <msta@google.com> Commit-Queue: Takuto Ikuta <tikuta@google.com>
diff --git a/docs/reference.md b/docs/reference.md index bea64cc..c9754c3 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -5781,8 +5781,8 @@ forwarded to dependents. * "deps" act as private implementation dependencies (dependencies required to compile this target's sources). This also allows dependent targets to - compile without waiting for this target's private "deps" (unless - check_includes_strict_exceptions is non-empty). + compile without waiting for this target's private "deps" (unless this + target is exempted as an includer via check_includes_strict_exceptions). When false (the default), the default loose include checking rules apply. ```
diff --git a/src/gn/check_includes_strict_exceptions.cc b/src/gn/check_includes_strict_exceptions.cc index a5eb6e6..834b624 100644 --- a/src/gn/check_includes_strict_exceptions.cc +++ b/src/gn/check_includes_strict_exceptions.cc
@@ -30,6 +30,37 @@ return HasGlobalException(from) || HasLocalException(from, to); } +bool CheckIncludesStrictExceptions::HasExceptionFrom(const Label& from) const { + if (includers_.empty()) + return false; + + // Instead of scanning all exception rules linearly, probe `includers_` for + // the only LabelPattern forms that can match `from`: + // 1. Exact target match (e.g. "//foo/bar:baz") + // 2. Non-recursive directory match (e.g. "//foo/bar:*") + // 3. Recursive directory match on any ancestor directory + // (e.g. "//*", "//foo/*", "//foo/bar/*") + if (includers_.contains(LabelPattern(LabelPattern::MATCH, from.dir(), + from.name(), Label())) || + includers_.contains( + LabelPattern(LabelPattern::DIRECTORY, from.dir(), "", Label()))) { + return true; + } + + // Walk each '/' boundary in `from.dir()` (which starts with "//" and ends + // with "/") to check ancestor directories from "//" down to `from.dir()`. + std::string_view dir = from.dir().value(); + for (size_t pos = dir.starts_with("//") ? 1 : 0; + (pos = dir.find('/', pos)) != std::string_view::npos; ++pos) { + if (includers_.contains(LabelPattern(LabelPattern::RECURSIVE_DIRECTORY, + SourceDir(dir.substr(0, pos + 1)), "", + Label()))) { + return true; + } + } + return false; +} + Result<CheckIncludesStrictExceptions> CheckIncludesStrictExceptions::FromValue( const Value& value, const SourceDir& current_dir,
diff --git a/src/gn/check_includes_strict_exceptions.h b/src/gn/check_includes_strict_exceptions.h index 2c616ed..bded3b3 100644 --- a/src/gn/check_includes_strict_exceptions.h +++ b/src/gn/check_includes_strict_exceptions.h
@@ -7,6 +7,7 @@ #include <string_view> #include <unordered_map> +#include <unordered_set> #include <vector> #include "gn/err.h" @@ -26,14 +27,22 @@ const SourceDir& current_dir, std::string_view source_root); - void AddGlobalException(const LabelPattern& from) { global_.push_back(from); } + void AddGlobalException(const LabelPattern& from) { + global_.push_back(from); + includers_.insert(from); + } void AddLocalException(const LabelPattern& from, const Label& to) { specific_targets_[to.GetWithNoToolchain()].push_back(from); + includers_.insert(from); } - // Checks whether `(from, to)` matches any local or global exception. + // Checks whether `(from, to)` matches any local or global exception. bool HasException(const Label& from, const Label& to) const; + // Checks whether `from` matches the includer of any local or global exception + // rule. + bool HasExceptionFrom(const Label& from) const; + // Checks whether `from` matches any global exception rule. bool HasGlobalException(const Label& from) const; @@ -41,11 +50,14 @@ bool HasLocalException(const Label& from, const Label& to) const; // Returns true if there are no exception rules. - bool empty() const { return global_.empty() && specific_targets_.empty(); } + bool empty() const { return includers_.empty(); } private: std::vector<LabelPattern> global_; std::unordered_map<Label, std::vector<LabelPattern>> specific_targets_; + // All `from` patterns across global and local rules, indexed for fast lookup + // in HasExceptionFrom(). + std::unordered_set<LabelPattern> includers_; }; #endif // TOOLS_GN_CHECK_INCLUDES_STRICT_EXCEPTIONS_H_
diff --git a/src/gn/check_includes_strict_exceptions_unittest.cc b/src/gn/check_includes_strict_exceptions_unittest.cc index 29c249b..ad7ef52 100644 --- a/src/gn/check_includes_strict_exceptions_unittest.cc +++ b/src/gn/check_includes_strict_exceptions_unittest.cc
@@ -90,3 +90,43 @@ exceptions.HasException(Label(SourceDir("//other_dir/"), "foo"), Label(SourceDir("//any_dir/"), "any_target"))); } + +TEST(CheckIncludesStrictExceptionsTest, HasExceptionFrom) { + CheckIncludesStrictExceptions exceptions; + EXPECT_FALSE( + exceptions.HasExceptionFrom(Label(SourceDir("//exact/"), "target"))); + + exceptions.AddLocalException( + LabelPattern(LabelPattern::MATCH, SourceDir("//exact/"), "target", + Label()), + Label(SourceDir("//included/"), "target")); + exceptions.AddLocalException( + LabelPattern(LabelPattern::DIRECTORY, SourceDir("//dir/"), "", Label()), + Label(SourceDir("//included/"), "target")); + exceptions.AddGlobalException(LabelPattern(LabelPattern::RECURSIVE_DIRECTORY, + SourceDir("//rec/"), "", Label())); + + // Exact match (with and without toolchain). + EXPECT_TRUE( + exceptions.HasExceptionFrom(Label(SourceDir("//exact/"), "target"))); + EXPECT_TRUE(exceptions.HasExceptionFrom( + Label(SourceDir("//exact/"), "target", SourceDir("//tc/"), "tc"))); + EXPECT_FALSE( + exceptions.HasExceptionFrom(Label(SourceDir("//exact/"), "other"))); + + // Directory match (non-recursive). + EXPECT_TRUE(exceptions.HasExceptionFrom(Label(SourceDir("//dir/"), "any"))); + EXPECT_FALSE( + exceptions.HasExceptionFrom(Label(SourceDir("//dir/sub/"), "any"))); + + // Recursive directory match. + EXPECT_TRUE(exceptions.HasExceptionFrom(Label(SourceDir("//rec/"), "any"))); + EXPECT_TRUE( + exceptions.HasExceptionFrom(Label(SourceDir("//rec/sub/deep/"), "any"))); + EXPECT_FALSE( + exceptions.HasExceptionFrom(Label(SourceDir("//rec_other/"), "any"))); + + // Being on the `included` side does not make it an exempted includer. + EXPECT_FALSE( + exceptions.HasExceptionFrom(Label(SourceDir("//included/"), "target"))); +}
diff --git a/src/gn/resolved_target_data.cc b/src/gn/resolved_target_data.cc index b20c526..6f3f522 100644 --- a/src/gn/resolved_target_data.cc +++ b/src/gn/resolved_target_data.cc
@@ -83,16 +83,16 @@ // `public_deps()`. The target's own compilation still gets hard deps from its // `private_deps()` in NinjaTargetWriter::WriteInputDepsStampOrPhonyAndGetDep. // - // However, a target marked as check_includes_strict doesn't truly have the - // properties of check_includes_strict until there are no exemptions with - // target as the includer. Since we don't have an efficient way to check this, - // and exemptions are designed to be temporary anyway, we just disable them if - // exemptions exist. + // However, if `target` is exempted as an includer in + // `check_includes_strict_exceptions`, its public headers might still #include + // headers from its private deps, so its private hard deps must still be + // forwarded to dependents. (Exemptions where `target` is only the `included` + // target do not allow `target`'s public headers to include private deps.) return !target->hard_dep() && ((target->check_includes_strict() && - target->settings() - ->build_settings() - ->check_includes_strict_exceptions() - .empty()) || + !target->settings() + ->build_settings() + ->check_includes_strict_exceptions() + .HasExceptionFrom(target->label())) || target->source_types_used().RustSourceUsed()); }
diff --git a/src/gn/resolved_target_data_unittest.cc b/src/gn/resolved_target_data_unittest.cc index 2607438..8698c70 100644 --- a/src/gn/resolved_target_data_unittest.cc +++ b/src/gn/resolved_target_data_unittest.cc
@@ -543,16 +543,40 @@ strict.public_deps().push_back(LabelTargetPair(&pub_action)); strict.private_deps().push_back(LabelTargetPair(&priv_action)); + TestTarget exempted(setup, "//bar:exempted", Target::SOURCE_SET); + exempted.set_check_includes_strict(true); + exempted.public_deps().push_back(LabelTargetPair(&pub_action)); + exempted.private_deps().push_back(LabelTargetPair(&priv_action)); + ASSERT_TRUE(pub_action.OnResolved(&err)); ASSERT_TRUE(priv_action.OnResolved(&err)); ASSERT_TRUE(strict.OnResolved(&err)); + ASSERT_TRUE(exempted.OnResolved(&err)); + + // Add an exception where //bar:exempted is the includer and //foo:strict is + // the included target. + CheckIncludesStrictExceptions exceptions; + exceptions.AddLocalException( + LabelPattern(LabelPattern::DIRECTORY, SourceDir("//bar/"), "", Label()), + strict.label()); + setup.build_settings()->set_check_includes_strict_exceptions( + std::move(exceptions)); ResolvedTargetData resolved; + // //foo:strict has no exceptions where it is the includer, so it only + // forwards public hard deps. const TargetSet& strict_hard_deps = resolved.GetHardDeps(&strict); EXPECT_EQ(1u, strict_hard_deps.size()); EXPECT_TRUE(strict_hard_deps.contains(&pub_action)); EXPECT_FALSE(strict_hard_deps.contains(&priv_action)); + + // //bar:exempted matches an exception as the includer, so its optimization + // is disabled and it forwards both public and private hard deps. + const TargetSet& exempted_hard_deps = resolved.GetHardDeps(&exempted); + EXPECT_EQ(2u, exempted_hard_deps.size()); + EXPECT_TRUE(exempted_hard_deps.contains(&pub_action)); + EXPECT_TRUE(exempted_hard_deps.contains(&priv_action)); } TEST(ResolvedTargetDataTest, HardDepsRustLibrary) {
diff --git a/src/gn/variables.cc b/src/gn/variables.cc index 3b7c9b7..adc7657 100644 --- a/src/gn/variables.cc +++ b/src/gn/variables.cc
@@ -938,8 +938,8 @@ forwarded to dependents. * "deps" act as private implementation dependencies (dependencies required to compile this target's sources). This also allows dependent targets to - compile without waiting for this target's private "deps" (unless - check_includes_strict_exceptions is non-empty). + compile without waiting for this target's private "deps" (unless this + target is exempted as an includer via check_includes_strict_exceptions). When false (the default), the default loose include checking rules apply.