Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Match extension functions #4459

Merged
merged 2 commits into from Mar 24, 2022
Merged

Match extension functions #4459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b830bb
Add missing tests
BraisGabin Jan 30, 2022
6a9f0ca
Match extension functions
BraisGabin Jan 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion ...ules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenMethodCall.kt
View file
Open in desktop
Expand Up @@ -48,7 +48,9 @@ class ForbiddenMethodCall(config: Config = Config.empty) : Rule(config) {
"Methods can be defined without full signature (i.e. `java.time.LocalDate.now`) which will report " +
"calls of all methods with this name or with full signature " +
"(i.e. `java.time.LocalDate(java.time.Clock)`) which would report only call " +
"with this concrete signature."
"with this concrete signature. If you want to forbid an extension function like" +
"`fun String.hello(a: Int)` you should add the receiver parameter as the first parameter like this: " +
Copy link
Member

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 function String.hello(a: Int) and normal function hello(s: String, a: Int).

Additionally, detekt should be compatible with other kotlin platforms.

Copy link
Member Author

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.

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.

When we disable type resolution, and we only want to forbid extension methods like String.hello(a: Int), we would actually report both extension function String.hello(a: Int) and normal function hello(s: String, a: Int).

This rule (and feature in general) only works with Type Solving so this is not a real issue.

Additionally, detekt should be compatible with other kotlin platforms.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The biggest issue to me is that it's not intuitive for a user how to use this feature.

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.

But in the future maybe in js you could have both functions and that would be a problem.

It's already a concern, as the compiler plugin runs with TR on all the platforms.

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.

What would be your suggested way?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be your suggested way?

The toString of a CallableDescriptor.original of this function:

package org.example

class A<T> {
  fun <R> String.hello(a: T -> R): T {
    TODO()
  }
}

is

public final fun <R> kotlin.String.hello(a: (T) -> R): T defined in org.example.A[SimpleFunctionDescriptorImpl@5b4f828d]

If I simplify that I get (the output of the function is not relevant for the signature):

<R> kotlin.String.hello((T) -> R) defined in org.example.A

T is not clear where it was defined so I would do something like this:

<R> kotlin.String.hello((T) -> R) defined in org.example.A<T>

Problems:

  • I don't like defined in but I don't know how to simplify it.
  • Create a parser for this can be difficult. And create proper error messages to explain to the user what they write wrong.

To compare, if we merge this PR (and the next one) the signature for that function would be:

org.example.A.hello(kotlin.String, (T) -> R)

Other option is to use the syntax of apiDump:

public final class io/github/detekt/test/utils/ResourcesKt {
	public static final fun readResourceContent (Ljava/lang/String;)Ljava/lang/String;
	public static final fun resource (Ljava/lang/String;)Ljava/net/URI;
	public static final fun resourceAsPath (Ljava/lang/String;)Ljava/nio/file/Path;
	public static final fun resourceUrl (Ljava/lang/String;)Ljava/net/URL;
}

But I really don't like this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other option is to use the syntax of apiDump:

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

Copy link
Member Author

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:

  • It's not easy to write
  • Only works for JVM (js doesn't need a ResourcesKt for the top level functions)
  • An extension functions has it's receiver as the first parameter (that's what I'm doing here)

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...

Copy link
Member Author

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 own setContent 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.

Copy link
Member

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.

"`hello(kotlin.String, kotlin.Int)`"
)
private val methods: List<FunctionMatcher> by config(
listOf(
Expand Down
16 changes: 16 additions & 0 deletions ...-style/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenMethodCallSpec.kt
View file
Open in desktop
Expand Up @@ -318,5 +318,21 @@ class ForbiddenMethodCallSpec : Spek({
).compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}

it("should report extension functions") {
val code = """
package org.example

fun String.bar() = Unit

fun foo() {
"".bar()
}
"""
val findings = ForbiddenMethodCall(
TestConfig(mapOf(METHODS to listOf("org.example.bar(kotlin.String)")))
).compileAndLintWithContext(env, code)
assertThat(findings).hasSize(1)
}
}
})
10 changes: 6 additions & 4 deletions detekt-tooling/src/main/kotlin/io/github/detekt/tooling/api/FunctionMatcher.kt
View file
Open in desktop
Expand Up @@ -35,8 +35,9 @@ sealed class FunctionMatcher {
override fun match(callableDescriptor: CallableDescriptor): Boolean {
if (callableDescriptor.fqNameOrNull()?.asString() != fullyQualifiedName) return false

val encounteredParamTypes = callableDescriptor.valueParameters
.map { it.type.fqNameOrNull()?.asString() }
val encounteredParamTypes =
(listOfNotNull(callableDescriptor.extensionReceiverParameter) + callableDescriptor.valueParameters)
.map { it.type.fqNameOrNull()?.asString() }

return encounteredParamTypes == parameters
}
Expand All @@ -45,8 +46,9 @@ sealed class FunctionMatcher {
if (bindingContext == BindingContext.EMPTY) return false
if (function.name != fullyQualifiedName) return false

val encounteredParameters = function.valueParameters
.map { bindingContext[BindingContext.TYPE, it.typeReference]?.fqNameOrNull()?.toString() }
val encounteredParameters =
(listOfNotNull(function.receiverTypeReference) + function.valueParameters.map { it.typeReference })
.map { bindingContext[BindingContext.TYPE, it]?.fqNameOrNull()?.toString() }

return encounteredParameters == parameters
}
Expand Down
60 changes: 60 additions & 0 deletions detekt-tooling/src/test/kotlin/io/github/detekt/tooling/api/FunctionMatcherSpec.kt
View file
Open in desktop
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are those single quotes around fun foo(a: String, ba: Int) required?

Copy link
Member Author

Choose a reason for hiding this comment

The 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 , . So, I need the single quotes.

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
}
}
}

Expand Down