Skip to content

Conversation

@lf-
Copy link
Contributor

@lf- lf- commented Jun 17, 2025

Prefer using the short name when referring to an eponymous target (//x instead of //x:x). If
you are in the same package, prefer the local reference (:x instead of //x).

See: https://bazel.build/build/style-guide#target-naming

This syntax is supported on the command-line, but not in BUCK files. Unfortunately, the only(?) Starlark formatter (buildifier: https://github.com/bazelbuild/buildtools) automatically abbreviates targets on the assumption that this syntax is valid, which causes errors when using Buck2:

...
4: Error coercing "//src/Foo"
5: Invalid absolute target pattern `//src/Foo` is not allowed
6: Expected a `:`, a trailing `/...` or the literal `...`.

We can adjust the parsing rules to allow this syntax in more places.

Supercedes #925

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 17, 2025
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

@lf-
Copy link
Contributor Author

lf- commented Jun 17, 2025

From #925

Discussed this with @wavewave. We would really like to have this feature because of Bazel compatibility and dev familiarity (fewer divergences with the command line and less repeating yourself) and we decided to keep holding on this patch internally as we think it's quite important to UX, so I would appreciate getting it upstreamed.

I think the solution to the short target syntax being objectionable/confusing when reading buck files is to have the formatter rewrite it to the canonical form, if that's an important property to have internally at Meta.

@thoughtpolice
Copy link
Contributor

I missed the original patch and to be honest, not sure how much it means for my vote, but I'd quite like this in my codebases too, I think (FTR, I didn't know Bazel did this.) I rely on the shorthand syntax on the command line extensively because it's nice with the autocomplete support, so I agree it'd be a nice bit of uniformity.

Thanks for resurrecting this! I might try it out in my fork for a few days.

@lf- lf- force-pushed the always-infer-target branch from 7bda312 to 510a0c9 Compare June 21, 2025 00:31
@lf-
Copy link
Contributor Author

lf- commented Jun 21, 2025

Fixed the broken tests and rebased :)

@lf-
Copy link
Contributor Author

lf- commented Jul 7, 2025

@JakobDegen checking in regarding this

@lf- lf- force-pushed the always-infer-target branch 4 times, most recently from 14cffb8 to ec8ca7b Compare July 15, 2025 23:37
@lf- lf- force-pushed the always-infer-target branch from ec8ca7b to a2a96c5 Compare July 19, 2025 00:11
@JakobDegen
Copy link
Contributor

Alright, so we chatted about this in our team syncs. We're not super thrilled about this - the general vibe is that we want people to understand what the things they write mean, and this probably hurts that. This applies to the command line too fwiw, I don't think we'd feel great about implementing it today if we hadn't done that already.

That being said though, bazel interop seems like an ok reason to add this and if someone does want to use it that's probably up to them. So we're ok with this, under the requirement that this has to be behind an off-by-default buckconfig

@thoughtpolice
Copy link
Contributor

I was looking at this PR recently and wondering how feasible it was to add a buckconfig flag for it. I've experimented with it a little and actually think it's nice, but haven't migrated code to use it yet. Unfortunately, it's so deep inside the code I'm not sure there's an easy way to get the buck context there, without just threading a parameter through everything? So that's an annoying lift.

This applies to the command line too fwiw, I don't think we'd feel great about implementing it today if we hadn't done that already.

Honestly, I find this one to be a useful timesaver feature as a user on the CLI. It makes shell completion much quicker without having to tab through a selection menu on the targets of a given package and pick the "right" one. Normally I'm just autocompleting what are effectively directory paths in the root// cell anyway, so just completing the directory is enough. So I guess this is just to say I think it's useful even if some find it a little ugly. :)

That being said though, bazel interop seems like an ok reason to add this and if someone does want to use it that's probably up to them. So we're ok with this, under the requirement that this has to be behind an off-by-default buckconfig

One problem I realized with this approach is: what happens when you try to use some code that uses the short-syntax, but you want to prohibit the short-syntax in your own code? For example, what happens in this scenario where I depend on an external cell foo//, and foo// needs this feature enabled (because its BUILD files are written assuming it is), but root// does not want it enabled? Can this scenario be handled correctly? There might be others I haven't thought of.

I wonder if there is a better approach in possibly making the use of this feature some kind of toggle-able warning/lint error instead of strictly enabling/disabling it, and instead it was always enabled all the time. Then users can just turn off the warning instead, but at Meta it would remain a hard error, of some sort?

> Prefer using the short name when referring to an eponymous target (`//x` instead of `//x:x`). If
> you are in the same package, prefer the local reference (`:x` instead of `//x`).

See: https://bazel.build/build/style-guide#target-naming

This syntax is supported on the command-line, but not in BUCK files. Unfortunately, the only(?)
Starlark formatter (buildifier: https://github.com/bazelbuild/buildtools) automatically abbreviates
targets on the assumption that this syntax is valid, which causes errors when using Buck2:

    ...
    4: Error coercing "//src/Foo"
    5: Invalid absolute target pattern `//src/Foo` is not allowed
    6: Expected a `:`, a trailing `/...` or the literal `...`.

We can adjust the parsing rules to allow this syntax in more places.
@lf- lf- force-pushed the always-infer-target branch from a2a96c5 to 34b67e4 Compare September 13, 2025 00:58
@lf-
Copy link
Contributor Author

lf- commented Sep 13, 2025

still TODO: address feedback. I need to figure out how to thread the config through.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants