Kotlin
Unused return value checker
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.
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?
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?
@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 { ... }.
@kyay10 Can you explain what is the use-case for creating a source set, but not referring to it afterwards?
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.
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("...")
@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.
@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?
I worry that not counting null checks against usage breaks the mental model people have in mind and are already used for "unused variable" warningval bar = foo() if (bar == null) return // no further usage of bar - 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.
@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.
@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
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!
@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.
@zarechenskiy thank you for the comment.
Is the "stable" property concept related to the Java interop? Or could you please answer that as well?
@hrach If a synthetic property call in Kotlin is desugared into a Java-annotated method with something like @CheckReturnValue, then yes, we'll emit warnings at call sites as well. These properties are not-stable in the sense that they cannot be smart-casted, so the behavior is the same as for computable properties:
fun foo(javaClass: JClass) {
javaClass.prop ?: return // warning!
}
In this example, would B be marked as unused?
fun foo(): Unit =
try {
...
} finally {
B
}
I very often see this pattern, where a non-Unit last expression in a lambda is declared to return the Unit type to avoid the expression being sent to the caller.
Since this pattern is used to explicitly hide the resulting expression, I think it is safe to assume the user intended to ignore it? But maybe not.
I'm unsure how the compiler will handle this case because I do not know if it sees it as “a try...finally of type B which is converted to Unit” or “a try...finally that must return Unit and thus I ignore type B”.
Therefore, we do not plan to address this problem in the current design stage. let, and some other functions will be marked as @IgnorableReturnValue to avoid a large number of false-positive errors.
I understand the situation, the constraints, and I understand the solution you've chosen. Still, abuse of ?.let is one of the main problems I see in existing large Kotlin codebases, due to developers attempting to use it everywhere. One such abuse is using .let to fire side-effects with no meaningful return value, and it would have been great if this proposal could flag such cases.
When this feature becomes stable, state 2 will be the default. Therefore, all Kotlin users would immediately benefit from every library that is correctly annotated without additional configuration.
I think we should probably have a plan for making state 3 the default at some point in the future. Library developers have a lot of things to do, and usually not a lot of time. Having to explicitly opt-in to this feature long-term means that a large amount of libraries probably won't enable it (even if simply by lack of knowledge of its existence) which makes this KEEP much less useful.
When state 2 is the default, in a single codebase, we will very often see a warning for a given usage, and then the same usage but using a function from another library won't cause the warning. Also, in multi-module applications (which I believe are the vast majority of application-level codebases) there won't be a warning if an expression from another module is unused. Such cases will harm the user's trust into the diagnostic.
To me, this marks that state 2 is not enough for a long-term solution. State 2 should be considered a transition state, with a clear deadline. For example, assuming this becomes stable in 2.4, we should announce that state 3 will become the default in 2.6, which leaves a year to users to annotate their values.
Of course, this heightens the risk that once a library is recompiled in full mode, some function somewhere was forbidden and creates warnings even though the return value was very much ignorable. However, this seems impossible to avoid to me, as this KEEP does not contain tooling to help library developers find which parts of their APIs may be ignorable (nor can I conceive how many such tooling could work). I suspect that many library authors do not immediately know which methods are ignorable, and many will be forgotten and not marked in the first few versions.
I agree with @CLOVIS-AI. Maybe, in the future, state 3 can be the default if someone has explicitApi turned on?
I understand why this proposal doesn't apply to regular properties, but I think it potentially should apply to provideDelegate. Multiple DSLs use provideDelegate to access the property name as some kind of identifier, for example:
Gradle:
val commonMain by sourceSets.dependencies.getting {
...
}
GitLab CI DSL:
gitlabCi {
val test by stage()
val build by job(test) {
script {
shell("echo hello world")
}
}
}
In both of these situations, a val is declared to allow the property to be referenced in some other part of the DSL. However, not referencing the property anywhere is also fine, as its mere creation has a side effect (through provideDelegate).
Currently, these examples suffer that the variable is declared as unused, even though it very much isn't, and cannot be removed without impacting the code. The user thus requires adding a suppress annotation, but since it applies to the entire scope, it means suppressing any other unused warning within the lambda, even if they would have detected real issues. I believe IntelliJ may have had a hard-coded case to suppress the unused warning in Gradle's case, because it used to complain about it and doesn't anymore.
Therefore, I think that if a provideDelegate method is annotated with @IgnorableReturnValue, no “unused” warnings should be emitted, even if the value is assigned to a variable, even though that counts as usage in the rest of this proposal.
I do however agree that getValue and setValue should never be ignorable.
@kyay10 said: Maybe, in the future, state 3 can be the default if someone has explicitApi turned on?
Maybe, but I don't think that's enough. I'm thinking of large multi-module application codebases, which are what I deal with daily. These are not written by library authors, and certainly won't enable explicit API. Yet, these modules are even less likely to contain ignorable methods, because ignore methods are usually a sign of a low-level feature, and application-level code tends not to have those, in my experience.
So I think we should strive for full mode to be enabled in such projects (once it's stable, of course), and application developers tend to be less passionate about following Kotlin options than library authors, so they are likely to never enable it themselves.
@CLOVIS-AI
In this example, would B be marked as unused?
fun foo(): Unit = try { ... } finally { B }```
No, there won’t be warnings in the first version.
However, this is an important topic, so let me elaborate, as there are some uncertainties, and feedback is welcome.
Overall, this relates to how the feature interacts with Unit-coercion:
fun runUnit(f: () -> Unit) = f()
fun compute(): String = ""
runUnit {
compute() // Unit-coercion. Warning or not?
}
Here, the runUnit call automatically transforms into:
runUnit {
compute()
Unit
}
This is often convenient: we place some computation at the bottom of a lambda, clearly indicating that we don't intend to use it later. Additionally, there’s an explicit Unit, which can serve as an opt-in. Moreover, a related feature, 'callable reference adaptation,' allows passing references to functional types with Unit as the expected type:
runUnit(::compute) // OK!
Ideally, both versions should provide a consistent answer on whether there are warnings or not.
However, there are cases where users might unintentionally ignore a value, for example, if compute can fail or if it’s a pure/stateless function (forgetting the return value of a pure function is always a bug).
So, I suggest the following approach: for regular calls, we don’t issue a warning for Unit-coercion because of mentioned advantages. However, if a method includes an additional metadata indicating it's a stateless computation or it might fail, we should treat it as a warning or error, even in Unit-coercion:
fun compute(): String { ... }
@Stateless
fun computeStateless(): String { ... }
fun mightFail(): Int | Error // potential syntax for errors
runUnit {
compute() // OK!
}
runUnit {
computeStateless() // warning/error!
}
runUnit {
mightFail() // warning/error!
}
Currently, we don’t have a mechanism for stateless or error functions, but we may consider adding one in the future. Such a mechanism would enable deeper analysis and additional capabilities while also integrating well with the unused return value checker
@CLOVIS-AI A nitpick: finally {} block does not return a value, so in try { ... } finally { B }, B is unused in any case. However, for regular lambdas, this feature is called 'Coercion to Unit', and yes, we plan to address it in the checker: https://youtrack.jetbrains.com/issue/KT-74860/Support-Unit-coercion-in-unused-return-value-checker
Still, abuse of ?.let is one of the main problems I see in existing large Kotlin codebases, due to developers attempting to use it everywhere. One such abuse is using .let to fire side-effects with no meaningful return value, and it would have been great if this proposal could flag such cases.
We may add .let as a special propagating case at earlier stages before full rollout of the contracts
Regarding Stage 2/3 as a stable one: it depends on the ecosystem and adoption. If we would see that feature is widely adopted and becomes an ecosystem standard, having 3 as a default indeed makes sense.
It might be worth extracting the library level opt-in/versioning scheme to a separate KEEP, as this is surely not the last time you will want to change the semantics of existing code in a migrate-able way. It's a general problem and having numerous separate annotations would be a pity.
Bear in mind, some libraries will never adopt this proposal ever, because it would be a breaking change that might create a lot of work for users, and they have committed to not break their APIs. I personally would prefer a conventional @MustUseReturnValue style annotation for this reason. It signals that not using a return value is definitely, always a bug, which is usually good enough that people are willing to incorporate such checks into stable APIs. Merely flagging that you think someone should use a return value (which is what a default of on will often mean), isn't a sufficiently strong reason to break an API, and therefore adoption won't happen.
I think therefore you should start with letting library developers explicitly flag functions and properties where the return value must be used, inferring those in some cases (e.g. when a function returns Closeable), and leave the global opt-in big bang world migration to a later KEEP.
Bear in mind, some libraries will never adopt this proposal ever, because it would be a breaking change that might create a lot of work for users, and they have committed to not break their APIs.
How would it be a breaking change? Adopting the proposal only enables new warnings, it doesn't break existing code. Adding a warning on existing code is completely fine, it's not a breaking change.
For any client codebase that has -Werror turned on new warnings are a breaking change. For any client codebase that doesn't it still creates upgrade work as many codebases have an expectation of compiling without warnings even if they're not treated as errors, and the resolutions in some cases may be a matter of opinion (there are lots of cases where an author may feel a return value should be used, but in reality it can be optional).
If it's targeted, then the upgrade pain can be OK. If it's potentially going to flag hundreds of times in user's code (which for something like this is imaginable) then I'd be reluctant to enable such a thing.
I accept that you can tell users to suppress the warnings globally though.
I personally would prefer a conventional @MustUseReturnValue style annotation for this reason. It signals that not using a return value is definitely, always a bug, which is usually good enough that people are willing to incorporate such checks into stable APIs.
I do not understand this argument, sorry. Either part of your API is becoming marked with @MustUseValue (explicitly or implicitly) and this is a new compiler error for users with -Werror, or you do not accept this proposal at all. It doesn't matter if the code becomes annotated automatically or manually — you should adopt @MustUseValue only when you're sure that @IgnorableReturnValue is placed everywhere it is supposed to be.
Having a third, 'non-annotated' state will be equivalent to having @Ignorable on all such functions, which contradicts the original goal of having @MustUse everywhere by default.
Regarding separate KEEP for switching the default — it is too soon to talk about it, but I imagine that an additional document that lists what libraries are already migrated and assesses the impact of switching indeed may be required.
I like the idea of catching unused-value defects. However, while most return values are expected to be used, having worked with several codebases with over a million lines of code, the opposite is not as rare as the KEEP suggests since there are some categories of patterns where this is extremely popular. For example, the builder pattern, which is probably the most popular, has all functions returning this.
The builder return values are handy for chaining but they are often ignored
fun appendInterestingProducts(results: ResultBuilder) {
if (topSellingProductIsAvailable()) {
// ignore builder return value
results.add(getTopSellingProduct)
}
// another common usage ignoring the builder return value
getMostViewedProduct()?.let { results.add(it) }
}
// alternative implementation which is also common
fun appendInterestingProducts(results: ResultBuilder) {
results.apply {
add(getTopSellingProduct())
add(getMostViewedProduct())
}
// similarly
with(resultBuilder) {
add(getTopSellingProduct())
add(getMostViewedProduct())
}
}
These types of patterns are actually very common in CRUD backend services as we usually use some type of builder to create and run multiple queries and then we use multiple levels of builders to populate the results.
I also want to point out the distinction between how often these types of functions with ignorable return values exist versus how often they are used. Common utilities might represent a smaller percentage of the codebase but they can represent a higher percentage of the usages. So it's important that the end-state of this feature adds value without adding friction for the typical developer.
We also can't expect all libraries to use this feature. To increase adoption, it should also be easy for library developers to handle the reverse scenario where almost all return values are ignorable except for the build() functions etc. One way to accomplish this would be to somehow declare that the entire module / file / class has ignorable return values and override it for the non-ignorable ones with @MustUseReturnValue.
Good point. We are aware of how builder patter is popular, and likely would develop a specialised solution for it further down the road.
I don't think this proposal is complete without showing how the popular builder pattern will be handled even if that will be implemented in a later phase.
The main thing that I'm concerned about is that this feature might add friction to regular Kotlin development. It would be annoying if we started to get thousands of warnings for builder usages and especially for projects where warnings are treated as errors.
I prefer to write functions that return some sort of Result<T> instead of just T. This keeps my code mostly clean from exceptions and enhances my ability to handle failures elegantly. By far the biggest issue with the Result<T> pattern is the risk that you would forget to check if the result is a success in the case of Result<Unit> (when there is no output, but it can still return a failure). This is very big risk as silent errors are much deadlier than loud ones. That is why I love this proposal and wish I could upvote it 100 times for how well it would help to address this risk. And I say that also because I don't just have a single Result type, I have many custom types that can represent failures and its too unsafe to have to manually mark each function that returns a type like this with a @CheckReturnValue or something.
@daniel-rusu , I share your feelings about build warnings in general (I'm so annoyed by unsolvable warnings) but is your concern not completely addressed by the recently added ability to exclude specific compiler warnings?
