-
-
Notifications
You must be signed in to change notification settings - Fork 758
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
Adding support for full method signatures in ForbiddenMethodCall #3505
Adding support for full method signatures in ForbiddenMethodCall #3505
Conversation
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.
Great work 💪 We need a bit of fine tuning and I think we should be able to merge this
val groups = METHOD_PARAM_REGEX.find(methodSignature)?.groups | ||
val methodName = groups?.get(METHOD_GROUP_NAME)?.value | ||
val params = groups?.get(METHOD_PARAM_GROUP_NAME)?.value?.split(",") | ||
?.map { it.trim() } | ||
?.filter { it.isNotBlank() } |
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.
Please replace with something like:
val groups = METHOD_PARAM_REGEX.find(methodSignature)?.groups | |
val methodName = groups?.get(METHOD_GROUP_NAME)?.value | |
val params = groups?.get(METHOD_PARAM_GROUP_NAME)?.value?.split(",") | |
?.map { it.trim() } | |
?.filter { it.isNotBlank() } | |
val groups = methodSignature.split('(',')') | |
val methodName = groups[0].trim() | |
val params = if (groups.size <= 1) { | |
null | |
} else { | |
groups[1].split(",") | |
.map(String::trim) | |
.filter(String::isNotBlank) | |
} |
Regexes are hard to maintain in the long run, especially for shared codebases like Detekt.
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.
If you wish, you can also create a util function inside the detekt-psi-utils
package and write a unit test (like a .parseFqNameAndParams()
or so).
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.
Probably, because it's not that easy because some functions can be called `like, this`(Int)
.
And, if we want to move out this code, it should be placed in tooling utils instead of psi utils, right? This code is to parse the configuration. It's not related with psi.
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.
it should be placed in tooling utils instead of psi utils, right?
I checked and detekt-psi-utils
is already imported inside the -rules
modules as an api
dependency. The detekt-tooling
is not and I don't think it's worth to add a module dependency for a function.
Not to be done in this PR, but I think we should rename detekt-psi-utils
to detekt-rules-utils
(given that inside there is also not PSI related utils) or create a separate detekt-rules-utils
just for those functions.
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.
Good idea. My first thought was to use regex and I was so fixed on this approach that I missed simpler one.
forbiddenMethods | ||
.filter { fqName == it.first } | ||
.forEach { | ||
if (it.second == null || it.second == methodParameterFqNames) { |
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.
To tackle the java.time.LocalDate.of(Int, Int, Int)
, you should make sure you run a param-by-param comparison of the two lists and check both the fully qualified and the simple name:
val configParams = it.second
val match = methodParameterFqNames.foldIndexed(true) { index: Int, acc: Boolean, param: String ->
acc && (param == configParams[index] || param.split('.').last() == configParams[index])
}
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 see one issue here. Won't we create inconsistency between how method should be defined (only fully qualified) and params (fully qualified or just class name)?
Other than that it seems ok, as it will allow you to shorten definition and only use qualified names when false-positives would be encountered otherwise.
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 see one issue here. Won't we create inconsistency between how method should be defined (only fully qualified) and params (fully qualified or just class name)?
Other than that it seems ok, as it will allow you to shorten definition and only use qualified names when false-positives would be encountered otherwise.
You're right. I think we could then revert to always be explicit. Maybe add a note example in the doc so users are aware on how to us it properly.
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.
Done.
83be499
to
12ab206
Compare
I have updated PR following your instructions. |
Codecov Report
@@ Coverage Diff @@
## master #3505 +/- ##
============================================
+ Coverage 77.62% 77.64% +0.01%
- Complexity 2806 2810 +4
============================================
Files 461 461
Lines 8672 8691 +19
Branches 1676 1683 +7
============================================
+ Hits 6732 6748 +16
Misses 1035 1035
- Partials 905 908 +3
Continue to review full report at Codecov.
|
Having the function extracted was more for testing reason. It's easier to write a couple of unit tests in a separate module for just that function. Anyway that's not a hard requirement for merging this PR. |
7c95ca9
to
527dede
Compare
@@ -8,6 +8,8 @@ import io.gitlab.arturbosch.detekt.api.Issue | |||
import io.gitlab.arturbosch.detekt.api.Rule | |||
import io.gitlab.arturbosch.detekt.api.Severity | |||
import io.gitlab.arturbosch.detekt.api.internal.valueOrDefaultCommaSeparated | |||
import io.gitlab.arturbosch.detekt.rules.extractMethodNameAndParams | |||
import org.jetbrains.kotlin.js.descriptorUtils.getJetTypeFqName |
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.
Not necessarily an issue for this PR, because getJetTypeFqName
is used at several places. However, the package path looks very weird because js
appears.
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.
As I mentioned it is my first meeting with kotlin reflection and it was the solution I have found but if you have some idea how to achieve it with other method I will be happy to try that out.
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 can investigate further on why this is needed. I don't think this would block merging this PR
What do we think about methods with default arguments? If (This PR looks good in general, this might be an edge case that we can follow up) |
527dede
to
aab67c1
Compare
As for the methods with default parameters I think we should report them all, because otherwise if we would like to forbid method with more than one default parameter, we would have to cover or possible combinations in configuration. I have added test case with default parameter. It worked without changes in the rule logic. |
Thanks for approval. If I can do anything else to help you with this PR please let me know! |
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.
If @BraisGabin approves, I think we can merge this.
Thanks for this awesome PR. It's a nice improvement of this rule.
Great work! |
No problem. I'm happy that I could have helped. |
This PR is for now a proof of concept for #3498.
Namely it's adding support for full method signatures (like
java.time.LocalDate.now(java.time.Clock)
in ForbiddenMethodCall rule.