Improve error on invalid -//foo and -@repo//foo options

[fmeum]

-//foo and -@repo//foo are always invalid Bazel options, but are usually meant to be either negative target patterns (which have to come after the -- separator) or Starlark options (which always start with --).

With this commit, the error shown to the user mentions these two situations and how to fix the issue in each case.

[fmeum]

@aranguyen Could you review this PR?

[michajlo]

Is there any more discussion around this and why putting the patterns after -- isn't sufficient?

[fmeum]

Is there any more discussion around this and why putting the patterns after -- isn't sufficient?

It's sufficient if you know about it, but the error message you get if you don't doesn't give any indication of this being how to fix your command line. Instead of catching this case and emitting a more helpful error message, it seemed simpler to just make the most common, unambiguous case work out of the box.

I don't have more context than https://github.com/CodeIntelligenceTesting/cifuzz/pull/383#discussion_r1005694516 and similar past experiences that myself or coworkers had with this limitation.

Given his reaction to the PR, maybe @brentleyjones can provide a separate opinion.

[brentleyjones]

I've just run into the same situation myself, and agree that it's hard to know when you need --, and if we can make this work without it 👍.

[michajlo]

IIUC this is a quite common convention for CLI tools, and while it is confusing I fear changing things will only move confusion around, not reduce it. The one example that I believe is cited in the code comments is that relative labels (which I personally use more often) behave differently. It's also liable to cause confusion around starlark-defined flags, which can look like target patterns but are indeed flags.

My interest here is keeping command-line parsing semantics as tight as possible so that any attempted automated introspection of command lines doesn't need to be too smart.

[fmeum]

I agree that not supporting all negative target patterns has potential to create confusion in different places and that supporting any kind of new syntax will make command line parsing more difficult.

Starlark flags shouldn't add to the confusion though as they always starts with two dashes whereas negative target patterns start with one - isn't that a clean separation?

Given that, would you support a version of this PR that doesn't add support for absolute negative target patterns without a --, but instead fails with a more descriptive error?

[michajlo]

Starlark flags shouldn't add to the confusion though as they always starts with two dashes whereas negative target patterns start with one - isn't that a clean separation?

I'm thinking of how one might be tricked into thinking -//foo is a valid syntax because the parser accepted it, or typo-ing -//foo and not noticing because the parser accepted it.

Given that, would you support a version of this PR that doesn't add support for absolute negative target patterns without a --, but instead fails with a more descriptive error?

As long as it can be done tastefully sure. Thought it may not be me stamping it, I'm mainly here because I have an interest in parsing semantics.

[aranguyen]

@fmeum Thank you very much for sending the PR. I just had the sync with the team and brought up your PR. We agree with #16563 (comment) and will be supportive of reviewing another PR that shows a more descriptive error.

[fmeum]

@aranguyen I updated the commit and the PR description, please take another look.

[aranguyen]

LGTM. Thanks!

[iancha1992]

@bazel-io fork 6.3.0