Allow to suppress issues if they are found in a function with a determined name #4148

BraisGabin · 2021-10-02T13:29:07Z

There are places where you don't care about an issue inside a determined function. One example is #3855

codecov · 2021-10-02T13:40:01Z

Codecov Report

Merging #4148 (a05c230) into main (c66b8d6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4148      +/-   ##
============================================
+ Coverage     84.22%   84.24%   +0.01%     
  Complexity     3258     3258              
============================================
  Files           472      473       +1     
  Lines         10309    10322      +13     
  Branches       1820     1824       +4     
============================================
+ Hits           8683     8696      +13     
  Misses          666      666              
  Partials        960      960

Impacted Files	Coverage Δ
...ab/arturbosch/detekt/core/config/ValidateConfig.kt	`100.00% <100.00%> (ø)`
...osch/detekt/core/suppressors/FunctionSuppressor.kt	`100.00% <100.00%> (ø)`
...b/arturbosch/detekt/core/suppressors/Suppressor.kt	`71.42% <100.00%> (+2.19%)`	⬆️

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 c66b8d6...a05c230. Read the comment docs.

chao2zhang · 2021-10-03T22:45:35Z

This would address the other part of #4080.

chao2zhang

Thanks for the test coverage.

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/Suppressors.kt

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressor.kt

BraisGabin · 2021-10-09T14:08:34Z

I'm moving this PR back to draft because I need #4176 merged first.

BraisGabin · 2021-10-09T14:10:52Z

...-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressorSpec.kt

+            }
+        }
+    }
+})


I'm not adding any new test for the cases where you define parameters because all those cases are already tested in FunctionSignatureSpec already. Here I'm basically testing that the scope of the suppression is correct.

cortinico · 2021-10-15T23:04:12Z

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressor.kt

+
+internal fun functionSuppressorFactory(rule: ConfigAware, bindingContext: BindingContext): Suppressor? {
+    val signatures = rule.valueOrDefault("ignoreFunction", emptyList<String>()).map(FunctionSignature::fromString)
+    return if (signatures.isNotEmpty()) {


Should you check here if the BindingContext is empty?

🤔 yes and no. If you uses just the name of the function to suppress it we don't use the BindingContext. For that reason I don't check it. But maybe this is adding a new "mixed behaviour" that we don't want.

cortinico · 2021-10-15T23:07:34Z

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt

@@ -120,7 +120,7 @@ internal class Analyzer(
        fun executeRules(rules: List<BaseRule>) {
            for (rule in rules) {
                rule.visitFile(file, bindingContext, compilerResources)
-                for (finding in filterSuppressedFindings(rule)) {
+                for (finding in filterSuppressedFindings(rule, bindingContext)) {


I just have one concern here: is this going to work for users that are running without type resolution?

If not, we should be aware that this will potentially create even more confusion (or maybe will push more people into looking into type resolution 🤔 ).

The suppressors are not directly related with type solving. There are some suppressors that work without it as ignoreAnnotated other mixed as the one that this PR implements and maybe there will be others that only works with type resolution.

There are some suppressors that work without it as ignoreAnnotated other mixed as the one that this PR implements and maybe there will be others that only works with type resolution.

How do we let the users know which suppressors works with TR or not (similarly to @RequiresTypeResolution)?

I don't know how are we going to document this feature. #4162 would help with the discoverability but probably we will need a new page to talk about Suppressors.

chao2zhang · 2021-11-17T15:45:41Z

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressor.kt

+private fun KtElement.isInFunctionNamed(
+    bindingContext: BindingContext,
+    functionMatchers: List<FunctionMatcher>,
+): Boolean {
+    return if (this is KtNamedFunction && functionMatchers.any { it.match(this, bindingContext) }) {
+        true
+    } else {
+        getStrictParentOfType<KtNamedFunction>()?.isInFunctionNamed(bindingContext, functionMatchers) ?: false
+    }
+}


Suggested change

private fun KtElement.isInFunctionNamed(

bindingContext: BindingContext,

functionMatchers: List<FunctionMatcher>,

): Boolean {

return if (this is KtNamedFunction && functionMatchers.any { it.match(this, bindingContext) }) {

true

} else {

getStrictParentOfType<KtNamedFunction>()?.isInFunctionNamed(bindingContext, functionMatchers) ?: false

}

}

private fun KtElement.isInFunctionNamed(

bindingContext: BindingContext,

functionMatchers: List<FunctionMatcher>,

): Boolean =

getParentOfType<KtNamedFunction>(false)?.let { function ->

functionMatchers.any { it.match(function, bindingContext) }

} ?: false

What do you think about writing in this way to avoid recursion? We can even inline this function.

You can have a function inside another function. I prefer the recursive version. But I should add a test to demostraste that intention or any one could refactor this code later.

I see your intention and it makes sense. It does not feel like the code in the existing implementation addresses the case where there is function nested inside another function though.

It work. I added the tests and it works with nested functions :)

After a second thought, you are right. My mind couldn't keep up with code today

chao2zhang · 2021-11-17T15:49:11Z

...-core/src/test/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressorSpec.kt

+                    """.trimIndent()
+                )
+            }
+            val binding by memoized { env.getContextForPaths(listOf(root)) }


Just to confirm, we can use BindingContext.EMPTY and the tests would still pass, because we don't require type resolution. Is that correct?

Good point! It depends. If you only match using the name you don't need the BindingContext but if you want to match the parameters you need the BindingContext. I'm going to add some tests to demostrate this.

Ups! I just reread my own comment: https://github.com/detekt/detekt/pull/4148/files/bda4de4f0177e4493dbd7af7019d209df2ae29fe#r725492307 That behaviour is tested on FunctionSignatureSpec.

I'm conflicted here. I want to write "functional-ish code". But if I do so the tests end up beening a bit crapy. I can't create a fake/mock of FunctionSignature and inject it.

My assumption is that, if a feature does not require type resolution, we should write our tests as not requiring type resolution.

I understand your comment now. You are completely right! I'm using now BindingContext.EMPTY

This was referenced Oct 2, 2021

Add regexp to MagicNumber rule #4080

Closed

Implement a more advanced way to ignore/suppress issues #4068

Closed

chao2zhang reviewed Oct 3, 2021

View reviewed changes

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/Suppressors.kt Outdated Show resolved Hide resolved

detekt-core/src/main/kotlin/io/gitlab/arturbosch/detekt/core/suppressors/FunctionSuppressor.kt Outdated Show resolved Hide resolved

chao2zhang added this to the 1.19.0 milestone Oct 3, 2021

BraisGabin mentioned this pull request Oct 9, 2021

Define FunctionSignature #4176

Merged

BraisGabin changed the base branch from main to pre-fix-3855 October 9, 2021 14:06

BraisGabin marked this pull request as draft October 9, 2021 14:07

BraisGabin force-pushed the fix-3855 branch from de95a39 to 4d35728 Compare October 9, 2021 14:08

BraisGabin commented Oct 9, 2021

View reviewed changes

BraisGabin force-pushed the pre-fix-3855 branch from 6db005f to bde99ef Compare October 9, 2021 14:14

BraisGabin force-pushed the fix-3855 branch from 4d35728 to 8645ee5 Compare October 9, 2021 14:15

BraisGabin force-pushed the pre-fix-3855 branch 2 times, most recently from f71d8a5 to 800d094 Compare October 11, 2021 09:15

cortinico reviewed Oct 15, 2021

View reviewed changes

BraisGabin force-pushed the pre-fix-3855 branch from 800d094 to 5631bf0 Compare October 18, 2021 07:39

BraisGabin force-pushed the fix-3855 branch from 8645ee5 to 5cc97fb Compare October 18, 2021 08:17

Base automatically changed from pre-fix-3855 to main November 6, 2021 23:54

BraisGabin force-pushed the fix-3855 branch from 5cc97fb to b74d3f0 Compare November 6, 2021 23:59

BraisGabin marked this pull request as ready for review November 7, 2021 00:01

BraisGabin mentioned this pull request Nov 8, 2021

LongMethod excludes functionality or global excludes config rule definition #3251

Closed

BraisGabin force-pushed the fix-3855 branch 2 times, most recently from a07caeb to bda4de4 Compare November 16, 2021 10:08

chao2zhang approved these changes Nov 17, 2021

View reviewed changes

chao2zhang changed the title ~~Allow to suppress issues if they are find in a function with a determined name~~ Allow to suppress issues if they are found in a function with a determined name Nov 17, 2021

BraisGabin added 3 commits November 17, 2021 18:49

Move builders outside AnnotationSuppressorSpec

fce539b

Implement FunctionSuppressor

ce3bbfb

Add ContextBinding to FunctionSuppressor

d963055

BraisGabin added 3 commits November 17, 2021 18:49

Use FunctionSignature instead

6238e20

Add test to ensure nested functions work

d2050bb

We don't need real Binding for these tests

a05c230

BraisGabin force-pushed the fix-3855 branch from bda4de4 to a05c230 Compare November 17, 2021 17:57

chao2zhang approved these changes Nov 17, 2021

View reviewed changes

BraisGabin merged commit 5fa01a9 into main Nov 23, 2021

BraisGabin deleted the fix-3855 branch November 23, 2021 17:32

cortinico added the notable changes Marker for notable changes in the changelog label Nov 23, 2021

schalkms mentioned this pull request Jan 2, 2022

Add list of functions to skip in IgnoredReturnValue rule #4434

Merged

TWiStErRob added the suppressors label Nov 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to suppress issues if they are found in a function with a determined name #4148

Allow to suppress issues if they are found in a function with a determined name #4148

BraisGabin commented Oct 2, 2021

codecov bot commented Oct 2, 2021 •

edited

Loading

chao2zhang commented Oct 3, 2021

chao2zhang left a comment

BraisGabin commented Oct 9, 2021

BraisGabin Oct 9, 2021

cortinico Oct 15, 2021

BraisGabin Oct 18, 2021

cortinico Oct 15, 2021

BraisGabin Oct 18, 2021

cortinico Oct 18, 2021

BraisGabin Oct 18, 2021

chao2zhang Nov 17, 2021

chao2zhang Nov 17, 2021

BraisGabin Nov 17, 2021

chao2zhang Nov 17, 2021

BraisGabin Nov 17, 2021

chao2zhang Nov 17, 2021

chao2zhang Nov 17, 2021

BraisGabin Nov 17, 2021

BraisGabin Nov 17, 2021

chao2zhang Nov 17, 2021

BraisGabin Nov 17, 2021

Allow to suppress issues if they are found in a function with a determined name #4148

Allow to suppress issues if they are found in a function with a determined name #4148

Conversation

BraisGabin commented Oct 2, 2021

codecov bot commented Oct 2, 2021 • edited Loading

Codecov Report

chao2zhang commented Oct 3, 2021

chao2zhang left a comment

Choose a reason for hiding this comment

BraisGabin commented Oct 9, 2021

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 2, 2021 •

edited

Loading