Open weissi opened 4 years ago
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.
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?
@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 if
s 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 😅
(It does, but only in library evolution mode which isn’t what SwiftPM uses and also it comes with other very serious drawbacks)
@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)
}
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)
@Davidde94 there are two issues:
enum
, we'd support them either with user events or by having an extra enum
case where you'd have a way those custom commandsenum
casesYour proposal could be part of a solution to (1) but this ticket is about (2).
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.
@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
.
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.
@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.
@Davidde94 yes, it's a hack until https://bugs.swift.org/browse/SR-12666 is resolved.
Commands need to be extensible so they can't be a big
enum
.My proposal