detekt
detekt copied to clipboard
Fail lint for `error(throwable)`
Using error(throwable)
is kind of bad thing to do. It will result in throw IllegalStateException(throwable.toString())
.
Expected Behavior of the rule
Fail lint, if error()
argument is of Throwable
type.
Hi, @BraisGabin / @WildOrangutan I think we can add this in the forbidden method. So that using error(throwable)
will be reported
Been doing some poking around and the issue lies in FunctionMatcher.WithParameters.match(CallableDescriptor)
, where it compares the encounteredParamTypes
and parameters
.
If we configure it to forbid method call kotlin.error(kotlin.Throwable)
, it results in:
-
encounteredParamTypes
to be[kotlin.Any]
and, -
parameters
to be[kotlin.Throwable]
and it is just doing a string comparison so it doesn't know both parameter types indicate the same function signature.
We can check their types to verify if one is a supertype of another? would appreciate it if there is a better alternative.
Thinking out loud. If I have this:
fun foo(any: Any){}
fun foo(number: Number){}
If now I forbid foo(Any)
then a call like foo(5)
would be forbidden? That shouldn't be the case. So I'm not sure if we can change this without breaking other things. Do you see a way to make it work? Then we can think about how to implement it and what we should use instead of String
.
yeah, that would be bad, but in this case, if I want to forbid food(Throwble)
; rather than checking the signature of the destination method being called we could compare the signature of the forbidden method and the signature of the method in user codebase (not sure what it is called).
In the current version, if we have :
fun foo(any: Any){}
fun foo(number: Number){}
and in the codebase, somewhere we have :
foo(Throwable())
and I forbid foo(Throwable)
, it results in a comparison of the forbidden method signature ( foo(Throwable)
) with the destination method signature ( foo(Any)
) but if we could compare the forbidden method signature ( foo(Throwable)
) with the provided signature foo(Throwable)
in user codebase if that is possible? also doing so would it break anything existing?
The problem is a bit more complex than that. Even ignoring the problem I pointed out before. In the codebease you would have something like foo(IllegalStateException())
. And you would forbid foo(Throwable)
.
Also, I would like to know if there are more real use cases to something like this. This change is not going to be easy so, if there are more use cases to forbid something like this we could thing to implement it. Otherwise I would vote for an specific rule.
We might be better off with a specific rule.