Extensibility for Command

[weissi]

Commands need to be extensible so they can't be a big enum.

My proposal

public struct RenameCommand {
    public var from: Mailbox
    public var to: Mailbox
    // ...
}
public struct SelectCommand {
    public var mailbox: Mailbox
    // ...
}
public struct Command {
    enum Commands {
        case rename(RenameCommand)
        case select(SelectCommand)
    }
    var command: Commands
    public static func rename(_ command: RenameCommand) -> Command {
        return Command(command: .rename(command))
    }
    public var asRename: RenameCommand? {
        switch self.command {
        case .rename(let command):
            return command
        default:
            return nil
        }
    }
    public var asSelect: SelectCommand? {
        switch self.command {
        case .select(let command):
            return command
        default:
            return nil
        }
    }
    public static func select(_ command: SelectCommand) -> Command {
        return Command(command: .select(command))
    }
}
func switchCommand(_ command: Command) {
    if let rename = command.asRename {
        // handle rename
        print(rename)
    } else if let select = command.asSelect {
        // handle select
        print(select)
    } else {
        // send BAD command
    }
}

[danieleggert]

The problem with this approach is when you implement a server though. You’ll basically have to do a giant

    if let rename = command.asRename {
        // handle rename
        print(rename)
    } else if let select = command.asSelect {
        // handle select
        print(select)
    } else {
        // send BAD command
    }

as you indicated — which is not nice and smells like it’s not particularly performant.

I still think that X-commands are so exotic that we can keep the current approach.

[mdiep]

I agree with @danieleggert that pattern matching is useful when handling commands in the context of a server. This also applies to testing, where you might write a fake server or want to extract information from the command. Enums are more natural and concise.

The existing design seems like it has extensibility in the right place—in Command.xcommand. Is there a specific type of extensibility that you foresee that isn't handled by that?

[weissi]

@danieleggert / @mdiep extensibility is necessary to add stuff like the IDLE command, IIRC that came in an extension...

Regarding performance, I’m pretty sure we can come up with a spelling that gets us a jump table. It’s unlikely (not impossible though, I’ll check) that LLVM realises that it could turn those tons of ifs into a jump table (over the tag bits of the underlying (private) enum). I have a few ideas that I’ll explore trying to get a jump table but I need some time because it all requires SIL/assembly reading because Swift doesn’t have any guarantees that allow you to use guaranteed optimisations.

Another option of course is to make a “matcher” protocol that has one function per command that you want to match and requires a special one for “unknown command”. To add a new command, we’d provide a default implementation that goes to the “unknown command”.

If only swift had open enums 😅

[weissi]

(It does, but only in library evolution mode which isn’t what SwiftPM uses and also it comes with other very serious drawbacks)

[weissi]

@mdiep / @danieleggert alternative is to keep the enum but "hack it open"

public /* @open */ enum Command {
    case rename(...)
    case select(...)
    @available(*, deprecated, message: "this is an open enum")
    case _doNotPatternMatchThisCaseAndUseADefaultInsteadThisEnumIsOpen🚯(NIONever)
}

[Davidde94]

I've been thinking about this a little over the last couple of days. What about being able to give the parser additional parsing functions.

E.g. we could implement some function

Parser.addCustomParser(_ func: (ByteBuffer, StackTracker) -> Void) {
    self.customParsers.append(func)
}

// then, inside the parser itself
func parseCommand(buffer: inout ByteBuffer, tracker: StackTracker) throws {
    do {
        // try our normal parsers
    } catch let error as ParserError {
        for parser in self.customParsers {
            try parser(&buffer, tracker)
        }
    }
}

(Note that the above is pseudo-ish code and will not compile)

[weissi]

@Davidde94 there are two issues:

1. Custom commands added by users. Those custom commands wouldn't be in the enum, we'd support them either with user events or by having an extra enum case where you'd have a way those custom commands
2. New command we introduce in NIOIMAP that need to have their own enum cases

Your proposal could be part of a solution to (1) but this ticket is about (2).

[Davidde94]

I've been thinking a little about this recently. I don't think it matters too much that it's an enum. If we add support for a new command then we can just bump the major version. Adding support for an entirely new set of RFCs seems like good enough cause to push out a new major IMO.

[weissi]

@Davidde94 we can't do that. "An entirely new RFC" sounds like there are only 1 or 2. But there are many so I don't think this is reasonable. It's very common to the IMAP protocol that new commands/responses get added so we need to support that. We can just add a case _doNotMatchThisPleaseAddADefaultToMatchNewUnknownCommands.

[danieleggert]

I’d argue, though, that there a few RFCs that add new commands. Most of them just change how existing commands work or what responses contain, etc.

Before we move ahead here, I think it would be good to look at any RFCs that aren’t supported, yet. Then see which ones are likely to make sense, and then evaluate what changes would be needed to support them.

I’d assume that for most of them we would have to push out a new major version — not due to commands, but due to how they’d change arguments to existing commands and/or responses.

#286 was our initial “wish list”. David initially had a list of “all” RFCs here https://github.com/apple/swift-nio-imap/blob/bc1e84b3b8ada9bfbcdd098a77a33e565ac0efea/README.md

[Davidde94]

@weissi your solution is a hack though. If we're concerned about pushing too many majors versions we can always batch RFCs. @danieleggert good idea, I can take care of this.

Also worth noting that this doesn't only apply to commands, but also responses.

[weissi]

@Davidde94 yes, it's a hack until https://bugs.swift.org/browse/SR-12666 is resolved.