Enable use of carapace completer as a library

[maxlandon]

Hello Rob, here again for a feature request, which will litterally save me.

Main subject

In the context of three projects of mine,

• readline
• console
• flags

... I need to be able to use the carapace completer from within a shell program.
Not only that, but on top of it (and that is because the Complete() function
of this commit also returns the common.Meta), it enables the readline shell
library to have as fine-grained a control over the suffix pattern matchers.

So basically this commit is way, way less complex than the previous PRs I've submitted:
it just Exports the main Complete() function, and returns the RawValues and the Meta.
It makes the necessary adjustements for the traditional cobra _carapace command.

Needless to say it again, on this PR rests the entire future of the projects above, and
be sure that if accepted, they would have a definite edge over what exists in the space
at present. Having a full-fledged CLI application will become as trivial as it can be.

I would love to present you a demonstration of the results right now: it's running great.
It's just in my go workspace for now and I'm polishing a few things before pushing it.

Remarks on this PR code

• If I were to object to my own work: so your exported API is clean and small.
  Additionally, there is no way to move any further completion logic internally
  (and there is absolutely no problem with that, don't get me wrong), so maybe
  the name of the exported Complete() could be changed to something more explicit,
  or something that would ensure no one will use it mistakingly.
  (On the other hand, Complete() is fine if one considers that none of your API
  functions start with this verb)
• Your last 4 commits (flags tests) have not been merged in this branch, although
  it seems to me that they are not conflicting.
• I added a / NoSpace suffix matcher to path completion: as far as my library is
  concerned (but I guess it applies to all), it further enhances the default spacing
  and autosuffix removal for path completion/insertion when the candidate is a
  directory (and only if a directory).
• A thousand times sorry for my pesky editor adding dots to comments everywhere.
  I'll find a way to deactivate that, just did'nt look into it yet...

Additional things.

So I got over your code /recent commits (which I get to know quite well by now), or just used it
within the course of the aforementioned projects, and I got to remember that there
were a few side-fixes in my other PRs (for instance, better error filtering when flags
raise errors because they've not been fed a result yet).
And some other things, or suggestions.
I'll open separate (and each of them very small, I promess) PRs for those, so that you
can cherry pick.

Once again, really great, great work with this library. I don't cease to be amazed at what you've done.

[rsteube]

I'm a bit worried about the implications this has. Having a global storage was fine so far since the whole thing has a focused purpose and shouldn't cause issues in normal usage.
Completely resetting it to get rid of the old references doesn't seem like a great idea at the moment.

I kept internals like RawValue so far private so i could make larger changes under the hood. Shell completion is quite complicated and i still expect some modifications here and there.
There's already Export in place though for interchanging completions so it's not too far off.. 🤔

[maxlandon]

• Storage: indeed storage being a single global var does not pose problems. Additionally, since the keys in the map are pointers to cobra commands, there is no change of name collision per the command name, (like gh pr create and gh issue create), since both create commands are different pointers.
• Storage reset: you will notice that this reset does not happen for normal shell completion: this reset is only called in the internalValues() function, which is only used when the completions are requested from the same runtime.
• Export: I thought of using it also. I ended up trying the approach above instead, because using export means having carapace being installed on the target system. With respect to using carapace "as a library", that would mean having a runtime dependency... which can be avoided.
• Also with Export: it does not return the common.Meta field, especially its suffix matchers, which can be very useful for a consumer of the completions (especially a Go program consuming them as a library)

Obviously, like for the other PRs, I'm neither in a hurry nor pushing for changes that would have long-term implications !

[maxlandon]

Pinging you on this too...

I still desperately need to compile carapace into a console binary...
As said in the first message, its just wonderful and working wonderfully great...
This is only possible thanks to your work => https://github.com/reeflective/console#showcase (and yes this is a closed-loop console)

But... having carapace executable as a runtime dependency is just too much when one can simply compile the logic into the binary (also considering the myriad carapace-bin completers than can then be used along)

Regardless, I'll merge the most recent carapace branch into this PR one, to keep those unsuccessful checks silent...

[rsteube]

I'll have another look at it, but i think the biggest blocker to me was that there is currently no way to store arbitrary data to a command (there is Context, but that is intendend for sth. else). Not too fond of the current storage solution and the necessity of clearing it.

Embedding carapace-bin wouldn't work btw. due to how the commands are constructed (using init) as well as some other design decisions.

Challenge here is to consistently recreate cobra commands (as they can be executed only once) and to do a proper cleanup (for which it would be best to store the completions in the command and let the garbage collector do that).

[maxlandon]

Storage type: this is up to you, I don't have an opinion on this. Storing arbitrary data for a command: you might not even realize it yourself (and that is a great compliment) but the way you designed your library just makes this possible out of the box, without any bugs or needs for workarounds !

Embedding carapace-bin: this not what I'm talking about here, this is about embedding carapace (and only carapace), that is, only embedding the completion engine. Thus there are no concerns about init functions.

Challenge: so this is also something I had to solve (and did) in my console library. The simple and fully working workaround is to declare (bind) commands in a function that is called after each execution loop (see link shared below).

To be clear: my console library works flawlessly with your carapace engine used as a library. It works just as good as used from a system shell with caparace-bin ! Precisely because my console uses cobra commands.

I mean... I think you don't even realize how well designed your library is, and just how easy it is to use it in my own context ! A few lines to illustrate, if you want to see a proof for yourself:

• The entire completer code of my console (50 LOC): https://github.com/reeflective/console/blob/77a4695d6f8a2cbab2ffa540ab891e06d7f607d0/tab-completer.go#L12
• How cobra commands can be used closed-loop (see first section, and last section for completions): https://github.com/reeflective/console/wiki/Binding-Commands.

[rsteube]

Yes it works. What I am more concerned about is concurrency and cleaning up instances.
Cobra was developed with a single execution in mind.
Carapace even more so as i was focussing to get things working first and am slowly polishing things up.
Context and getting rid of the os.Args modifications fixed a lot of the concurrency issues but storage still has no mutex preventing concurrent access.
Fully resetting the storage would also clear the completions of other commands and there's flagCompletionFunctions in cobra as well which will grow continuously.

[maxlandon]

I pulled your master changes onto this branch, for the sake of keeping up-to-date (this at least allows us to debate this with your most recent work). To note, even 200 commits later, everything seems to work great.

I also saw your refactored your complete() command with a traverse() one returning the action and context, which facilitates things even more. (I highly appreciate being able to use this even without being merged into master, kudos to you again !)

About this failing lint test: I'm not sure what causes that, as the code is strictly identical to master except for a newline added by my automatic formatter (I gave up trying to mute it for a few things)... If you have any idea or solution you would save me once again !

Demo:

[maxlandon]

About that last sentence in your last message:

Fully resetting the storage would also clear the completions of other commands and there's flagCompletionFunctions in cobra as well which will grow continuously.

So it's true that as my current implementation stands, all commands completions are being cleared.
However, it seems to be me that it's not a problem: since in a closed loop application you must bind your commands from within a function to be called after each execution, you should as well include any call to carapace.Gen() or any of its resulting carapace.Flag/ArgCompletion() calls into this function as well. In fact it's needed because you want to be able to complete any flag, any number of times before executing.

About the cobra.FlagsCompletion remark, I'm not sure what you mean. Maybe that was just meant to illustrate the beginning of that sentence though... If yes nevermind: I think I then also answered this in my first point.

[maxlandon]

Thinking of the storage type:
In my code I get to call carapace.Gen(cmd) on various subcommands, so as to bind their respective positional/flag completions.

In fact and despite the quite simplistic storage model (as it stands now), it is quite efficient because it obsviously stores pointers to commands, so that when carapace has to resolve the command to complete, it cannot confuse one command for the other, since they are all different in memory, and this regardless of how complex/deep your command tree is, or regardless of two subcommands having the same name.

Lastly, and related to this, the only (not bothering but redundant) thing that could be done is:
when carapace.Gen(subcmd) is called, if the subcmd.Root() already has a _carapace subcommand, don't add one to the subcmd one.
I'll PR you a small fix for this on master in a few moments.

[rsteube]

It is not known when a command is added to a parent which makes root command determination pretty much impossible.
So I just add the _carapace subcommand to each command.

The references themself aren't an issue but they prevent garbage collection which will be a problem in a long running app where commands are constantly created.

Regarding cobra's flagcompletion: cobra has global variables which keep references which can't be cleaned up because they are private.

[maxlandon]

Carapace command: thanks for precising.
Pointers not garbage collected: I had not considered this. I'll try to test a few things to see if this can be solved.

Thanks !

[maxlandon]

I was looking at how to fallback on cobra completer functions for one my mentionned projectss, and that was a good occasion to realize why you didn't implement such fallbacks yet, because there are quite a few things to be sure of first.
I saw this pesky completion map singleton...
I will make tests this week, to see if this singleton map raises problems here.

Overall implementing a fallback to cobra completer functions is not really hard fortunately, and will be really great once implemented in carapace. Between others, it seems that cobra added support for usage messages recently.

This last remark (and most of this message) is not much related to this PR,,, but just to say some of your answers and your patience is suddenly clearer to me.

[maxlandon]

Hello,

Just a quick ping to know if you had time to reflect on this, or if you had any related ideas, or things to watch out for. To be clear this is just a ping for attempting to slowly but efficiently narrow down the problems at hand.

As said in my very last comment, I've come to understand why this whole stuff needs to be forethought, and that anyway it also depends on the default behavior of other libraries (cobra in the first place).

I also did not have time to to try new implementations (nor had a really good model come to my mind) for the storage map problem, but I've worked a lot on libraries related to the space, so on my side I'm accumulating knowledge on what to do what not.

So... yes any suggestion or note to add here would be welcome !

[rsteube]

I've been thinking about it occasionally but haven't got a solution for it yet.
So far i'm afraid this would need a stronger decoupling from cobra as there are limits as to what can be done as dependent library.

[maxlandon]

Thinking out loud and writing this down:

• It might be useful to intervene in the cobra repo discussions/issues/PRs about completion in general. From what I've seen, they seem to be on a trend where everyone is going to slowly and non-uniformly add support for his own shell, and as we both know this will end up in something at least light-years away from carapace (btw took me a year to get the why of the name).
  By this I don't forcefully mean to intervene with civilisational talks, but more like "hey this global map of completion flags is a bad idea", or "hey would be good to apply to completions the same encapsulating of command core (ie. forcing/helping people not to use init()). Or another "hey cobra please stay minimalist in assumptions to consumers of your lib"). You get the point.
  I might get into this, drop a few messages, but since carapace is not my library, I would rather either:
  • Not do it myself or alone.
  • Do it with your consent/ideas/contribution.
  • Speak further here and refine the idea before anything.

[rsteube]

I tried to reach out in the past, but it didn't seem possible to get anything forward at the time.
Things have vastly improved in recent years though so you can try to get involved if you want.

[maxlandon]

Trying to advance a little bit on this.
Summary of my work/thoughts/tests on this:

Avoiding global state for correct garbage collection

• So as you know we're close to avoiding global state in cobra completions. The work should be merged in the next release (1.9.0).
• However, I don't see any way to avoid the global storage map in the carapace code, and consequently any other way than simply clearing this map when carapace is used as a library.

Alternative (but a bad one)

I only see of one, and I'm not really fond at all with it... The only place where a carapace.Carapace object can be stored non-globally (per-command/root), is in the command.Context type.
I'm just saying this for the sake of exhaustiveness... I don't like this path at all.

Most likely working solution

To me, the current way (as in this PR) to deal with closed-loop usage of carapace is to periodically clear the storage map and regenerate it. The good news here is that if cobra global state was the only obstacle to this, then the problem is likely to be solved very soon.

What would be great

Maintaining this PR requires some work. Not big big tons, but still some stress and headaches each time (since I only catch up on your master every 3-4 months. All of this for the equivalent of a patch.
Not trying to push you against the wall: I've now had more than one occasion to see how your conservative mind has been beneficial to all -including me as a user of your lib-, however please be sure that I'm forever grateful if we can keep advancing on this, until a solution is found.

(it will also help me get rid of a forked branch that I have to use in conjunction with a go replace directive in various consumer repos -including big ones-)

Available when you need to work this out !

[rsteube]

Had a look at Context before as well, but it is used for something else (and could be overwritten).
What's needed is an additional map in Command.
I'll open an issue for it in cobra, but I don't expect it being resolved quickly.

spf13/cobra#2083

[rsteube]

Would https://github.com/rsteube/carapace/compare/experimental work for you?

[maxlandon]

Hello @rsteube

Absolutely perfect, it works like a charm !
Fits for me and my lib !