Public review of the Standard Library APIs [Closed]

By the way, you might want to think about scrapping the idea of ever doing a Kotlin 2.0 - rather, if you build up a big enough collection of changes that can’t be done with an acceptable compatibility cost, invent a new brand name and new website. Then implement a Kotlin-to-SparklyLang converter as you did with Java-to-Kotlin to give people a migration path … but leave Kotlin still in existence so the open source community (or you) can still do new releases with bug fixes or further backwards-compatible improvements.

The reasons being:

  1. Kotlin can be a great competitor to Java or C# (commercially supported, not too radical, good IDE support etc). But not if people are immediately told that they’re writing code that will eventually stop compiling (but not told when).
  2. Experience from Python shows that even apparently “easy” backwards compat breaks can fracture the community forever and cause a de-facto fork anyway, so at that point having a separate brand name just simplifies things anyway.

It will also encourage you to evaluate the costs of doing that better. If you have to pick a new brand name, it will mean you have to advertise the project and make the case for why Kotlin users should switch to the new language. That in turn means you have to be sure yourselves … keeping the same brand name means the costs might seem lower because “oh it’s just another upgrade to what we already have”. A little bit of friction is no bad thing, here.

In my opinion there too much functions in the kotlin package. I would prefer some grouping (e. g. collections related functions, string related functions and so on).

there is a “Java Puzzlers” behaviour in case of
file.deleteRecursively()

it returns:

  1. true if can delete smth
  2. false if can’t delete smth
  3. and the puzzler → false when there is no such folder structure

so if one needs to check if deletion didn’t happen,
because of some blocking, or it was partial, or was already deleted:
the code would be:
!file.exists() || !file.deleteRecursively()

Thanks everyone for feedback provided so far. Do not stop. While we’re analyzing it, I’ll post here some things we have already spotted during our review and plan to fix before release.

###Operations on maps and null values

We have several extensions for Map and MutableMap in the standard library, namely getOrElse, getOrPut, getOrImplicitDefault. They all try to get a value mapped to the specified key, or execute Or-branch in case if the key is missing from the Map.

This behavior differs from the one of similar functions like putIfAbsent or computeIfAbsent in java.util.concurrent.ConcurrentMap or java.util.Map from JDK8. These functions treat a key mapped to a null value the same way as a key missing in the map.

Provided these JDK8 operations would be eventually available for kotlin.Map it would not only lead to inconsistency in null handling, but also make us unable to use these function to implement our own extensions. One notable example is getOrPut for ConcurrentMap which we were unable neither to implement according to its contract in a concurrent-safe manner, nor to provide an overload of getOrPut for a subset of ConcurrentMaps with non-nullable values. That time we had to introduce an instantly deprecated overload for ConcurrentMaps and provide an extension with new name concurrentGetOrPut.

As a result we’re tending now to change the contract of our operations to treat nulls mapped to keys the same way as if those keys were missing. For example getOrElse would call defaultValue function when it encounter null value, and getOrPut would put new value over the null value.
Note that the behavior would change only for the maps that could contain nullable values.

Note: I updated the post to note that reflection APIs are not part of the standard library, and we’ll probably change something there.

We keep having a hard time with T.run vs. with vs. let. These three “global” functions sound very different, but actually only differ in “cosmetics”. Almost always any of them can be used, and it’s unclear which to prefer. This regularly leads to debates in code reviews. Furthermore, it’s not intuitive why it’s with(foo) vs. foo.run. (In fact, this seems to be the only difference between the two!)

On the other hand, we sometimes miss the ability to access the receiver of apply as a function argument. For example, val foo = Foo().apply { configure(it) } feels more natural than val foo = Foo().apply { configure(this) }. We use apply more often than any of the other methods.

I hope there is a way to unify these methods, or make their similarity and distinctions more apparent, that removes a lot of this guesswork. I made one proposal in Let vs. with vs. run - #2 by ilya.gorbunov. Another idea that comes to mind is to remove one of T.run and with, although this still leaves with vs. let.

If no other solution is found, damage could be reduced by having KDoc explain the rationale and intended use cases for each method, rather than just paraphrasing the code.

3 Likes

@abreslav Would that be possible to update the docs to beta 4 on the weekend? The API is changing rapidly, and the docs are coming outdated. Maybe the solution would be to regenerate them every night as a part of CI?

+1. “with” should die

apply - is sort of “object initializer” from C#.
C#:

    var o = new MyObject {
      Prop1 = "Value1",
      Prop2 = "Value2"
    }

kotlin:

    var o = MyObject().apply {
       prop1 = "Value1"
       prop2 = "Value1"
    }

I understand (and wonder if configure would have been a better name). Just saying that if some of the initialization is factored out into a separate method, separateMethod(this) looks a bit odd.

Extension method File.inputStream should have return type FileInputStream rather than InputStream.

Extension method File.outputStream should have return type FileOutputStream rather than OutputStream.

1 Like

I would like the extension methods defined for collections and iterables to be made type safe by returning nullable types rather than throwing exceptions. For example, first() throws if there is no first element; it should instead return T?.

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