Public review of the Standard Library APIs [Closed]

We currently have only one place for the docs of the latest public version (which is Beta 3). I see the problem with regard to this review, but it will take some time to set up a place for nightly regenerated docs. We’ll think about it. Thanks

1 Like

Could you please add function for calculating max and min values to Math class using varargs? It will be so suitable!

2 Likes

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