Is there any rule that would allow enforcing a return value (from a function marked with an annotati...

# detekt

[bbaldino]

Is there any rule that would allow enforcing a return value (from a function marked with an annotation) wasn't ignored?

[Artur Bosch]

Could you elaborate with a code snippet please?

[bbaldino]

for example, the following function:

fun Byte.putBit(bitPos: Int, isSet: Boolean): Byte {
    return if (isSet) {
        (this.toInt() or (0b10000000 ushr bitPos)).toByte()
    } else {
        (this.toInt() and (0b10000000 ushr bitPos).inv()).toByte()
    }
}

[bbaldino]

it would be easy for someone to call myByte.putBit(1, true) and think it would be modified in place, but actually it returns the new value. i'm looking for a way similar to 'nodiscard' in c++ where i can generate a warning of some kind in this scenario

[bbaldino]

to warn the user that they should not ignore the return value of the function

[diego-gomez-olvera]

In Android there is https://developer.android.com/reference/androidx/annotation/CheckResult

[diego-gomez-olvera]

and JSR-305 https://errorprone.info/bugpattern/CheckReturnValue

[diego-gomez-olvera]

I see that there is https://youtrack.jetbrains.com/issue/KT-12719 for Kotlin

[bbaldino]

ah cool, i couldn't find a bug. voted.

[bbaldino]

and will check out errorprone

[schalkms]

Thanks for helping out @diego-gomez-olvera!

[diego-gomez-olvera]

No problem

[bbaldino]

so is there any rule for an ignored return value?

[bbaldino]

and, if not (i don't seem to see one), would it be possible to add one? is it possible to get information about a called function when examining a call site (i.e. whether or not it has a return)

[schalkms]

would it be possible to add one?

Yes, you could add such a rule to detekt. You would need to use type and symbol solving, which is shown in the following example.
https://github.com/arturbosch/detekt/blob/master/detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/UselessCallOnNotNull.kt

If you have a strong need for this rule, you can certainly re-implement this in detekt and contribute back to the project.

[bbaldino]

Thanks for the reference. Do you think it would be something that would belong in detekt? I definitely think it would be nice to have. Especially if an annotation was used, calling a method which had a CheckResult (or whatever) annotation and not using its return would almost definitely indicate a bug.

[schalkms]

Do you think it would be something that would belong in detekt?

Yeah, it would be a nice add-on with a very low priority, since there are existing solutions. There are many other features, which have way higher priority. We rely on contributions from our awesome community for such rules. Of course we try to assist as much as possible.

[bbaldino]

Ok, good to know. One other question: does detekt introduce any of its own annotations? One decision would be whether or not to warn on ALL functions who return a value which isn't saved vs. just those with an annotation. Now that I've typed that out, though, perhaps doing it for all cases is best. EDIT: And the rule could be disabled by users for those cases where it shouldn't warn.

[schalkms]

One decision would be whether or not to warn on ALL functions who return a value which isn't saved vs. just those with an annotation

We handle this with configuration options. For example:

LongParameterList:
    active: true
    threshold: 6
    ignoreDefaultParameters: false

[bbaldino]

Ok, I more had in mind using https://arturbosch.github.io/detekt/suppressing-rules.html

[bbaldino]

But, basically, just always warn and let users use detekt's existing mechanism to disable it (entirely, case-by-case, etc.) if they want

[schalkms]

And the rule could be disabled by users for those cases where it shouldn't warn.

Yes. Detekt supports the @Suppress annotation.

[schalkms]

But, basically, just always warn and let users use detekt's existing mechanism to disable it (entirely, case-by-case, etc.) if they want

Yes, considering the user defined config options like threshold for instance.

[schalkms]

I hope that I could answer all your questions.

[bbaldino]

👍 Yup, thanks.

[schalkms]

@bbaldino could you please also create an issue for this on GitHub with all the written information copied from Slack? This would certainly help to better track this feature.

[bbaldino]

Sure

[bbaldino]

https://github.com/arturbosch/detekt/issues/2239

[bbaldino]

So, I'm looking at the UselessCallOnNotNull test, to see how it's parsing the expression. But it looks like the KtQualifiedExpression passed to visitQualifiedExpression does not include the assignment (i.e. for val testList = listOf("string").orEmpty, it just has listOf("string").orEmpty ). Do you happen to know if there's an expression type which includes any assignment (if it exists) as well as the expression?

[bbaldino]

Ok, found that it's visitDeclaration

[bbaldino]

I opened a PR with an attempt here: https://github.com/arturbosch/detekt/pull/2242. I'd consider it a draft at this point, but hopefully it's good enough to get some feedback.

[Artur Bosch]

This is a vey cool rule and fits perfectly in the potential-bugs rule set, thanks for contributing. I will try to come to your PR later that week.