Unused return value checker

[sandwwraith]

This is an issue to discuss Unused return value checker proposal. The text of the proposal is located here.

This proposal depends on the other sub-proposal, Underscore for unused local variables, located here.
Since both proposals are connected, we'll discuss them together.

The goal of the proposals is to enhance the functionality of the existing 'unused expression' diagnostic in the Kotlin compiler.
New diagnostic should be able to report complex expressions whose return values are meaningful (e.g., non-Unit) but are not used.

[kyay10]

You mention that "class or top-level properties" cannot be unnamed. This makes sense for class properties, since one can use an init block. However, there's no init option for the top-level. I would dislike having unnamed top-level properties, but I also dislike that such code:

val unused = run {
  initialiseSomethingStatic()
}

may be needed sometimes. What are your thoughts on this?

[kyay10]

You also mention that "Unnamed variables cannot be delegated". I have a very clear use case for this though: triggering provideDelegate without using the resulting delegate object. An almost-example of this is Gradle sourceSets:

val myCustomSourceSet by creating {
  ...
}

When one won't refer to myCustomSourceSet afterwards, it might be helpful to have:

val _ by creating {
  ...
}

Except that, in the case of Gradle, the name of a property is used. Maybe such properties should have an empty name to signal that they're unused?

[sandwwraith]

@kyay10 I would say that using top-level code for static initialization may be bad practice and lead to hard-to-debug errors because such code is actually executed only when the corresponding file facade class is loaded, which may happen at different points in time. But if I had to do this, my solution would be to wrap init {} into object StaticInits { ... }.

[sandwwraith]

@kyay10 Can you explain what is the use-case for creating a source set, but not referring to it afterwards?

[kyay10]

My bad, what I had in mind was getting I believe because it's used for configuration quite often. Again, in Gradle this doesn't actually make much sense because of the name being significant, but one can imagine a DSL that doesn't use property names but also has some desirable side effect on provideDelegate that one wishes to trigger.

[hrach]

Could you please share the argumentation behind this?

 foo() ?: return // foo() is considered unused because its value is lost after comparison with null.`

The value IS USED to check if it is null or not.

Isn't the rewrite a useless noise?

val _ = foo() ?: return

---

If that would be considered, next thing I'd argue for is to support also functions returning Nothing in the Elvis operator.

I.e. this shouldn't report as well.

foo() ?: error("...")

[sandwwraith]

@hrach This is a place we also have a lot of conversations about within the team. Generally, reasoning is the following:

fun foo(): String? = "a"

fun bar() {
  foo() ?: return
  // Where to get "a"?
}

If foo() has a nullable type, even if we compared it with null, then the value is still lost — we cannot access it later. Intuition and our experiments on Kotlin's compiler code base suggest that the author here probably wanted to write val a = foo() ?: return to access non-null value later. We'll see how this approach will play out when the feature will be available to the public. If it is too problematic, we can always change it.

[hrach]

@sandwwraith let me play devil's advocate role, I'm not sure the things I'm arguing for are the best way, but allow me to share a few arguments:

• the feature is called "unused", check against null is usage, this makes me nervous as I feel that the basic common knowledge is not enough to be sure the code I write won't have to be fixed after highlighting kicks in.
• the verbose desugared version is not/won't be reporting this, or unused variable, would be?

  val bar = foo()
  if (bar == null) return
  // no further usage of bar

  I worry that not counting null checks against usage breaks the mental model people have in mind and are already used for "unused variable" warning
• is this expression an unused return value please? if (foo() == null) return, if not, why? what's different from your example?
• null guarding via early return type/exception is a common pattern; also it is a common pattern NOT to store booleans, but data (aka paidAt)

  order.paidAt() ?: return
  // only paid orders processed now

• maybe my worries are driven by not considering this purely as a behavior of functions; have you considered the same behavior for dynamic properties (with getters)?
• in the interop section, you do not state if those checks would be present on call-like-property expressions that are actually annotated methods in Java, could you specify please?

Anyway, it would be great to read more about the discussion you had internally. :) Thx for responses.

[TWiStErRob]

@sandwwraith Re "Feature modes", I think there's a gap in the mode listing. What happens if a library never re-compiles (or takes unreasonably long, like years) with @MustUseReturnValue?

I think the gap here is that in these circumstances we (as users of Kotlin) should be able to force checks for code that's unannotated. To enable this either there needs to be a facility to externally annotate classes/packages/modules/libraries, or we're in need of a 4th mode: "Enforced", where everything on the classpath is treated as if it was annotated @MustUseReturnValue and potential false positives can be accepted via val _ or @Suppress.

Re naming of new mode: current 3rd mode (full) might be called "Safe" and this above-proposed 4th mode would be the real "Full" mode.

[zarechenskiy]

@hrach Let me offer a slightly different perspective on this problem. This feature helps distinguish pure (side-effect-free) functions from those with side effects at call-sites. Typically, if a function has no side effects and its return value is unused, it's almost certainly a bug. However, if the function does have side effects, omitting the return value is acceptable. Clearer naming might help us:

// return element or null if failed to write
writeToFile(e) ?: return 
writeToFile(e) ?: throw ...

// here, most likely, we just forgot our computation, why did we compute it at all?
computeDistance(x, y) ?: return // (2)

I understand that, even in the second example, one could argue such calls might exist. Yes, but having a separate function like isDistanceValid would make it much clearer.

Functions like those in the first example (such as writeTo...), where the main computation is a side effect and the return value acts as a flag of this computation, should be marked with @IgnorableReturnValue. These functions are quite special and significantly less common than others.

In summary, there are arguments both for and against warning on foo() ?: return. However, we believe it's safer to emit the warning by default, as it often indicates a potential bug or code smell

[zarechenskiy]

Regarding functions and computable properties: it's true that this proposal currently lacks this section for now, and we're working on a generalization.

We'll probably introduce a notion of stable and non-stable values. In short, stable values can be smart-casted, while non-stable ones cannot. An easier way to think about it is that non-stable values include not just functions and computable properties but also properties from other modules, delegated properties, mutable properties, and more.

For stable values, we foresee the following

fun foo(stable: String?) {
  stable ?: return
  ...
  // if "stable" value wasn't used, then it's bug like "unused smart-casted value"
  // but if "stable" was used somehow, then it's fine

For non-stable (functions, computable properties, delegates and so on) it's easier:

fun bar() {
  computableProperty ?: return // warning!

[zarechenskiy]

@TWiStErRob

I think the gap here is that in these circumstances we (as users of Kotlin) should be able to force checks for code that's unannotated. To enable this either there needs to be a facility to externally annotate classes/packages/modules/libraries, or we're in need of a 4th mode: "Enforced", where everything on the classpath is treated as if it was annotated @MustUseReturnValue and potential false positives can be accepted via val _ or @Suppress.

We'll assess the state of the ecosystem and provide some options. From today's perspective, we just might make the wrong decision. We decided not to include the option with external annotations as we've already had a painful experience with external nullability annotations for libraries we don't control. It was a mess, there was always a discrepancy between these annotations and library updates since they had to exist separately. Plus, even a minor addition to the build tooling could introduce unexpected bugs here and there.