Skip to content

False positive: UseCheckOrError #2514

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 26, 2020 · 6 comments · Fixed by #2556
Closed

False positive: UseCheckOrError #2514

BraisGabin opened this issue Mar 26, 2020 · 6 comments · Fixed by #2556

Comments

@BraisGabin
Copy link
Member

Expected Behavior

This code should not be flagged

val lambda: (Throwable) -> Unit = {
  if (it is IllegalStateException) {
    println("something")
  } else {
    throw IllegalStateException(it)
  }
}

Observed Behavior

It is flagged

Context

Replacing throw IllegalStateException(error) with error(error) changes the behaviour so it should not be flagged.

Your Environment

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

schalkms commented Mar 26, 2020

Okay. Have you tried to rewrite the lambda to something like this?

val lambda: (Throwable) -> Unit = {
  check(it is IllegalStateException) { throw IllegalStateException(it) } // or just { it }
  println("something")
}

@BraisGabin
Copy link
Member Author

My snipped was just a simplification. The point is that throw IllegalStateException(it) is not the same as error(it) unless it is a String. We need type resolution to fix this problem.

@schalkms
Copy link
Member

schalkms commented Mar 27, 2020

@BraisGabin mine was also about simplification. The whole rule is about simplification. You could use either error or check. I'm curios, what part of my code snippet didn't work for you?

@BraisGabin
Copy link
Member Author

when(throwable) {
    is NumberFormatException -> println("a")
    is IllegalArgumentException -> println("b")
    is NullPointerException -> println("c")
    else -> throw IllegalStateException(throwable)
}

The code is something like this but with domain exceptions (users not logged, user disabled, user no verfied...).

@arturbosch
Copy link
Member

The basic rule part could check for a string literal argument.
The advanced part when BindingContext is set could resolve the type ^^.

@schalkms
Copy link
Member

Now I see the issue. error() and check() don't provide an overload for wrapping exceptions. That's unfortunate. To solve this problem, one needs to use the BindingContext and check for exactly 1 argument of type String.

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