Enhance label.rs to support implicit targets and expose absolute-ness.

[cfredric]

This PR enhances the label parsing library (label.rs) to allow it to support implicit targets (e.g. //foo/bar has an implicit target of bar, and is equivalent to //foo/bar:bar), and to expose whether the label that was parsed was absolute (e.g. to distinguish //foo and foo).

I also reworked some of the implementation of consume_package_name, to make it a little more idiomatic and easier to follow (IMO).

[illicitonion]

Thanks for the contribution! The implicit target stuff, and general rework, looks great, but I'm not so sure about exposing the absolute-ness of the label - what's your use-case for consuming it?

It feels like in general important questions are things like "Are these in the same package" rather than "Was this written absolutely in a BUILD file", so I'm curious as to how you're looking to use the API

[cfredric]

The use case is to allow callers to distinguish labels like foo/bar:baz and //foo/bar:baz from each other. The current implementation of Label provides no way of distinguishing these cases. However, they're not equivalent. The former is a relative label (relative to the BUILD file's package, if we're in a BUILD file at all), and the latter is an absolute label. The test_package_name_parsing() test shows that both those examples are parsed to have a package_name of foo/bar; the repository_name and name fields will also be the same.

It would be nice to be able to distinguish these cases if:

• we're parsing labels that are not in a BUILD file (and therefore don't have a notion of "the current package", so we cannot permit relative labels); or,
• we're trying to resolve labels in a BUILD file to the current package (so we need to know which labels need resolution and which do not).

My use case is the former (parsing labels in Rust source code where I want to disallow relative labels); this is related to #1008. Specifically, I'd like to use this parsing library in the proc_macro implementation (assuming I contribute the macro upstream, too), instead of writing my own parser.

[cfredric]

Come to think of it, it's not even possible to answer the question "Are these in the same package" with the current implementation. It's possible to have two Labels with the same package_name, where one label is relative and the other is absolute. Then it's (currently) impossible to know whether their packages are really the same or the package names just overlap, e.g. //utils vs utils (abbreviated from //foo/utils).

Exposing the is_absolute bit helps this slightly, by allowing us to say "these are definitely in the same package" for two absolute labels whose repository_names and package_names match; or "definitely yes" for two relative labels whose package_names match; but "maybe" for an absolute label and a relative label whose package_names match.

[illicitonion]

Aha, that's an interesting corner! I was generally thinking about BUILD files, where relative labels aren't allowed at all; things either need to be listed as absolute labels (//...) or intra-package labels (:...).

How would you feel about adding a "context" param (i.e. the directory from which the label is being resolved, relative to the workspace root)? That way we can always normalise labels to be canonically represented, and get rid of that ambiguity completely.

[cfredric]

Let me caveat my answer by saying I'm not a Bazel expert :) but: that would work for the use case of analyzing a BUILD file (or more generally, analyzing something when you know where you are in the repo), but I worry that it wouldn't help my use case. I want to analyze/rewrite labels in Rust source via a proc_macro, to turn something like:

import! {
  "//some_project:utils";
  "//some_project:ugly_name" as something_else;
  "//third_party/rust/serde/v1:serde";
  "//third_party/rust/clap/v2:clap" as aliased_clap;
}

into

extern crate some_project_COLON_utils as utils;
extern crate some_project_COLON_ugly_name as something_else;
extern crate serde;
extern crate clap as aliased_clap;

In order to provide the "context" to that macro (to resolve relative labels), I'd need to know the Bazel package of the file that invoked the macro. There's a nightly API that would let me grab the source file of the caller, but I'd still need to know what package that file is in, which (to my knowledge) wouldn't be possible without doing a bazel query or doing a directory traversal myself (to find the location of the nearest ancestor BUILD file).

On top of that complication, I don't even want to resolve relative labels in this macro invocation; I want to prohibit them entirely. (Requiring labels to be absolute makes large scale changes, e.g. moving a widely-depended-upon crate, more straightforward: I can just do a simple find/replace of the absolute label, instead of worrying about all the ways it could be specified relatively.) So even if I could easily provide the context, it still wouldn't give me a way to do what I want.

---

Maybe an idea is to have the parser accept an optional context? That way, the parser can either resolve relative labels unambiguously (if context is Some(package)), or can disallow relative labels entirely (if context is None), and the Label struct can still be modified to always be unambiguous. WDYT?

[illicitonion]

Maybe an idea is to have the parser accept an optional context? That way, the parser can either resolve relative labels unambiguously (if context is Some(package)), or can disallow relative labels entirely (if context is None), and the Label struct can still be modified to always be unambiguous. WDYT?

That sounds good to me (or even having two public entry-points, one which requires a context and supports relative, and the other which does neither.

But - it sounds like we'd both be happy simply rejecting labels which don't either take one of the following forms:

• Absolute, fully-qualified (//foo:bar)
• Absolute, implicit name (//foo)
• Relative (:bar)

I don't really see a use for supporting foo:bar, and it sounds like you don't either?

[cfredric]

Yup, I'd be happy with that - then I could just check that everything had a non-None package.

Actually, according to https://docs.bazel.build/versions/4.2.1/build-ref.html#labels, foo:bar-style labels are forbidden anyway ("Relative labels cannot be used to refer to targets in other packages; ..."). So I think your suggestion is already the status quo in Bazel, and our parser is just a little too permissive. I can take a stab at fixing that up, if you agree?

[hlopko]

I agree with all you said, and I like the PR, thank you @cfredric.

Regarding the relative labels in the form foo:bar, those cannot appear in the BUILD file, but you can use them to tell Bazel to build something (for example bazel test //test/... works the same as bazel test test/... from the root of rules_rust; if your CWD is somewhere deeper, they are also usable). So eventually we will want to support that in some form. I'm fine with submitting this PR as is, and maybe adding a TODO linking a new GitHub issue to add this support in the future.

@illicitonion please feel free to merge this PR in after reviewing/approving. Thanks!

[illicitonion]

LGTM - thanks!