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.

[hrach]

@zarechenskiy thank you for the comment.

Is the "stable" property concept related to the Java interop? Or could you please answer that as well?

[zarechenskiy]

@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!
}

[CLOVIS-AI]

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”.

[CLOVIS-AI]

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.

[CLOVIS-AI]

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.

[kyay10]

I agree with @CLOVIS-AI. Maybe, in the future, state 3 can be the default if someone has explicitApi turned on?

[CLOVIS-AI]

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.

[CLOVIS-AI]

@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.

[zarechenskiy]

@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

[sandwwraith]

@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.

[mikehearn]

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.

[CLOVIS-AI]

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.

[mikehearn]

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.

[sandwwraith]

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.

[daniel-rusu]

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.

[sandwwraith]

Good point. We are aware of how builder patter is popular, and likely would develop a specialised solution for it further down the road.

[daniel-rusu]

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.

[mgroth0]

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?

[kyay10]

What's the plan for supporting functions like:

fun Foo.copy(destination: MutableList<Blah> = mutableListOf()): List<Blah>

I think at least one such function exists in the stdlib. The result of such a function is pointless if a destination is provided, but the result is necessary if no destination was provided. Should such functions be split into 2 declarations?

[daniel-rusu]

@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?

Excluding this category of compiler warnings would defeat the purpose of this feature. Excluding a category is either done when we disagree with the warnings or if we plan to address them at a later date, but if there is no planned mechanism for dealing with builders then this effectively ignores this new language feature. Teams that treat warnings as errors want to ensure that all warnings are resolved as the number of warnings always grow over time in larger codebases when not strictly prevented. We rely on the error mechanism as the primary means of becoming aware of and preventing a new warning from being introduced. Being required to exclude this compiler warning would essentially divide adoption among the community especially for enterprise codebases where builders are extremely common.

I think that it's premature to introduce a feature without having a plan for how it will interact with one of the most popular patterns.

[daniel-rusu]

I just took a look at the second proposal for unused local variables. The example given is:

val _ = writeTo(file) // We are not interested in whether the write operation was successful

This looks like it's just saving a few characters compared to a more obvious explicit solution. Even worse, without the comment, a newer developer would be confused why we're declaring a local variable without a name:

val _ = writeTo(file)

An explicit solution removes confusion and makes the intent obvious even without a comment:

@Suppress("UNUSED_RETURN_VALUE")
writeTo(file)

Alternatively, this feature could come with a new annotation instead:

@IgnoreReturnValue
writeTo(file)

The new unused-local-variable syntax unnecessarily complicates the language especially since the only use-case is for this new language feature of detecting unused return values. So it doesn't appear to meet the minus 100 points rule and reduces code clarity compared to an explicit solution.

[kyay10]

@daniel-rusu One argument in favour of that syntax is that it makes it rather easy to later use the return value if you'd like, by simply giving it a name

[JakeWharton]

_ as an unused name already exists in the language, but only in certain positions. Expanding it to work in more is generally useful. I wanted it earlier in the week for code that did for (unused in collection), as an example.

[daniel-rusu]

@daniel-rusu One argument in favour of that syntax is that it makes it rather easy to later use the return value if you'd like, by simply giving it a name

The difference in effort is too miniscule to be a deciding factor. This "saving" is more than offset by the negative impacts to code clarity especially when the ratio of reading to writing code is around 10 to 1 according to most estimates.

Besides, coding for this type of future possibility goes against development best practices of choosing the simplest and clearest solution that meets current needs.

[daniel-rusu]

_ as an unused name already exists in the language, but only in certain positions. Expanding it to work in more is generally useful. I wanted it earlier in the week for code that did for (unused in collection), as an example.

The KEEP says:

Underscore can be used only in local variable declaration: val _ = ...

and later down re-confirms that a val is required.

So it doesn't look like it would help in your use-case. As it stands, the usefulness of the new syntax seems to be strictly tied to this new feature of dealing with unused return values.

[JakeWharton]

Sure, I'm just countering your point that somehow it costs any fictional language bucks to add it. It already exists and, if anything, expanding it to work anywhere you declare a name is a reduction in cognitive complexity.

[daniel-rusu]

Sure, I'm just countering your point that somehow it costs any fictional language bucks to add it. It already exists and, if anything, expanding it to work anywhere you declare a name is a reduction in cognitive complexity.

If the expansion was uniform everywhere then yeah, but the KEEP is only adding this new syntax specifically for dealing with unused return values and it doesn't state any goals of further expansion in future phases. We also don't currently declare named local variables when ignoring return values as we just call the function without assigning it to anything:

writeTo(file)

This new feature in combination with the new unused-local-variable syntax would require the line to be changed to this in order to remove the warning:

val _ = writeTo(file)

Since this new syntax is only targeting unused return values for local variables, my point is that this goal would be better achieved by some sort of annotation as that leads to less cognitive effort when reading code because the intent is more obvious and explicit:

@IgnoreReturnValue
writeTo(file)

[JakeWharton]

Yes, I read your original post, and I don't personally agree.

[sandwwraith]

Even worse, without the comment, a newer developer would be confused why we're declaring a local variable without a name

I do not understand where the confusion would arise if the only purpose of declaring an unnamed variable is to specify explicitly that it is unused/ignored.

[daniel-rusu]

Even worse, without the comment, a newer developer would be confused why we're declaring a local variable without a name

I do not understand where the confusion would arise if the only purpose of declaring an unnamed variable is to specify explicitly that it is unused/ignored.

I think that's because you're looking at this from the perspective of a developer that was already trained on this syntax and chose to use this construct. After all, we could memorize any complicated pattern and make it mean whatever we want but that's not a good way to guage the complexity or intuitiveness of the solution. Instead, I'm evaluating it in comparison to alternative solutions by looking at what would minimize confusion for users that haven't already reached an intermediate level with Kotlin and haven't memorized this new capability yet. Making new constructs intuitive where we can read it and automatically infer the meaning also simplifies the mental model of the language for existing developers.

We currently use _ for scenarios where we are forced to follow existing structures such as when abiding by a lambda signature or when skipping over an element while destructuring. The _ isn't an addition but rather a replacement in the existing structure to signify that we're ignoring that section of the construct. Although it's not in this KEEP, if we hypothetically allowed using _ in other constructs like for( _ in values) then that would align with the current pattern of designating a portion of an existing construct as ignored. However, this KEEP strays from the current pattern as declaring a new local variable extends the existing construct instead of replacing an existing portion of it.

For a new or existing Kotlin developer that hasn't memorized this new syntax yet, we would call a function and get a warning about the unused return value. If that's intentional, our first instinct would be to deal with it as we currently do for other warnings. We would try to suppress the warning with some annotation. It's unintuitive to consider declaring a new variable because we don't intent to use it. Declaring a new variable and skipping its name is 2 steps removed so it's even more unintuitive.

New developers learn by reading existing code. When encountering something like:

@IgnoreReturnValue
writeTo(file)

it's immediately obvious that we're writing to some file and ignoring the return value. However, if a new developer reads:

val _ = writeTo(file)

then this would be quite perplexing and confusing about why it's written like this. The new developer would do some research and find out that _ is used when we don't want to use the variable, but then why declare the variable in the first place? This is not intuitive at all for new developers whereas reading an annotation would immediately make the intent trivial.

Another red flag with the new syntax is that it feels like we need to add a comment explaining that we want to ignore the return value, such as the example in the KEEP. An explicit annotation would be self-explanatory without the need for any comments because that would be more intuitive.

Here's an example where the new syntax is even more awkward and confusing:

// original
createFile()?.let { writeTo(it) }

// fixed with new syntax
createFile()?.let { val _ = writeTo(it) }

// fixed with annotation
createFile()?.let { @IgnoreReturnValue writeTo(it) }

the new syntax makes this example misleading and confusing even for developers that are aware of this feature when we quickly skim the code because it almost looks like the lambda parameter.

[JakeWharton]

While your comment is a bit hyperbolic painting underscore as syntax Satan and the annotation as syntax Jesus, ignoring that, you claim

An explicit annotation would be self-explanatory without the need for any comments because that would be more intuitive.

I reject this outright.

It's one thing to argue that the change in syntax to an annotation makes the language feature self-describing. But the syntactic change is entirely orthogonal to the justification as to why it is safe to ignore the value.

[daniel-rusu]

While your comment is a bit hyperbolic painting underscore as syntax Satan and the annotation as syntax Jesus, ignoring that, you claim

I don't think it's appropriate or professional to use religious terms in technical discussions

[JakeWharton]

Apologies. They are fictional characters to me.

[benjishults]

Here, we are proposing val _ = abc() to mean that abc() is executed and we don't care about the return value.

Part of me likes this very much.

However, elsewhere in the language (and in Java's pattern-matching JEPs), the _ is used to indicate not just that we don't care about the value but also that we don't need to evaluate the expression that would produce the value.

For example, when we destructure

data.let { (_, a) -> /* ... */ }

we are telling the compiler that it should not execute the component1() function on data. Since that function may be expensive and have side effects, this is a benefit.

However, here we are proposing that _ mean something quite different: I.e., that the expression is evaluated and we don't care about the result.

If we start using _ to mean such different things in different contexts, that seems like a problem.

[mgroth0]

It's unintuitive to consider declaring a new variable because we don't intent to use it. Declaring a new variable and skipping its name is 2 steps removed so it's even more unintuitive.

the new syntax is even more awkward and confusing

@daniel-rusu I agree that it is counterinutive on some level to declare a val for an unused return value. But in my opinion you are exaggerating how much this will confuse people or make the code harder to read.

val _ is also shorter than @IgnoreReturnValue, which is nice. Also it looks very official. val _ says "Hey I'm baked in to the language so get used to me". @IgnoreReturnValue says "Am I a first-class language feature or am I from some library or compiler plugin? Nobody knows".

val _ takes less editing to change to an actual variable, and it is less editing to go from a variable name to _ as well.

@IgnoreReturnValue also would require two lines instead of one with some code formatters that require annotations to not be on the same line.

we are telling the compiler that it should not execute the component1() function on data. Since that function may be expensive and have side effects, this is a benefit.

@benjishults that's interesting but we've already crossed this bridge haven't we? For example:

list.forEach { _ ->

}

So if I understand correctly it seems like _ already means different things in different contexts.

[benjishults]

@benjishults that's interesting but we've already crossed this bridge haven't we? For example:

list.forEach { _ ->

}

So if I understand correctly it seems like _ already means different things in different contexts.

Yes, I think you're right. Now that I think about it more, a better example would be cases where we implement an interface that requires parameters that our implementation doesn't need. In that case, we use _ to tell the compiler and IDE that we don't care about using the variable but, at the call site, the argument expression is still evaluated.

I think my initial objection goes away in light of this.

So, I'm back to liking the val _ proposal without reservation. 😄