Match extension functions #4459

BraisGabin · 2022-01-06T12:30:45Z

More context here: #4448 (comment) I'm implementing the no breaking option. It isn't nice but it works.

Related with #4448 but this PR doesn't fix the issue yet.

codecov · 2022-01-06T12:38:01Z

Codecov Report

Merging #4459 (6a9f0ca) into main (1ca3ab5) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@            Coverage Diff            @@
##               main    #4459   +/-   ##
=========================================
  Coverage     84.13%   84.13%           
  Complexity     3313     3313           
=========================================
  Files           477      477           
  Lines         10922    10924    +2     
  Branches       2030     2030           
=========================================
+ Hits           9189     9191    +2     
  Misses          700      700           
  Partials       1033     1033

Impacted Files	Coverage Δ
...turbosch/detekt/rules/style/ForbiddenMethodCall.kt	`88.57% <ø> (ø)`
...in/io/github/detekt/tooling/api/FunctionMatcher.kt	`86.76% <66.66%> (+0.40%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ca3ab5...6a9f0ca. Read the comment docs.

BraisGabin · 2022-01-06T14:36:12Z

I have no idea what's going on here: https://github.com/detekt/detekt/runs/4726908840?check_suite_focus=true#step:4:499

I didn't change that class at all. Well, I did change it, but one year ago: 04/01/2021

chao2zhang · 2022-01-26T06:59:53Z

...t-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/ForbiddenMethodCall.kt

@@ -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: " +


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.

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.

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?

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.

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

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

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.

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.

BraisGabin · 2022-03-20T16:36:44Z

Can we take a decission here?

chao2zhang · 2022-03-20T18:21:44Z

Would this work across all Kotlin Multiplatform? I would vote for merge if this is not particularly tailored to JVM

BraisGabin · 2022-03-21T11:35:44Z

Would this work across all Kotlin Multiplatform? I would vote for merge if this is not particularly tailored to JVM

Yes, it should work for all of them.

cortinico · 2022-03-23T01:14:36Z

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.

☝️ That's my latest opinion. I'm in for merging this

chao2zhang · 2022-03-24T03:44:43Z

detekt-tooling/src/test/kotlin/io/github/detekt/tooling/api/FunctionMatcherSpec.kt

+            "fun foo(a: String),            true",
+            "fun Int.foo(),                 false",
+            "fun String.foo(a: Int),        false",
+            "'fun foo(a: String, ba: Int)', false",


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

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.

you could use a different separator though (Random guy yelling from the side line) 😉

* Add missing tests * Match extension functions

BraisGabin added this to the 1.20.0 milestone Jan 6, 2022

BraisGabin marked this pull request as draft January 8, 2022 11:48

chao2zhang self-requested a review January 23, 2022 20:19

Base automatically changed from function-matcher to main January 25, 2022 08:41

BraisGabin force-pushed the function-extension-matcher branch from f73771a to 5d8588f Compare January 25, 2022 08:44

BraisGabin marked this pull request as ready for review January 25, 2022 08:44

BraisGabin mentioned this pull request Jan 25, 2022

Migrate tests in detekt-rules-errorprone to junit #4523

Merged

chao2zhang reviewed Jan 26, 2022

View reviewed changes

BraisGabin force-pushed the function-extension-matcher branch from 5d8588f to dc617af Compare January 30, 2022 11:47

BraisGabin added 2 commits January 31, 2022 09:40

Add missing tests

2b830bb

Match extension functions

6a9f0ca

BraisGabin force-pushed the function-extension-matcher branch from dc617af to 6a9f0ca Compare January 31, 2022 08:40

chao2zhang approved these changes Mar 24, 2022

View reviewed changes

BraisGabin merged commit d7ac501 into main Mar 24, 2022

BraisGabin deleted the function-extension-matcher branch March 24, 2022 07:54

BraisGabin mentioned this pull request Mar 24, 2022

Function generics matcher #4460

Merged

cortinico added the notable changes Marker for notable changes in the changelog label Mar 24, 2022

chao2zhang pushed a commit that referenced this pull request Apr 8, 2022

Match extension functions (#4459)

23ab86e

* Add missing tests * Match extension functions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match extension functions #4459

Match extension functions #4459

BraisGabin commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

BraisGabin commented Jan 6, 2022

chao2zhang Jan 26, 2022

BraisGabin Jan 26, 2022

cortinico Jan 30, 2022

BraisGabin Jan 31, 2022 •

edited

Loading

cortinico Jan 31, 2022

BraisGabin Feb 1, 2022

BraisGabin Feb 3, 2022

cortinico Feb 4, 2022

BraisGabin commented Mar 20, 2022

chao2zhang commented Mar 20, 2022

BraisGabin commented Mar 21, 2022

cortinico commented Mar 23, 2022

chao2zhang Mar 24, 2022

BraisGabin Mar 24, 2022

marschwar Mar 24, 2022

Match extension functions #4459

Match extension functions #4459

Conversation

BraisGabin commented Jan 6, 2022 • edited Loading

codecov bot commented Jan 6, 2022 • edited Loading

Codecov Report

BraisGabin commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin commented Mar 20, 2022

chao2zhang commented Mar 20, 2022

BraisGabin commented Mar 21, 2022

cortinico commented Mar 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin commented Jan 6, 2022 •

edited

Loading

codecov bot commented Jan 6, 2022 •

edited

Loading

BraisGabin Jan 31, 2022 •

edited

Loading