detekt icon indicating copy to clipboard operation
detekt copied to clipboard

Fail lint for `error(throwable)`

Open WildOrangutan opened this issue 4 months ago • 6 comments

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.

WildOrangutan avatar Feb 16 '24 15:02 WildOrangutan

Hi, @BraisGabin / @WildOrangutan I think we can add this in the forbidden method. So that using error(throwable) will be reported

atulgpt avatar Mar 05 '24 07:03 atulgpt

Been doing some poking around and the issue lies in FunctionMatcher.WithParameters.match(CallableDescriptor), where it compares the encounteredParamTypes and parameters.

image

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.

psuzn avatar Mar 05 '24 18:03 psuzn

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.

BraisGabin avatar Mar 07 '24 14:03 BraisGabin

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?

psuzn avatar Mar 07 '24 15:03 psuzn

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.

BraisGabin avatar Mar 07 '24 17:03 BraisGabin

We might be better off with a specific rule.

psuzn avatar Mar 08 '24 02:03 psuzn