Provide a better way to handle ambiguous headers in BCR indexer

[mateusz-olczyk]

Problem

Module.WithAmbiguousTargetsResolved() silently drops ambiguous headers without allowing gazelle to decide what to do with them. The headers won't be included in bzldep-index.json.

@jayconrod @WojciechMazur , I'm interested in your opinions about the problem.

Real-world example

E.g., every source in grpc/examples/cpp/helloworld contains #include <grpcpp/grpcpp.h>.

Unfortunately, this header is exposed by two targets in grpc module:

$ bazel query --keep_going 2>/dev/null '
  rdeps(//..., "//:include/grpcpp/grpcpp.h", 1)
  intersect kind(cc_library, //...)
  intersect attr(visibility, //visibility:public, //...)
  except attr(testonly, True, //...)
'

//:grpc++
//:grpc++_unsecure

Core differences are as follows:

	//:grpc++	//:grpc++_unsecure
extra hdrs	//:src/cpp/client/secure_credentials.h //:src/cpp/common/secure_auth_context.h //:src/cpp/server/secure_server_credentials.h	🚫
extra deps	//:gpr_public_hdrs //:grpc++_base //:grpc++_xds_client //:grpc++_xds_server //:ref_counted_ptr //src/core:slice @com_google_absl//absl/base:core_headers @com_google_absl//absl/types:span @com_google_protobuf//:protobuf @com_google_protobuf//src/google/protobuf/io:io	//:channel_arg_names //:gpr //:grpc++_base_unsecure //:grpc++_codegen_proto //:grpc_security_base //:grpc_unsecure //src/core:grpc_insecure_credentials

In this particular case, we can assume that the user might prefer //:grpc++ over //:grpc++_unsecure because //:grpc++ is more generic. And usually, insecure transport is used only during the prototyping phase, quickly evolving later to some encryption method. Sometimes the choice is less obvious.

Anyway, something should be chosen, because almost always the user wants to set up a server and/or a client. Putting appopriate cc_grpc_library in deps won't be sufficient then.

Solution

1. BCR indexer should include ambiguous headers in the output JSON file.
2. Gazelle should later react somehow to this.

Questions

File format

The current file format is:

{
  "hdr1.h": "@repo1//:target1",
  "hdr2.h": "@repo2//:target2"
}

cannot be simply extended and has to be changed.

I see 2 approaches to achieve this.

Lists instead of single values

{
  "hdr1.h": ["@repo1//:target1"],
  "hdr2.h": ["@repo2//:target2"],
  "ambiguous_header.h": ["@other_repo//:target1", "@other_repo//:target2"]
}

Root-level distinction

{
  "unique" : {
    "hdr1.h": ["@repo1//:target1"],
    "hdr2.h": ["@repo2//:target2"]
  },
  "ambiguous": {
    "ambiguous_header.h": ["@other_repo//:target1", "@other_repo//:target2"]
  }
}

Gazelle behavior

How should the gazelle react in case of ambiguity? Should we provide some heuristics for automatic choice? We should consider 2 scenarios:

Dependency already specified

Referring to the example above, we should respect the user's decision when one of //:grpc++ or //:grpc++_unsecure is already put in deps of a rule. Whatever was chosen, we should not modify the deps, requiring the user to use # keep directive. No warnings should be displayed.

No dependency specified yet

It happens usually when there's no rule, and we generate it from scratch for the first time. Or, less often, the rule exists, but none of the ambiguous dependencies are specified.

What should we do? I think we should at least display a warning.

Should we additionally put one of arbitrarily chosen dependencies into the deps list?

• pros: increasing the chance that the project will be ready to build, minimizing the amount of work required to make everything buildable after the gazelle run
• cons: the user may ignore the displayed warning and forget to think more about the decision; in the opposite approach, the linking error could not be ignored

Cross-module ambiguity

Should we consider only ambiguities within a single module? Or should cross-module ambiguities also be displayed as warnings?

[jayconrod]

As I understand, when a header is provided by multiple targets during indexing, we pick one as follows:

// Strategy 1 - Single Root: If all targets with overlapping headers form a dependency chain
// where one target depends on all others (directly or transitively), that root target is
// selected and gets all the headers.
//
// Strategy 2 - Common Dependent: If multiple root targets exist (e.g., A and B both define
// header.h, neither depends on the other), but there exists a target C that depends on both
// A and B, then C is selected and all headers from A and B are merged into C.
//
// Strategy 3 - Exclusion: If no common dependent exists, headers that appear in multiple
// root targets are EXCLUDED from all of them. Each target keeps only its unique headers.
// This means some headers may not be indexed at all if they cannot be unambiguously assigned.

So I think the problem here is with strategy 3: grpcpp.h is excluded because it appears in multiple targets. I agree this is not the right behavior. We should resolve that header to something or prompt the user what to do.

How about one of the following options:

1. During indexing, choose a target arbitrarily, say, the first in lexicographic order. We can collect a list of hardcoded overrides for when the wrong choice was made. The user can override with a # gazelle:resolve directive. We might want to print a warning if no # gazelle:resolve directive is present.
2. Don't resolve the header, but print a warning, telling the user to add a # gazelle:resolve directive.

If we print a warning, it should tell the user what the possible targets are so they can add the right # gazelle:resolve directive. That means we'd need to adjust the JSON format, as you suggested.

[jayconrod]

Current state:

The indexer for BCR is not fully implemented for the new format, so the BCR index doesn't account for ambiguities at all. Manual indexers do support the format though.

Within gazelle_cc, we've implemented some automatic selection in ambiguous cases, but even if the BCR index were complete, it doesn't seem like it picks the right target for a given header. So this may need another look.

[mateusz-olczyk]

I described one of the problems and my intuition in the description of #183:

For some reason, the algorithm resolves "grpcpp/grpcpp.h" only to "@grpc//:grpc++_codegen_proto", which is incorrect.

bazel query 'somepath(":grpc++_codegen_proto", ":grpc++")' returns empty result, which is bad, because :grpc++ contains the crucial compiled code.

I guess the function GroupTargetsByHeaders() is not the right strategy for solving conflicts, because it groups targets if they intersect with at least one header in their "hdrs" lists - whatever it is.

Instead of grouping targets like this, we should consider the mapping from each recognized header to its parent targets. And then resolve conflicts in such defined groups of targets. This way, each header is treated separately.