Strongly typed API

[KillTheMule]

General thoughts

A lot of the API takes or returns a Value, and leaves it to the user to sort out what to put in or get out. Nvim-rs will try to improve upon this.

• One tool will be using TryFrom on the Values sent from neovim and return an error containing the value if it failed. This will need an additional variant in CallError, so it's a breaking change.

  • 6a8a154

• A second thing to do is to handwrite more if the API and manually do conversions that aren't done by the autogenerated API. Below is a list of functions which are affected, grouped by some ad-hoc thoughts, sometimes with comments. Those changes will of course be breaking, too.

• Thirdly, provide common structs. This will come from rewriting the API mostly, but contain some more things (events, like nvim_buf_line_events, from Values... which are there? UI events for sure, which are quite an undertaking in themselves).

• A fourth, unreflected idea is to help people handle requests/notifications, which wrap arguments as Values. There's no way to know the structure beforehand, so the way probably is having people define structs and use rmp-serde for deserialization. So we could write examples? Does that need new error variants? I.e. what does serde emit, and do we need to return that?

Plans

Changing CallError and using TryFrom should be a next step. Needs a new release of msgpack-rust, though, might need to ping the maintainers.

Changing the API is a major undertaking, and might be open-ended. Maybe we can do it in batches, grouped like below? Would like feedback :)

Providing event types can probably be made non-breaking (with the right use of generics, since Value implements TryFrom<Value>), so we could go slower.

The API functions

Nvim Options:

Those handle nvim options. Maybe make an NvimOption enum and work with that? Types are a closed set (String, Bool, Number - what is that exactly?), so it shouldn't be hard.

    buffer.get_option(
      &self, name: &str
    ) -> Result<Value, Box<CallError>>

    buffer.set_option(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    window.get_option(
      &self, name: &str
    ) -> Result<Value, Box<CallError>>

    window.set_option(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    neovim.set_option(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    neovim.get_option(
      &self, name: &str
    ) -> Result<Value, Box<CallError>>

    // This is actually just a bool. Could also take a UiOption
    neovim.ui_set_option(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

Variables:

Those deal with variables in neovim. Can be more types than options. Partially solvable by just using generics nicely, but getting heterogenous things back might just be a case for point 4 above.

    buffer.get_var(
      &self,
      name: &str
    ) -> Result<Value, Box<CallError>>

    buffer.set_var(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    window.get_var(
      &self,
      name: &str
    ) -> Result<Value, Box<CallError>>

    window.set_var(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    tabpage.get_var(
      &self, name: &str
    ) -> Result<Value, Box<CallError>>

    tabpage.set_var(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    neovim.get_var(
    &self, name: &str
    ) -> Result<Value, Box<CallError>>

    neovim.set_var(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

    neovim.get_vvar(
    &self, name: &str
    ) -> Result<Value, Box<CallError>>

    neovim.set_vvar(
      &self, name: &str, value: Value,
    ) -> Result<(), Box<CallError>>

Fns with Values we're receiving

Receving Values means we need to parse them for the user. Sometimes easy, somtimes borderline impossible. Problems are heterogeneous Dictionaries, and deeply nested things. Might be solvable with custom types in several cases, but might be too hard or complex in others.

    // Returns something very complex, we might just skip this fn
    neovim.parse_expression(
      &self, expr: &str, flags: &str, highlight: bool,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    // List of Dicts, keys are strings, values are heterogenous
    neovim.list_uis(
      &self
    ) -> Result<Vec<Value>, Box<CallError>>

    // List of i64
    neovim.get_proc_children(
      &self, pid: i64,
    ) -> Result<Vec<Value>, Box<CallError>>

    // Map of process properties(??) or Nil
    neovim.get_proc(
      &self,
      pid: i64
    ) -> Result<Value, Box<CallError>>

    // Dict names -> ids
    neovim.get_namespaces(
      &self,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    // Dicts names -> 24bit Values
    neovim.get_color_map(
      &self,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    // Dictionary { "mode": String, "blocking": Boolean }
    neovim.get_mode(
      &self
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    // 2-tuple channelid, api map
    neovim.get_api_info(
      &self
    ) -> Result<Vec<Value>, Box<CallError>>

    neovim.get_chan_info(
      &self, chan: i64,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    // Array of dictionaries from get_chan_info
    neovim.list_chans(
      &self
    ) -> Result<Vec<Value>, Box<CallError>>

Fns with Values we're sending

Sending Values means we have to encode them for the user. Sounds easy, but unless we allow functions with a gazillion single arguments we probably needs predefined structures the user needs to pass in. Options are also often heterogenous dictionaries, and might change when neovim evolves.

    neovim.set_client_info(
      &self, name: &str, version: Vec<(Value, Value)>, typ: &str, methods: Vec<(Value, Value)>, attributes: Vec<(Value, Value)>,
    ) -> Result<(), Box<CallError>>

    neovim.select_popupmenu_item(
      &self, item: i64, insert: bool, finish: bool, opts: Vec<(Value, Value)>,
    ) -> Result<(), Box<CallError>>

    neovim.open_win(
      &self, buffer: &Buffer<W>, enter: bool, config: Vec<(Value, Value)>,
    ) -> Result<Window<W>, Box<CallError>>

    buffer.attach(
      &self, send_buffer: bool, opts: Vec<(Value, Value)>,
    ) -> Result<bool, Box<CallError>>

Keymap, extmarks, virtualtext, window config, highlights

Those are kind of similar in that we might need to introduce structs to pass in or to return, but they exist in nice groups which could be tackled one by one.

    neovim.get_keymap(
      &self, mode: &str,
    ) -> Result<Vec<Vec<(Value, Value)>>, Box<CallError>>

    neovim.set_keymap(
      &self, mode: &str, lhs: &str, rhs: &str, opts: Vec<(Value, Value)>,
    ) -> Result<(), Box<CallError>>

    buffer.get_keymap(
      &self, mode: &str,
    ) -> Result<Vec<Vec<(Value, Value)>>, Box<CallError>>

    buffer.set_keymap(
      &self, mode: &str, lhs: &str, rhs: &str, opts: Vec<(Value, Value)>,
    ) -> Result<(), Box<CallError>>

    buffer.get_extmarks(
      &self, ns_id: i64, start: Value, end: Value, opts: Vec<(Value, Value)>,
    ) -> Result<Vec<Value>, Box<CallError>>

    buffer.set_extmark(
      &self, ns_id: i64, id: i64, line: i64, col: i64, opts: Vec<(Value, Value)>,
    ) -> Result<i64, Box<CallError>>

    buffer.set_virtual_text(
      &self, ns_id: i64, line: i64, chunks: Vec<Value>, opts: Vec<(Value, Value)>,
    ) -> Result<i64, Box<CallError>>

    buffer.get_virtual_text(
      &self, lnum: i64,
    ) -> Result<Vec<Value>, Box<CallError>>

    window.set_config(
      &self, config: Vec<(Value, Value)>,
    ) -> Result<(), Box<CallError>>

    window.get_config(
      &self,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    neovim.get_hl_by_name(
      &self, name: &str, rgb: bool,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    neovim.get_hl_by_id(
      &self, hl_id: i64, rgb: bool,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

Complex things that might return very different things

Basically, like variables. This time though, there's lua and viml to consider.

    neovim.call_atomic(
      &self, calls: Vec<Value>,
    ) -> Result<Vec<Value>, Box<CallError>>

    neovim.eval(
    &self, expr: &str
    ) -> Result<Value, Box<CallError>>

    neovim.execute_lua(
      &self, code: &str, args: Vec<Value>,
    ) -> Result<Value, Box<CallError>>

    neovim.exec_lua(
      &self, code: &str, args: Vec<Value>,
    ) -> Result<Value, Box<CallError>>

    neovim.call_function(
      &self, fname: &str, args: Vec<Value>,
    ) -> Result<Value, Box<CallError>>

    neovim.call_dict_function(
      &self, dict: Value, fname: &str, args: Vec<Value>,
    ) -> Result<Value, Box<CallError>>

Fns that send & receive Values

Just because I couldn't find another category. Might just be we need to define the right structs?

    neovim.get_context(
      &self, opts: Vec<(Value, Value)>,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

    neovim.load_context(
      &self, dict: Vec<(Value, Value)>,
    ) -> Result<Value, Box<CallError>>

    neovim.get_commands(
      &self, opts: Vec<(Value, Value)>,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>


    buffer.get_commands(
      &self, opts: Vec<(Value, Value)>,
    ) -> Result<Vec<(Value, Value)>, Box<CallError>>

cc @Kethku @unrealhoang

[smolck]

This is something I'd really like to see for the UI events specifically, as every Rust GUI (including one I'm working on myself) has to implement the UI event deserialization stuff itself: for example here in GNvim, and here in Neovide. As you can tell from those files, it's a lot of extra boilerplate, that is largely duplicated for every project interacting with the UI events.

I'm interested in taking this on myself, specifically going off of the work in #19, but before I do so I'd like to verify that's the route you want to take @KillTheMule, with adding serde as a dependency etc.? If you're still on the fence about it (or even regardless), perhaps a serde or strongly-typed or similar crate feature could be added which is required for access to the typed API?

In any case, please let me know what you're thinking for this and hopefully I can get an MVP PR up soon-ish (probably) going off of #19 which defines the UI event structs etc. so users of this library no longer need all that extra boilerplate.

[Kethku]

@smolck needless to say, I'm very interested in this becoming cleaner. Let me know if/where you would like help

[KillTheMule]

Hey, sounds very cool. I've made up my mind about serde, I'm open to add it as a dependency for sure now (I wasn't really on the fence, I just didn't fully understand what it brings on the table, and since the author of #19 said he lost interest, I didn't start to ask questions. I think I understand now :)). Doesn't need to be optional imho, seeing that at least 2 UI authors are ok with it ;)

I'd have had quite some questions about #19 which I didn't ask, so I hope you're open to answering a bit when you put up the PR :) Maybe you could sort of make it a sequence of commits that lets me follow along a bit, because I haven't yet worked with serde and might be slow on the uptake :D

[smolck]

Hey, sounds very cool. I've made up my mind about serde, I'm open to add it as a dependency for sure now (I wasn't really on the fence, I just didn't fully understand what it brings on the table, and since the author of #19 said he lost interest, I didn't start to ask questions. I think I understand now :)). Doesn't need to be optional imho, seeing that at least 2 UI authors are ok with it ;)

Cool, sounds good!

I'd have had quite some questions about #19 which I didn't ask, so I hope you're open to answering a bit when you put up the PR :)

Any questions worth asking before I get started on something? Just so I don't go in a direction that'll have to be changed anyways.

[KillTheMule]

I'll put up inline comments. You don't need to answer them, unless you want to keep that part of the code (even then, maybe doing it in the new PR might be better). Most won't even be important, I think, but I'll just throw them out as they come ;)

[smolck]

even then, maybe doing it in the new PR might be better

Yeah my plan is to do a separate PR with the commits from that branch, and then go from there.

One thing I'm unsure of though atm is how e.g. handle_notify in Handler would work without exposing rmpv::Value to the user. The general APIs make sense, but I'm not sure about this part of the library. Currently, the signature for handle_notify looks like:

    async fn handle_notify(
        &self,
        name: String,
        args: Vec<Value>,
        _nvim: Neovim<...>,
    ) { ... }

How would that look without exposing rmpv::Value? Ultimately you still have to have some enum to represent all the possible types that could be passed into that, no? Now my plan here would be to have it take some enum NotifyArgs for args instead of args: Vec<Value>, where NotifyArgs is defined something like this:

enum NotifyArgs {
    UiEvents(Vec<UiEvent>),
    SomeOtherTbdName(Vec<Value>),
}

But that still exposes Vec<Value>, and seemingly by necessity, because how else would one handle the different possible values that could be passed into an rpcnotify(...) call from the viml/lua side? Writing our own Value thing doesn't seem right, we already have rmpv::Value. So I don't really see how we could do this cleanly without exposing rmpv::Value?

[smolck]

Also any objections to rewriting the bindings generator with Lua instead of Python? The benefit being (a) I think I can make it cleaner, so no external deps or the kinda weird jinja template stuff; and (b) this would also mean it can be run directly from nvim (open the file, :luafile %).

If you're wondering how it might look, at least in part, I did this for my Swift nvim api library: https://github.com/smolck/NvimApi.swift/blob/master/Scripts/api_gen.lua (it's not the best, I'd see about cleaning it up for use here, and obviously it'd need to spit out Rust code, but the general idea is there).

No worries if you think it's better to keep what we have, just figured I'd throw this out there, since I'll likely be touching the generation anyways (since the API will need to be generated differently if we stop exposing rmpv::Value to users of the library).

[KillTheMule]

As I said, I don't have experience with this either, but in my head we would take something like T: CanBeDeserialized instead of Vec<Value>, and if the implementer expects, e.g., a string here, they could do let s: String = args.deserialize().unwrap();. Maybe handle_notify needs a return value like Result<(), Error> so implementers don't need to unwrap, but I'm unclear on what to actually do with the error then...

(e) Ok, this needs much more thought I guess, can't really put a generic there... maybe? But maybe it's not even the right idea to remove Value here, but just do so for the API.

[KillTheMule]

Also any objections to rewriting the bindings generator with Lua instead of Python?

I love lua and don't like python very much, so... no :) I've already started to have a hand-written part of the API and a autogenerated, I guess that's a nice way forward to progressively provide more types for the API.

[smolck]

As I said, I don't have experience with this either, but in my head we would take something like T: CanBeDeserialized instead of Vec<Value>, and if the implementer expects, e.g., a string here, they could do let s: String = args.deserialize().unwrap();.

Ohhhh okay, yeah, that should work, where CanBeDeserialized is serde's Deserialize trait. Not sure how I feel about having to explicitly deserialize() things, but that's definitely better than the current interface, so it's a step in the right direction regardless.

(e) Ok, this needs much more thought I guess, can't really put a generic there... maybe? But maybe it's not even the right idea to remove Value here, but just do so for the API.

No you should be able to? No reason you can't have the signature be:

async fn handle_notify<T: Deserialize>(&self, name: String, args: T, nvim: Neovim<...>) { ... }

as you said, right?

Maybe handle_notify needs a return value like Result<(), Error> so implementers don't need to unwrap, but I'm unclear on what to actually do with the error then...

Could err_write to nvim, or let the implementor decide how that's handled; could also just make any errors returned from it result in a panic (with a nice error message I guess), since that's what would be being done anyways probably (with .unwrap()). If we go this route though, I'd lean towards letting the implementor decide how it's handled. But ultimately I think it might be best to just keep it explicit with no return, and then if an implementor really hates .unwrap()s they can just define a separate function to call into that returns a Result.

[KillTheMule]

No reason you can't have the signature be:

I really need to go to bed, but... I don't see how the handler loop would choose which one to call. It get's message that contains the params, and we can hardly have a channel for every possible T or so.

Could err_write to nvim

No, that sound wrong, that should be an implementor choice. Maybe we need an associated error type, and then a handle_handler_error function as part of the Handler trait.

implementor really hates .unwrap()

That's a boat I feel myself being in :D But they could always set up a channel and send errors along I guess.

[oberblastmeister]

The type signature can be generic because we can use Value internally. Value implements Serialize and Deserialize when with-serde feature is enabled for rmpv.

[oberblastmeister]

Also if you don't want to explicitly do deserialize and idea is to use associated types like this

trait Handler {
    type RequestArg: DeserializeOwned;
    type NotifyArg: DeserializeOwned;
}

[oberblastmeister]

But that still exposes Vec, and seemingly by necessity, because how else would one handle the different possible values that could be passed into an rpcnotify(...) call from the viml/lua side?

Shouldn't we just panic? All messages should be correct and if they are not they will fail to deserialize, and we can panic.