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.

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

Fixed ProtectedMemberInFinalClass rule reporting valid JVM finalize #5788

Merged

Conversation

lexa-diky
Copy link
Contributor

Fixes #5660

Added check that declaration does not match JVM finalize signature:

name == "finalize"
parameters are empty

@github-actions github-actions bot added the rules label Feb 14, 2023
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for this nice 1st contribution and for fixing this scenario!

@@ -10,6 +11,9 @@ fun KtFunction.isEqualsFunction() =
fun KtFunction.isHashCodeFunction() =
this.name == "hashCode" && this.isOverride() && this.valueParameters.isEmpty()

fun KtDeclaration.isJvmFinalizeFunction() =
Copy link
Member

Choose a reason for hiding this comment

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

Please use KtFunction. here and also check for the function being overridden. Then this function is also reusable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok will make isJvmFinalizeFunction take KtFunction as context. But can't make it check if function is overridden, because official Kotlin documentation state that you don't have to specify override keyword explicitly.

https://kotlinlang.org/docs/java-interop.html#finalize

class C {
    protected fun finalize() {
        // finalization logic
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Don't check that the function has the keyword. Check that the function doesn't have it, as the documentation sais. And also check that it's not private if possible. And if possible add a comment with the link to the documentation so we have a reference for the future.

With that I think this PR is good to go :) Thanks for the contribution!

Copy link
Contributor Author

@lexa-diky lexa-diky Feb 21, 2023

Choose a reason for hiding this comment

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

Oh, probably was a bit tired writing this comment :). Yes of course we should check for !isOverride. Pushed changes according changes.

P.S. Fun thing finalize function is not mentioned in Kotlin Language specification 😞, only in doc (there is antlr tree sample, that's all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like Pre Merge Checks / gradle (windows-latest, 11) (pull_request) is failed because of flaky test. Could, please, someone restart it.

Copy link
Member

Choose a reason for hiding this comment

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

We know that we have flaky test on windows. This is ready to merge :) thank you again for the PR!

@codecov
Copy link

codecov bot commented Feb 18, 2023

Codecov Report

Merging #5788 (1c972f1) into main (5d45cba) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #5788      +/-   ##
============================================
+ Coverage     84.48%   84.59%   +0.11%     
- Complexity     3743     3790      +47     
============================================
  Files           546      547       +1     
  Lines         12788    12914     +126     
  Branches       2230     2267      +37     
============================================
+ Hits          10804    10925     +121     
+ Misses          869      862       -7     
- Partials       1115     1127      +12     
Impacted Files Coverage Δ
.../gitlab/arturbosch/detekt/rules/MethodSignature.kt 75.00% <100.00%> (+5.00%) ⬆️
.../detekt/rules/style/ProtectedMemberInFinalClass.kt 91.66% <100.00%> (+0.36%) ⬆️
...io/gitlab/arturbosch/detekt/core/rules/RuleSets.kt 89.65% <0.00%> (-3.45%) ⬇️
...ls/src/main/kotlin/io/github/detekt/psi/KtFiles.kt 74.35% <0.00%> (-2.57%) ⬇️
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 89.90% <0.00%> (-2.32%) ⬇️
...io/gitlab/arturbosch/detekt/core/KtTreeCompiler.kt 77.41% <0.00%> (-1.37%) ⬇️
...gitlab/arturbosch/detekt/core/config/YamlConfig.kt 87.87% <0.00%> (-1.02%) ⬇️
...ules/documentation/LicenceHeaderLoaderExtension.kt 81.39% <0.00%> (-0.83%) ⬇️
.../main/kotlin/io/github/detekt/parser/KtCompiler.kt 80.00% <0.00%> (ø)
...in/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt 100.00% <0.00%> (ø)
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive in ProtectedMemberInFinalClass with JVM finalizer
3 participants