Public review of the Standard Library APIs [Closed]

Kotlin (after Java) lacks functions for printing bytes as hexadecimals. The best there is is Integer.toHexString(aByte.toInt()). I think we need Byte.toHexString(), Int.toHexString(), ByteArray.toHexString(), and some others. Python has a nice set of such functions.

And maybe an md5(ByteArray): ByteArray would be nice too, since it is often used on its own (e.i. for salting)

3 Likes

Sorry if someone already mentioned this.

Personal preference, but I think the apply, run, and let extensions should be infix functions.

val map = hashMapOf<String, String>().apply { 
    put("Key", Value")
}

Looks more confusing and out of place than this…

val map = hashMapOf<String, String>() apply {
    put("Key", Value")
}

… because the former looks like (to the uninitiated) a type specific extension (which it is), where the latter looks like a language idiom. Viewing it as a language idiom may more likely lead an interested user toward the language reference for guidance (which is the correct place) rather than the type. Assuming of course they aren’t using IDEA and can’t just ctrl+click .apply. :smile:

Of course, like I said, this is just a personal preference, but changing it to an infix wouldn’t break any existing code and leave it up the user.

The current approach, namely to stay in line with Java collection APIs by throwing exceptions, and to provide orNull variants (e.g. firstOrNull()) where appropriate, makes sense to me. Are there particular methods for which you are missing an orNull variant?

Iterable.forEachIndexed: The parameter list of (the action passed to) forEachIndexed should naturally extend the parameter list of forEach. Because it’s forEachIndexed rather than indexedForEach, it should be forEachIndexed { elem, idx -> ... } rather than forEachIndexed { idx, elem -> ... }. The current order is counter-intuitive.

Same for other similar methods (e.g. mapIndexed).

1 Like

In general, I’m a happy user of the standard library and haven’t encountered any recent gotchas that I recall.

I’ve just had a quick read through the sources and here’s what stood out.

Edit: remove X from the links to follow
Overloaded methods accepting a string Charset

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/collections/ArraysJVM.kt#L68

Seems unnecessary for the rare occasions you’d use it and then Charset.forName() is available.

Usage of raw time units

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/concurrent/Timer.kt#L12

Not a big deal, but some kind of units would be better (although I understand you’re targeting 1.6).

Inconsistent naming of block-like structures (block, body, operation, action)

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/concurrent/Thread.kt#L25

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/io/FileReadWrite.kt#L178

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/util/Integers.kt#L10

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/io/ReadWrite.kt#L25

I’d prefer a consistent name where possible.

Leaking of internals

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/io/Constants.kt#L9

Amusing Australian slang

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/kotlin/io/files/FilePathComponents.kt#L82

“Is rooted” is pretty much equivalent to “is f*cked” in Australia. I’m Australian and would like to keep it regardless. :wink:

Same functionality via different methods

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/generated/_Collections.kt#L153

I think there’s other cases of this too. I’d strongly prefer one way of doing things for readability.

References to java.util

Xhttps://github.com/JetBrains/kotlin/blob/01095bc65257f399a97768e1f0f9ae7d31144a23/libraries/stdlib/src/generated/_Collections.kt#L1048

I’m a bit uneasy about stdlib things returning anything from java.util. It seems prudent to only return kotlin.* interfaces to allow other backends not to be tainted by java packages. There’s many cases of this e.g. arrayListOf()

Evolution
In general I think you should feel free to evolve the standard library after 1.0 as long as you maintain backwards compatibility. Even dropping things should be fine as you have the Deprecated.HIDDEN mechanism.

Documentation
Some overview docs are sorely needed for the Ordering functions and how they are chained together. Likewise for Ranges and Regexes. As far as I can tell its all perfectly functional, but it’s not obvious how to use them.

Cheers,
Andrew

1 Like

FileTreeWalk: Often I don’t want to include the start directory itself in the walk, but there appears to be no clean way to achieve this. The constructor’s filter parameter excludes directories recursively, and filtering the returned Sequence has no effect on enter and leave.

KDoc neither mentions that start itself is included in the walk, nor that the filter passed to the constructor filters directories recursively (this is only mentioned for the treeFilter method). The filter parameter should be renamed to treeFilter, to match the corresponding method name like the other parameters do.

Update: The fact that a FileTreeWalk sequence can only be iterated a single time, and will be empty from then on (because iterator returns the same iterator each time), came as a big surprise to me. If this is intended, it needs to be documented. If multiple iterations aren’t supported, I’d expect subsequent iterations to fail.

T.apply: When I read apply, I think of function application. configure would be a better name, and would convey that this method is all about side effects.

2 Likes

Arrays: Kotlin doesn’t seem to provide a way to compare array contents, leaving java.util.Arrays.(deep)Equals as the only option. Also, there is no assertArrayEquals.

Any chance for equals to compare deep array contents? Would be sad if Java’s mistake had to be repeated. Same for toString.

PS: I couldn’t find any docs whatsoever on how Kotlin compares arrays.

I think that it is a pity that Kotlin standard library and syntax does not have better support for half open ranges (i,e start<= i < end), since they are so common, considering also they exist in many other languages (Swift, Ruby, Python, Go (slices)), But I guess this is something that can added post 1.0. The standard library has only ClosedRange, so I guess that introduction of OpenRange is s planned for the future

It compares arrays in an identical way to Java (by reference). I’ve gone around this one before and tried to convince Andrey to change it, but IIRC there were some complicated edge cases and the straightforward “== means the .equals method” consistency won, even though it’s a technical rather than semantic consistency. It’s regrettable for sure because “a == b” with two arrays basically never does what you expect.

If we had value types it could be patched over. I have an OpaqueBytes class in my code that fixes equals/hashcode/tostring for byte arrays but it’s obviously an inferior solution.

1 Like

The lazy function’s signature fun <T> lazy(initializer: () -> T): Lazy<T> does not allow to use the instance reference while defining an extension property. See the problem here: http://stackoverflow.com/questions/34391255/this-reference-in-a-lazy-initializer-of-kotlin-extension-property/34393382#34393382

I suggest changing the implementation to save this reference and pass it to the initializing body: fun <T> lazy(initializer: T.() -> T): Lazy<T>

Classes are going to be moved in RC, and functions are moved in beta4. There will be 4 packages where functions and classes would land in: kotlin.collections, kotlin.ranges, kotlin.sequences and kotlin.text.

Agree, these methods may return more specific type.

Most of these issues are already fixed in beta4. We’re going to make FileTreeWalk constructor internal, so that the construction would be performed only with fluent methods. treeFilter is to be removed entirely, and recursive directory filtering is performed in onEnter callback.

Single time iterability is also fixed in beta4, now each iterator of FileTreeWalk sequence has its own independent state.

To exclude starting directory you can filter it out with filter or filterNot sequence operations.

Could binary operations be added on all types to which they apply? Right now I often do:

val x: Byte = 1.toByte()
val y = x.toInt() and 1
1 Like

Closeable.use: I’m missing AutoCloseable.use for JDK 1.7+.

Update: We are planning to close this call on Jan 11, 2016

The “Any.to” extension method is perhaps not something that would be great to support forever. Although it’s nice to have in mapOf other times it’s just sort of junk hanging around.

The stdlib is missing methods to instantiate MutableList, MutableMap and MutableSet. This is especially problematic since the new overload resolution algorithm doesn’t allow calling the forEach extension function on an ArrayList instance. I also created https://youtrack.jetbrains.com/issue/KT-10531 to track this.

2 Likes

#consistent collection operations
As raised in the Slack standard library channel I have some issues with the somewhat inconsistent behaviour between collection functions like map, filter, etc…

All functions related to Iterables are returning snapshots where all functions related to Sequences are lazily evaluated. This makes it very hard to reason about what code is doing. Take for instance the following example code:

fun main(args: Array<String>) {
    val xs = listOf(1,2,3)
    val mapped = xs.map(plusOne)
    
    mapped.forEach {  }
    mapped.forEach {  }
}

val plusOne : (Int) -> Int = {i -> println("mapping [$i]"); i + 1}

By merely changing listOf(1,2,3) to listOf(1,2,3).asSequence() the behaviour changes from mapping once to mapping per call/iteration.

##proposition
So I prefer all standard library collection functions to be lazily evaluated so everything becomes consistent and therefor easier to reason about. If you do require a snapshot a simple library function can be introduced like Iterable/Sequence.toList().

##disadvantages

  • changes behaviour of current code bases without the possibility to properly emit (compiler) warnings
  • lazy evals can be prone to errors to inexperienced developers, implicitly making db calls or keeping large amounts of memory alive in closures.

##advantage

  • consistent API
  • if there is any change to this, now is the time, can’t be done later

##other remarks
As pointed out by @natpryce in the Slack channel it’s not that clear anymore why there should be an Iterable and a Sequence. I thought there was some sound reason for this but I have already forgotten.

1 Like

Just looked at with and agree it seems totally worthless. Sure seems like it should be an extension function instead of passing receiver as a parameter. In reality it is like map function. As an extension function it could be renamed apply and it is simply apply that returns a new value or possibly called transform.

let is just like run but instead of an extension function it takes a function that accepts the target as first parameter. So it seems to me it should also just be called run.

But in reality all of these methods (if with were also an extension function) could have the same name. The only difference being what type of function is passed, but that would probably cause ambiguity with lambdas.