Use reference in fallback property delegate #3982

marschwar · 2021-07-26T20:54:29Z

The fallback property syntax has always been awkward to use. Triggered by a missing feature for #3972 I wanted to find a way to

define fallback property without relying on a string matching some other property that is never used
allow for different transformation functions in the new and old property
reference the fallback property directly and not just by its name

~~Drawbacks of this solution:~~

~~The property to fall back onto has to be public~~
~~This solution relies on kotlin-reflectto be available at runtime (which looks like is the case since kotlin-compiler-embeddable depends on it)~~

BraisGabin

I can't download the code and test it myself but it's strange that you need to expose the deprecated properties. We should be able to avoid that, right?

detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/ConfigPropertySpec.kt

marschwar · 2021-07-27T15:55:24Z

I can't download the code and test it myself but it's strange that you need to expose the deprecated properties. We should be able to avoid that, right?

You can checkout to code using the github tooling gh pr checkout 3982.

There are 2 reasons why the deprecated properties must be there:

The documentation should include the property marked as deprecated.
In order to retrieve the (transformed) value, the property has to be there and also must be publicly accessible.

I am wondering if it safe to include the kotlin-reflect dependency or if there is a way around it. Any assistance on this is welcome. Please also read the PR description (if not already done). Thanks in advance.

codecov · 2021-07-27T18:48:02Z

Codecov Report

Merging #3982 (66ad476) into main (cc74174) will decrease coverage by 0.03%.
The diff coverage is 94.25%.

@@             Coverage Diff              @@
##               main    #3982      +/-   ##
============================================
- Coverage     83.60%   83.56%   -0.04%     
- Complexity     3187     3190       +3     
============================================
  Files           459      460       +1     
  Lines          9098     9108      +10     
  Branches       1769     1773       +4     
============================================
+ Hits           7606     7611       +5     
- Misses          560      563       +3     
- Partials        932      934       +2

Impacted Files	Coverage Δ
...ekt/generator/collection/ConfigurationCollector.kt	`74.73% <28.57%> (-3.16%)`	⬇️
.../io/gitlab/arturbosch/detekt/api/ConfigProperty.kt	`100.00% <100.00%> (ø)`
...ab/arturbosch/detekt/core/config/ValidateConfig.kt	`100.00% <100.00%> (ø)`
...bosch/detekt/rules/complexity/LongParameterList.kt	`86.66% <100.00%> (-1.09%)`	⬇️
...rturbosch/detekt/rules/empty/EmptyFunctionBlock.kt	`93.33% <100.00%> (ø)`
...sch/detekt/rules/naming/FunctionParameterNaming.kt	`90.90% <100.00%> (ø)`
...h/detekt/rules/naming/MemberNameEqualsClassName.kt	`84.21% <100.00%> (ø)`
...io/gitlab/arturbosch/detekt/generator/Generator.kt	`0.00% <0.00%> (ø)`
.../arturbosch/detekt/generator/out/AbstractWriter.kt	`0.00% <0.00%> (ø)`
...sch/detekt/generator/collection/DetektCollector.kt	`0.00% <0.00%> (ø)`
... and 13 more

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 b4c095e...66ad476. Read the comment docs.

marschwar · 2021-07-28T19:02:43Z

detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/ConfigPropertySpec.kt

+                        val fallback: String by config(42) { (it + fallbackOffset).toString() }
+                        val missing: String by config(42) { (it + fallbackOffset).toString() }
+                        val present: String by configWithFallback(::fallback, defaultValue) { v ->


This is very close to your use case @BraisGabin. Am I right?

BraisGabin · 2021-07-29T10:11:54Z

...r/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/ConfigurationCollector.kt

+            if (fallbackProperty.isPrivate()) {
+                invalidDocumentation {
+                    "The fallback property '$fallbackPropertyReference' of property '$name' may not be private"
                }


I played a bit with your PR and I still don't get why the fallback can't be private. I make one of them private and I commented this code and the tests and the generateDocumentation task worked fine.

You can change the visibility in line detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/ConfigPropertySpec.kt:300 to private and then the test will fail with:

java.lang.IllegalAccessException: class kotlin.reflect.jvm.internal.calls.CallerImpl$Method cannot access a member of class io.gitlab.arturbosch.detekt.api.ConfigPropertySpec$1$1$4$1$subject$2$1 with modifiers "private final"

The reason is that the FallbackConfigProperty tries to access the getter of the field via reflection which is not possible when it is private. That code path is only executed if:

the config does not contain the key of the new property (configured with configProperty(::xxx, 42))

AND the config contains the key of the old (deprecated) property

cortinico

Triggered by a missing feature

What is exactly the missing feature?
I don't have a strong opinion on this PR to be frank.
Just a small note, imho we're making the API safer but also harder to use.
Maybe we can provide methods with both String and KProperty instead? ~~of introducing a breaking change?~~

detekt-api/api/detekt-api.api

marschwar · 2021-08-01T17:34:26Z

What is exactly the missing feature?

Previously it was only possible to use the exact value from the deprecated property but not apply a function to its value.

marschwar · 2021-08-01T17:47:45Z

Just a small note, imho we're making the API safer but also harder to use.
Maybe we can provide methods with both String and KProperty instead?

I am a little bit weary to increase the api surface even further. Especially for such a functionality that is most likely not used very much. I am very much open to suggestions on how to implement the missing feature in a better way. As I wrote earlier, I do not really know if relying on reflection for this feature is such a great idea.

…ence # Conflicts: # gradle/libs.versions.toml

cortinico · 2021-08-06T18:13:29Z

How do we want to proceed? Ideally if we want to merge this, it should go inside 1.18.0

BraisGabin · 2021-08-07T12:12:08Z

Sorry, I didn't have the time to look at this until now. I did this:

marschwar/detekt@fallback-with-reference...BraisGabin:fallback-with-reference

It seems that now it allows private properties. The key is to use KtProperty0 instead of KtProperty because the first have a get() method that allow us to not call the getter that seems that is the one that uses reflection. (It may seems that I know what I'm talking about but I just tried and it worked).

marschwar · 2021-08-07T12:17:05Z

Thank you @BraisGabin. Let me look at it tonight. It seems like a good solution.

Fallback with reference

marschwar · 2021-08-07T19:16:00Z

/**
* Represents a property without any kind of receiver.
* Such property is either originally declared in a receiverless context such as a package,
* or has the receiver bound to it.
*/
public actual interface KProperty0<out V> : KProperty<V>, () -> V

I don't really understand why it is working either but it seems to do the trick. Thank you for looking into it.

chao2zhang · 2021-08-08T07:05:43Z

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/ConfigProperty.kt

+            return transform(getValueOrDefault(thisRef, property.name, defaultValue))
+        }
+        if (thisRef.isConfigured(fallbackProperty.name)) {
+            return fallbackProperty.get()


Question: If we specify fallback property and transformer at the same time, would the transformer be disregarded?

If the property exists in the config, the transformer is executed. If that is not the case and the fallback property exists, then its transformer (if present) is used.

marschwar · 2021-08-09T19:42:48Z

We could add @UnstableApi for now so this decision does not block the next release

BraisGabin · 2021-08-10T14:38:40Z

I'd vote to move this to 1.19.0 so we can release 1.18.0. Add this code after 3 RC doesn't feel the right move.

cortinico · 2021-08-11T10:09:04Z

I'd vote to move this to 1.19.0 so we can release 1.18.0. Add this code after 3 RC doesn't feel the right move.

Agree. Given the unstable annotation we added to those APIs, we can easily change it in the next release.

BraisGabin · 2021-09-11T13:55:05Z

Can we merge this now?

cortinico · 2021-09-13T12:47:06Z

Can we merge this now?

@marschwar Can you take a look at the merge conflicts? Then I belive we can merge this.

Markus Schwarz added 2 commits July 26, 2021 22:42

Use reference in fallback property delegate

238e547

api dump

2902a2c

BraisGabin reviewed Jul 27, 2021

View reviewed changes

detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/ConfigPropertySpec.kt Show resolved Hide resolved

marschwar mentioned this pull request Jul 27, 2021

UnderscoresInNumericLiterals acceptableDecimalLength is off by one #3972

Merged

Markus Schwarz added 2 commits July 27, 2021 20:37

Suppress deprecation warnings

7c6f712

Add kotlin-reflect to set of api dependencies

545e08b

Add test case for fallback transformation

663d3fa

marschwar commented Jul 28, 2021

View reviewed changes

marschwar marked this pull request as ready for review July 28, 2021 19:05

BraisGabin reviewed Jul 29, 2021

View reviewed changes

cortinico reviewed Aug 1, 2021

View reviewed changes

detekt-api/api/detekt-api.api Outdated Show resolved Hide resolved

Merge remote-tracking branch 'upstream/main' into fallback-with-refer…

c20f52d

…ence # Conflicts: # gradle/libs.versions.toml

BraisGabin added 2 commits August 7, 2021 14:06

Allow private fallback properties

f4a8afa

We shouldn't need reflection :)

93d66e8

Merge pull request #12 from BraisGabin/fallback-with-reference

652b7fb

Fallback with reference

api dump

85346ec

BraisGabin approved these changes Aug 7, 2021

View reviewed changes

marschwar force-pushed the fallback-with-reference branch 2 times, most recently from 8d3de2c to 3bf2c61 Compare August 7, 2021 19:48

remove check that fallback property must be private

3bf2c61

chao2zhang approved these changes Aug 8, 2021

View reviewed changes

cortinico approved these changes Aug 9, 2021

View reviewed changes

cortinico added the api label Aug 9, 2021

cortinico added this to the 1.18.0 milestone Aug 9, 2021

marschwar mentioned this pull request Aug 9, 2021

mark configWithFallback as unstable #4028

Merged

cortinico modified the milestones: 1.18.0, 1.19.0 Aug 11, 2021

cortinico added the notable changes Marker for notable changes in the changelog label Sep 13, 2021

Merge branch 'main' into fallback-with-reference

66ad476

marschwar merged commit 7962979 into detekt:main Sep 14, 2021

marschwar deleted the fallback-with-reference branch September 14, 2021 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use reference in fallback property delegate #3982

Use reference in fallback property delegate #3982

marschwar commented Jul 26, 2021 •

edited

Loading

BraisGabin left a comment

marschwar commented Jul 27, 2021

codecov bot commented Jul 27, 2021 •

edited

Loading

marschwar Jul 28, 2021

BraisGabin Jul 29, 2021

marschwar Jul 29, 2021

cortinico left a comment •

edited

Loading

marschwar commented Aug 1, 2021 •

edited

Loading

marschwar commented Aug 1, 2021

cortinico commented Aug 6, 2021

BraisGabin commented Aug 7, 2021

marschwar commented Aug 7, 2021

marschwar commented Aug 7, 2021

chao2zhang Aug 8, 2021

marschwar Aug 8, 2021

marschwar commented Aug 9, 2021

BraisGabin commented Aug 10, 2021

cortinico commented Aug 11, 2021

BraisGabin commented Sep 11, 2021

cortinico commented Sep 13, 2021

Use reference in fallback property delegate #3982

Use reference in fallback property delegate #3982

Conversation

marschwar commented Jul 26, 2021 • edited Loading

BraisGabin left a comment

Choose a reason for hiding this comment

marschwar commented Jul 27, 2021

codecov bot commented Jul 27, 2021 • edited Loading

Codecov Report

marschwar Jul 28, 2021

Choose a reason for hiding this comment

BraisGabin Jul 29, 2021

Choose a reason for hiding this comment

marschwar Jul 29, 2021

Choose a reason for hiding this comment

cortinico left a comment • edited Loading

Choose a reason for hiding this comment

marschwar commented Aug 1, 2021 • edited Loading

marschwar commented Aug 1, 2021

cortinico commented Aug 6, 2021

BraisGabin commented Aug 7, 2021

marschwar commented Aug 7, 2021

marschwar commented Aug 7, 2021

chao2zhang Aug 8, 2021

Choose a reason for hiding this comment

marschwar Aug 8, 2021

Choose a reason for hiding this comment

marschwar commented Aug 9, 2021

BraisGabin commented Aug 10, 2021

cortinico commented Aug 11, 2021

BraisGabin commented Sep 11, 2021

cortinico commented Sep 13, 2021

marschwar commented Jul 26, 2021 •

edited

Loading

codecov bot commented Jul 27, 2021 •

edited

Loading

cortinico left a comment •

edited

Loading

marschwar commented Aug 1, 2021 •

edited

Loading