Skip to content

Commit

Permalink
Don't suggest adding a dependency if testonly incompatible.
Browse files Browse the repository at this point in the history
  • Loading branch information
hzeller committed Jun 20, 2024
1 parent d08ed3a commit c708ca2
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 5 deletions.
4 changes: 4 additions & 0 deletions bant/explore/query-utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ class TargetFinder : public BaseVoidVisitor {
current_.name = scalar->AsString();
} else if (lhs == "alwayslink") {
current_.alwayslink = scalar->AsInt();
} else if (lhs == "testonly") {
current_.testonly = scalar->AsInt();
} else if (lhs == "include_prefix") {
current_.include_prefix = scalar->AsString();
} else if (lhs == "strip_include_prefix") {
Expand Down Expand Up @@ -127,6 +129,8 @@ class TargetFinder : public BaseVoidVisitor {
// But until then, we need to check for the constant symbol manually.
if (lhs == "alwayslink") {
current_.alwayslink = (id->id() == "True");
} else if (lhs == "testonly") {
current_.testonly = (id->id() == "True");
}
}
}
Expand Down
1 change: 1 addition & 0 deletions bant/explore/query-utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ struct Result {
std::string_view include_prefix; // ... to manipulate the path ...
std::string_view strip_include_prefix; // ... files from hdrs are found.
bool alwayslink = false;
bool testonly = false;
};

// Callback of a query.
Expand Down
2 changes: 2 additions & 0 deletions bant/tool/dwyu-internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ class DWYUGenerator {
const ParsedBuildFile &build_file);

bool IsAlwayslink(const BazelTarget &target) const;
bool IsTestonlyMismatch(const BazelTarget &target, const BazelTarget &dep,
const query::Result &dep_detail) const;
bool CanSee(const BazelTarget &target, const BazelTarget &dep) const;

Session &session_;
Expand Down
42 changes: 37 additions & 5 deletions bant/tool/dwyu.cc
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,8 @@ void DWYUGenerator::InitKnownLibraries() {
for (const auto &[_, parsed_package] : project_.ParsedFiles()) {
const BazelPackage &current_package = parsed_package->package;
query::FindTargets(parsed_package->ast,
{"cc_library", "cc_proto_library", "alias"}, //
{"cc_library", "cc_proto_library", "alias", //
"cc_test"}, // also indexing test for testonly check.
[&](const query::Result &target) {
auto self = BazelTarget::ParseFrom(
absl::StrCat(":", target.name), current_package);
Expand All @@ -246,18 +247,49 @@ bool DWYUGenerator::IsAlwayslink(const BazelTarget &target) const {
return found->second.alwayslink;
}

bool DWYUGenerator::IsTestonlyMismatch(const BazelTarget &target,
const BazelTarget &dep,
const query::Result &dep_detail) const {
if (!dep_detail.testonly) return false; // non-testonly always compatible.

auto target_detail_found = known_libs_.find(target);
if (target_detail_found == known_libs_.end()) {
return false; // Should not happen, but let's not flag as issue.
}
const query::Result &target_detail = target_detail_found->second;
if (target_detail.testonly || target_detail.rule == "cc_test") {
return false;
}

project_.Loc(session_.info(), target_detail.name)
<< " '" << target << "' is using headers that would be provided by '" << dep
<< "', but the latter is marked testonly, the former not. "
<< "Not adding dependency.\n";
return true;
}

bool DWYUGenerator::CanSee(const BazelTarget &target,
const BazelTarget &dep) const {
if (target.package == dep.package) {
// We can implicitly see all the targets in the same package.
return true;
}
auto found = known_libs_.find(dep);
if (found == known_libs_.end()) return true; // Unknown ? Be Bold.
if (!found->second.deprecation.empty()) {
// Consider a library with a deprecation as not visible.
return false;
}

// If the dependency in question is testonly, but we're not, then
// this is equivalent to not visible.
if (IsTestonlyMismatch(target, dep, found->second)) {
return false;
}

// On to actual visibility checks.

if (target.package == dep.package) {
// We can implicitly see all the targets in the same package.
return true;
}

List *visibility_list = found->second.visibility;
if (!visibility_list) return true;
bool any_valid_visiblity_pattern = false;
Expand Down
28 changes: 28 additions & 0 deletions bant/tool/dwyu_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,34 @@ cc_library(
tester.RunForTarget("//some/path:bar");
}

TEST(DWYUTest, DoNotAdd_IfTestonlyMismatch) {
ParsedProjectTestUtil pp;
pp.Add("//lib/path", R"(
cc_library(
name = "foo",
srcs = ["foo.cc"],
hdrs = ["foo.h"],
testonly = True,
)
)");

pp.Add("//some/path", R"(
cc_library(
name = "bar",
srcs = ["bar.cc"],
# needed //lib/path:foo dependency not given, but it is testonly.
)
)");

DWYUTestFixture tester(pp.project());
// No add expected.
tester.AddSource("some/path/bar.cc", R"(
#include "lib/path/foo.h"
)");
tester.RunForTarget("//some/path:bar");
EXPECT_THAT(tester.LogContent(), HasSubstr("is marked testonly"));
}

TEST(DWYUTest, Remove_SuperfluousDependency) {
ParsedProjectTestUtil pp;
pp.Add("//some/path", R"(
Expand Down

0 comments on commit c708ca2

Please sign in to comment.