Cmdliner completions break if CMDLINER_LEGACY_PREFIXES is true at --__complete time

[WardBrian]

For example, this actually works fine:

$ CMDLINER_LEGACY_PREFIXES=1 stanc -<TAB>
-?                              --debug-generate-inits          --debug-transformed-mir         --info                          --print-cpp

But this, which would usually be equivalent, does not:

$ export CMDLINER_LEGACY_PREFIXES=1
$ stanc -<TAB>
Usage: %%NAME%% [--help] [OPTION]… [MODEL_FILE]
stanc: option - ambiguous and could be one of --O0, --O1, --Oexperimental,

In some ways this seems silly -- it's listing all the valid completions in the error message!

To directly see the issue, the same error arises if you

$ CMDLINER_LEGACY_PREFIXES=1 stanc --__complete --__complete=-

(it works fine if there are prefixes used before the --_complete= item, or if it's unambiguous, like in stanc this does offer valid completions even though the argument names are auto-format and canonicalize: $ CMDLINER_LEGACY_PREFIXES=1 stanc --__complete --__complete=--canon=<TAB>)

---

In particular, this means that the patch you suggest here and here breaks completion for more or less all options, as - will always be highly ambiguous. Some sort of special-casing seems worthwhile?

[WardBrian]

I'm still trying to grok the code in the cmdliner_cline.ml, but it looks like Not_found is already special casing this, and simply expanding that logic does seem to fix the issue? e.g.

--- i/src/cmdliner_cline.ml
+++ w/src/cmdliner_cline.ml
@@ -202,7 +202,7 @@ let parse_opt_args
           in
           let cline = Cmdliner_def.Cline.add arg_info arg cline in
           loop errs (k + 1) comp cline pargs args
-      | Error `Not_found when for_completion ->
+      | Error (`Not_found | `Ambiguous) when for_completion ->

[dbuenzli]

In fact CDMLINER_LEGACY_PREFIXES was never intended for users to reclaim the past behaviour for themselves, it was meant to be used to easily salvage scripts that were inadvertently using the prefix behaviour. So I was perhaps a bit too fast at proposing these solutions on the dune or opam repos.

If what you propose works then we can go with that but I don't want to make it more complex than that. Projects who want to retain the pre 2.0.0 behaviour should hack around themsleves.

[WardBrian]

I understand the intent completely. I probably would have found this during my earlier work but we only had one flag that was advertised by its prefix and I could just special-case that one in some argv processing and avoid the LEGACY flag entirely.

opam (and to a lesser extent dune) seem to rely on it a lot more, so I understand their hesitance and am trying to respect it in the hopes of getting something merged.

---

Anyway!

That one-liner diff does seem to have resolved the issue, at least as it first presented. There might be more bad interactions between the two still undiscovered, but "broken in some edge case I can't spot in 10 minutes" is probably preferable to "broke on the very first thing I tried"

Plus, since `Ambiguous is only generated when CMDLINER_LEGACY_PREFIXES is set, it's impossible for that diff to break the 'happy path', so I'd argue it's worth the change.

What do you think? In general I agree that if there end up being more significant hacks needed, I agree those can/should live in the vendored downstreams, but this seems like a natural path in the code. If you don't want to mention `Ambiguous more, perhaps the equivalent Error _ when for_completion?

[dbuenzli]

What do you think?

If what you propose works it's totally in. But I don't want to start hacking around (especially not in the completion scripts :-) more than that if completion really relies on the new behaviour.

[WardBrian]

I've had it in my branch of opam while playing around for the hour since posting and I'd say yes, it does work as intended