https://kotlinlang.org logo
#detekt
Title
b

bbaldino

01/07/2020, 10:04 PM
Is there any rule that would allow enforcing a return value (from a function marked with an annotation) wasn't ignored?
a

Artur Bosch

01/08/2020, 7:05 AM
Could you elaborate with a code snippet please?
b

bbaldino

01/08/2020, 5:49 PM
for example, the following function:
Copy code
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()
    }
}
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
to warn the user that they should not ignore the return value of the function
d

diego-gomez-olvera

01/09/2020, 11:37 AM
I see that there is https://youtrack.jetbrains.com/issue/KT-12719 for Kotlin
b

bbaldino

01/09/2020, 5:24 PM
ah cool, i couldn't find a bug. voted.
and will check out errorprone
s

schalkms

01/09/2020, 9:00 PM
Thanks for helping out @diego-gomez-olvera!
d

diego-gomez-olvera

01/10/2020, 8:58 AM
No problem
b

bbaldino

01/10/2020, 5:53 PM
so is there any rule for an ignored return value?
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)
s

schalkms

01/10/2020, 8:52 PM
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.
b

bbaldino

01/10/2020, 9:05 PM
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.
s

schalkms

01/10/2020, 9:11 PM
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.
b

bbaldino

01/10/2020, 9:13 PM
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.
s

schalkms

01/10/2020, 9:15 PM
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:
Copy code
LongParameterList:
    active: true
    threshold: 6
    ignoreDefaultParameters: false
b

bbaldino

01/10/2020, 9:16 PM
But, basically, just always warn and let users use detekt's existing mechanism to disable it (entirely, case-by-case, etc.) if they want
s

schalkms

01/10/2020, 9:17 PM
And the rule could be disabled by users for those cases where it shouldn't warn.
Yes. Detekt supports the
@Suppress
annotation.
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.
I hope that I could answer all your questions.
b

bbaldino

01/10/2020, 9:18 PM
👍 Yup, thanks.
🙂 1
s

schalkms

01/10/2020, 9:24 PM
@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.
b

bbaldino

01/10/2020, 9:24 PM
Sure
👍 1
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?
Ok, found that it's
visitDeclaration
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.
a

Artur Bosch

01/12/2020, 5:59 PM
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.
2 Views