Thread
#detekt
    b

    bbaldino

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

    Artur Bosch

    2 years ago
    Could you elaborate with a code snippet please?
    b

    bbaldino

    2 years ago
    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()
        }
    }
    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
    diego-gomez-olvera

    diego-gomez-olvera

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

    bbaldino

    2 years ago
    ah cool, i couldn't find a bug. voted.
    and will check out errorprone
    s

    schalkms

    2 years ago
    Thanks for helping out @diego-gomez-olvera!
    diego-gomez-olvera

    diego-gomez-olvera

    2 years ago
    No problem
    b

    bbaldino

    2 years ago
    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

    2 years ago
    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

    2 years ago
    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

    2 years ago
    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

    2 years ago
    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

    2 years ago
    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
    b

    bbaldino

    2 years ago
    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

    2 years ago
    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

    2 years ago
    👍 Yup, thanks.
    s

    schalkms

    2 years ago
    @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

    2 years ago
    Sure
    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

    2 years ago
    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.