Skip to content

False positive in OptionalUnit rule #2452

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
BraisGabin opened this issue Mar 18, 2020 · 5 comments · Fixed by #2886
Closed

False positive in OptionalUnit rule #2452

BraisGabin opened this issue Mar 18, 2020 · 5 comments · Fixed by #2886

Comments

@BraisGabin
Copy link
Member

BraisGabin commented Mar 18, 2020

Expected Behavior

val <T> T.exhaustive: T
    get() = this

enum class E { A, B }

fun a(value: E) {
    when (value) {
        E.A -> println("hi")
        E.B -> {
            if (System.currentTimeMillis() % 2 == 0L) {
                println("hi")
            }
            Unit
        }
    }.exhaustive
}

This code is correct. I need to add Unit at the end of B because if is not an expression so exhaustive fails.

Observed Behavior

This code is flagged

Context

I want to force exhaustive whens so I need to do things like this. Maybe I'm missing an easier and more elegant way to do it, if so, please tell me.

Your Environment

  • Version of detekt used: 1.6.0
@schalkms
Copy link
Member

Unfortunately, the mentioned code snippet doesn't compile.

I don't know how you would want to detect this case in the OptionalUnit rule without having sophisticated data flow analysis.
Anyways, I think this is way too complex to handle.

@BraisGabin
Copy link
Member Author

BraisGabin commented Mar 18, 2020

The code now compiles.

I think that it's not that difficult... I'll give it a try.

@schalkms
Copy link
Member

I think that it's not that difficult... I'll give it a try.

I'm curious, how would you want to decide whether the Unit is unnecessary?
If you leave out the .exhaustive you don't need the Unit at the end.

@BraisGabin
Copy link
Member Author

You are right, I wasn't thinking about that! I was just thinking about an Unit after an if... Yeap, it's a bit more difficult. I'll give it a try tomorrow. I can't leave my house so I have A LOT of time x"D

@schalkms
Copy link
Member

I don’t think it’s a good idea to reimplement this just for the sake of fixing this false-positive. This could fix the original problem, but it results in other false-positives.
I’d suggest to take a look at the IntelliJ or Kotlin inspection that triggers this and maybe call the corresponding function.
Otherwise, we would add complexity that isn’t worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants