symmetryinvestments / imap-d

D library for IMAP (JMAP is a work-in-progress but the basics work)
14 stars 10 forks source link

Session API is a bit verbose. #46

Closed otrho closed 3 years ago

otrho commented 3 years ago

The API for this library revolves around the imap.Session. It holds the context for an active connection to a server and is needed for the majority of the library methods, which all take a mutable reference to it in case it needs to be updated.

In D this fine and most of the time you'd use UFCS to call e.g., session.select(mailbox). Looks good.

In SIL however, the wrapping will see the ref Session and produce an out parameter for it. The wrapped methods return a pair of an 'out parameter' and a 'return value'. This ends up being quite verbose in practice and we almost never want to bother with the Session out parameter as it's really just an opaque handle. Strictly speaking though we should be taking it and passing it on in subsequent calls.

Ideally I'd like to have the session as an opaque handle which can be passed around, and in the SIL context is essentially immutable. If the Session was in fact a pointer then it would be an opaque immutable handle, but I'm not sure if this is viable or desirable.

Actually, I'm not sure what the best approach to improve this problem is. In essence, I'd like to create the session (per server connection) and pass it around like a handle, and to not have to worry about using the verbose and ugly .returnValue property after each API call.

Laeeth commented 3 years ago

Yes. Previously modifying ref argument was allowed and the returnValue wasn't needed. I didn't have time to change the imap code since then. Feel free to change it to something sane

rmanthorpe commented 3 years ago

I'm really a fan of immutable state so I want to believe we could have it return the modified Session and a return value and it not be a massive pain. I think the only way to do that would be to have some kind of session monad, but I don't think that's the road we want to go down.

Instead I think the opaque pointer plan is a good one. You could either rename Session to SessionImpl and define Session as

struct Session {
    private SessionImpl* session;
    alias session this;
}

and just drop the refs from Session parameters throughout. I had a look to see if you could do this with type maps in SIL but you're in the middle of a few edge cases it wasn't design for so it would need a little (perfectly doable) work to support it. Then you would do something like this:

    @SILname("Session")
    static struct SessionProxy {
        this(<whatever the Session params are> args...) { impl = new Session(args...); }
        @SILignore Session* impl;
    }

    ref A getA(AProxy p) { return *p.impl; }
    alias TMs = ParamMaps!getA;
    handlers.registerType!(TMs, SessionProxy);

then in SIL

> s = Session(args...)
> result = s |> somethingThatMutatesSession(args...) // no need for a returnValue, s is mutated.

And the key thing is it wouldn't need any D code changes to support it.

skoppe commented 3 years ago

Instead I think the opaque pointer plan is a good one.

How is that different from making Session a class?

Another option that might work is to get rid of the ref everywhere (in the functions exposed to sil) and make copies.

For functions you can just mutate the session parameter but for methods that would be:

struct Session {
  auto fun(Args args) {
    auto clone = this;
    // do actual work using the clone
    clone.mutate(args);
    return clone;
  }
}
rmanthorpe commented 3 years ago

How is that different from making Session a class?

It's not. It's pretty much exactly the same as a final class just messier :shrug:. The only difference would be client code wouldn't have to add new calls when then instantiate Session (not a significant concern).

Another option that might work is to get rid of the ref everywhere (in the functions exposed to sil) and make copies. For functions you can just mutate the session parameter

But how do you get the mutated session back in that case?

skoppe commented 3 years ago

But how do you get the mutated session back in that case?

Just return it like with function chaining:

auto setFoo(Session s, Foo f) {
  s.foo = f;
  return s;
}

It is in line with addEntry and replaceEntry function in that they return the modified object.

However, seeing the imap API it does need some adjustments to adopt this. Changing Session to be a class is easier short term.

rmanthorpe commented 3 years ago

I mean on the free functions and the functions of other objects that take and mutate a ref Session but return something else. Those are the problems where you need an extra return value.

skoppe commented 3 years ago

However, seeing the imap API it does need some adjustments to adopt this. Changing Session to be a class is easier short term.

otrho commented 3 years ago

Fixed with #58 .