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
Match extension functions #4459
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,6 +186,66 @@ class FunctionMatcherSpec(private val env: KotlinCoreEnvironment) { | |
val methodSignature = FunctionMatcher.fromFunctionSignature("toString(String)") | ||
assertThat(methodSignature.match(function, bindingContext)).isEqualTo(result) | ||
} | ||
|
||
@DisplayName("When lambdas foo(() -> kotlin.String)") | ||
@ParameterizedTest(name = "in case {0} it return {1}") | ||
@CsvSource( | ||
"fun foo(a: () -> String), true", | ||
"fun foo(a: () -> Unit), true", | ||
"fun foo(a: (String) -> String), false", | ||
"fun foo(a: (String) -> Unit), false", | ||
"fun foo(a: (Int) -> Unit), false", | ||
) | ||
fun `When foo(() - kotlin#String)`(code: String, result: Boolean) { | ||
val (function, bindingContext) = buildKtFunction(env, code) | ||
val methodSignature = FunctionMatcher.fromFunctionSignature("foo(() -> kotlin.String)") | ||
assertThat(methodSignature.match(function, bindingContext)).isEqualTo(result) | ||
} | ||
|
||
@DisplayName("When lambdas foo((kotlin.String) -> Unit)") | ||
@ParameterizedTest(name = "in case {0} it return {1}") | ||
@CsvSource( | ||
"fun foo(a: () -> String), false", | ||
"fun foo(a: () -> Unit), false", | ||
"fun foo(a: (String) -> String), true", | ||
"fun foo(a: (String) -> Unit), true", | ||
"fun foo(a: (Int) -> Unit), true", | ||
) | ||
fun `When foo((kotlin#String) - Unit)`(code: String, result: Boolean) { | ||
val (function, bindingContext) = buildKtFunction(env, code) | ||
val methodSignature = FunctionMatcher.fromFunctionSignature("foo((kotlin.String) -> Unit)") | ||
assertThat(methodSignature.match(function, bindingContext)).isEqualTo(result) | ||
} | ||
|
||
@DisplayName("When extension functions foo(kotlin.String)") | ||
@ParameterizedTest(name = "in case {0} it return {1}") | ||
@CsvSource( | ||
"fun String.foo(), true", | ||
"fun foo(a: String), true", | ||
"fun Int.foo(), false", | ||
"fun String.foo(a: Int), false", | ||
"'fun foo(a: String, ba: Int)', false", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are those single quotes around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes they are. This string is parsed as a csv. And this function has two parameters so it needs a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you could use a different separator though (Random guy yelling from the side line) 😉 |
||
) | ||
fun `When foo(kotlin#String)`(code: String, result: Boolean) { | ||
val (function, bindingContext) = buildKtFunction(env, code) | ||
val methodSignature = FunctionMatcher.fromFunctionSignature("foo(kotlin.String)") | ||
assertThat(methodSignature.match(function, bindingContext)).isEqualTo(result) | ||
} | ||
|
||
@DisplayName("When extension functions foo(kotlin.String, kotlin.Int)") | ||
@ParameterizedTest(name = "in case {0} it return {1}") | ||
@CsvSource( | ||
"fun String.foo(), false", | ||
"fun foo(a: String), false", | ||
"fun Int.foo(), false", | ||
"fun String.foo(a: Int), true", | ||
"'fun foo(a: String, ba: Int)', true", | ||
) | ||
fun `When foo(kotlin#String, kotlin#Int)`(code: String, result: Boolean) { | ||
val (function, bindingContext) = buildKtFunction(env, code) | ||
val methodSignature = FunctionMatcher.fromFunctionSignature("foo(kotlin.String, kotlin.Int)") | ||
assertThat(methodSignature.match(function, bindingContext)).isEqualTo(result) | ||
} | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels like a stop gap solution to me. When we disable type resolution, and we only want to forbid extension methods like
String.hello(a: Int)
, we would actually report both extension functionString.hello(a: Int)
and normal functionhello(s: String, a: Int)
.Additionally, detekt should be compatible with other kotlin platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I can't think of any other way to fix this without adding breaking changes to the syntax. And, even adding breaking changes, I can only think of more complex syntax, I can't think on any elegant way to fix this. Any idea in this regard is more than welcome.
This rule (and feature in general) only works with Type Solving so this is not a real issue.
Agree. Right now it's noy an issue because we only support type solving for JVM. But in the future maybe in js you could have both functions and that would be a problem.
The biggest issue to me is that it's not intuitive for a user how to use this feature. And I think that it should. But as I said before to handle this case properly we need to add a breaking change in the syntax. And, even then, I don't know how to define the function signature in an easy and elegant way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's the major point here. TR being a requirement is not a problem, as if you want to use an advanced capability, you need to opt-in to TR.
It's already a concern, as the compiler plugin runs with TR on all the platforms.
What would be your suggested way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
toString
of aCallableDescriptor.original
of this function:is
If I simplify that I get (the output of the function is not relevant for the signature):
T
is not clear where it was defined so I would do something like this:Problems:
defined in
but I don't know how to simplify it.To compare, if we merge this PR (and the next one) the signature for that function would be:
Other option is to use the syntax of
apiDump
:But I really don't like this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was actually thinking about something like that. I know that is not super immediate to grasp. But at the same time it comes from the bytecode representation of a method, and it's sort of "consistent" with the rest of the tools in the ecosystem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that one the issues raised by @chao2zhang applies too:
ResourcesKt
for the top level functions)I agree that using something standard would be great but I don't think that syntax would work for us. And I don't know of any other standard one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we unblock this? To me we should think if this change improves of make worst this rule. I think that it's better than don't support extension functions. An example would like to forbid the usage of
androidx.activity.compose.setContent
in favor of my our ownsetContent
and right now it's impossible and #4448 is other reason.We can open an issue to talk about which should be the function signature syntax that we should use and then we can implement it. And once we take a decission we can implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I'm in for merging this. I think having the extension function receiver modelled as first parameter is a good compromise and aligns to the approach taken by other tools as well.