I just completed my first decent-sized Kotlin project at GitHub - cretz/asmble: Compile WebAssembly to JVM and other WASM tools. Code feedback/review very welcome.
During development, I jotted down some things that annoyed me as I wrote it and I have collected them in this gist. Granted most are just my opinions and shouldn’t change the language, I just figured some feedback might be valuable. I hope the Kotlin devs take a quick peek even if they disagree w/ most.
Thanks for a great job. It is a great and exhaustive list. I’m sorry for narrowing this discussion to one specific item, but I have one specific clarification question. When you say in item 53 that “Either needs to be in the stdlib” do you actually mean a use-case for it as a union type, that is a disjoint unit of arbitrary types (lie String and Int and in Scala’s doc on Either) or your use-case is it to represent a pair of Exception and some result (like Scala’s Try)?
Thanks a lot for your feedback! I’ve commented on the gist with responses to many of your issues; my colleagues will probably leave a few more comments.
My recommendation is for Either<L, R>. I made a tiny one here. If you made it “right-biased” then a Try could easily be a typealias Try<T> = Either<Throwable, T> I dunno. But yes, my use case was a union. Of course as an alterative, I would love actual union types, e.g. String|Int like what Scala.js and Ceylon do, but that’s a bit radical.
Thanks a lot for explanation! I’ve looked through the code, too. Let me make some comments, if you excuse me.
I’ve looked at your usages of Either. You are indeed using it as a union type on your own types. It does seem from the cursory reading of your code that it can be slightly improved. Let us take one example where you return Either<Node.Import, Node.Global> from globalAtIndex. In this particular example you can define an interface ImportOrGlobal that is implemented by both Node.Import and Node.Global and return that one instead of Either. It looks like the immediate uses for this function can be made slightly simpler and easier to read after this change. The same seems true for other uses of Either in your project.
Also, I’ve looked at some of the usages of Triple in your code base. Many of them would defenitly benefit from one-line declaration of the data class. For example, you could declare data class FuncSig(val nameMap: NameMap, val exprsUsed: Int, val sig: List<Node.Type.Func>) right before toFuncSig function. Not only this will immensly improve readability of the toFuncSig function declaration itself, but this would also reduce the need to redeclare (by destructuring) the names of the actual components at every use-site of the corresponding function. The last, but not the least, it will make it easier to refactor the code in the future (e.g. adding a new returned item will not force all prior uses to be changed).
I do suggest you to try it, if you care. This will make your code more idiomatic from Kotlin standpoint.
I didn’t want an ImportOrGlobal because you have to go to the definition site to add an interface and it’s not my AST’s fault that I can’t represent a union in another package. Also with your interface idea I have now just lost my exhaustiveness checks in my when statements and now always have to add an else because there are no sealed interfaces. For other things I do have it like for parse results that represent a success or error. Arguably I could use exceptions here, but I choose to represent it as a possible result state. I could go through the Kotlin/IntelliJ code bases and say that that all uses of Pair should be changed to use a ThisAndThat class. We recognize the value of shortcuts, whether it’s composing two types or unioning them.
I agree about uses of Triple. I take the shortcut in these cases. I take the shortcut in a lot of cases. You’ll see I declare a data class in a lot of cases and I am aware of the choice. Quite often I choose pair or triple for return value tuples that I expect immediate destructuring by the caller. Another place these types of generic tuples are valuable is in local code when doing something like a fold where you keep immutable state and return one or the other, one and the other, etc when you have multiple stateful representations at once. Granted, you can make your points for Pair and Triple in just about every place they are used in anyone’s code.
Good point about exaustivness checks. Thanks.