Better handling for the Suppresion of errors

[BraisGabin]

I added a regresion with #1977. Luckly it was not released yet.

enumEntry.nameIdentifier is not a KtElement so it's not setted in the Entity.ktElement. Right now the only use of that the KtElement is to check if there is a @Suppress annotation. So if it was null, you can't @Suppress. With this PR I fix that.

I'm fixing more than that so, please, check if this is correct. What I'm doing is that, instead of checking if the current element is a KtElement I look for the nearest KtElement parent. This way, other warnings as SpacingBetweenPackageAndImports can use @Supress too.

Maybe I'm breaking the API for the plugins. If you think so, I can implement this without any api change.

[codecov-io]

Codecov Report

Merging #2013 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #2013      +/-   ##
============================================
+ Coverage     80.71%   80.71%   +<.01%     
  Complexity     1985     1985              
============================================
  Files           329      329              
  Lines          5590     5591       +1     
  Branches       1021     1020       -1     
============================================
+ Hits           4512     4513       +1     
  Misses          539      539              
  Partials        539      539

Impacted Files	Coverage Δ	Complexity Δ
...n/kotlin/io/gitlab/arturbosch/detekt/api/Entity.kt	92.3% <100%> (+0.64%)	5 <0> (ø)	⬇️

---

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 b048c6b...1b9ee32. Read the comment docs.

[arturbosch]

Looks good to me.
I guess the new function could be element.getNonStrictParentOfType<KtElement>()
Could you add a testcase for SpacingBetweenPackageAndImports. I'm not sure how the Psi mixes the different kind of elements and if we really can expect a LeafPsiElement.parent to reach a KtElement.

[BraisGabin]

I can't implement something like element.getNonStrictParentOfType<KtElement>() because to do so I need to inline the function. But the function is recursive so it can't be inlined. Well, the truth is that, because is tailrec, it should be elegible to inline but it's not right now. Bug here: https://youtrack.jetbrains.com/issue/KT-27897

But I renamed the function to: getNonStrictParentOfTypeKtElement.

I debugged the SpacingBetweenPackageAndImports case and yes, it finds the KtFile. The thing is: What do you want me to test exactly?

Reading this: https://www.jetbrains.org/intellij/sdk/docs/basics/architectural_overview/psi_files.html the PsiFile is always the root of the tree. If that's true and detekt is a tool for Kotlin we can even ensure that this function is going to return always a non-null value, at least we will get the KtFile. So, what do you think about this? (I did the change already in the code)

---

As a side effect we could change Entity.ktElement to not nullable. The only place where we are setting it as null are tests and in the FormattingRule. And in this last case we have the reference of the KtFile near, so we can use it.

[BraisGabin]

🤦‍♂ I just realised that the function getNonStrictParentOfType already exist. I just changed to use it.

[BraisGabin]

I think that the fails of this PR was a problem in the travis end.

[arturbosch]

I would let the KtElement inside a Finding to be nullable.
A KtFile should really only be returned if the rule is about the file itself, for example a custom TooBigFile rule.
For formatting rules for example there is no need to "postprocess" the Finding via a KtElement.

element?.getNonStrictParentOfType<KtElement>() should do it (@schalkms ).

[BraisGabin]

Right now Entity.from forces you to send a non-null element so we don't need to use ?..

Move Entity.ktElement to non-null was just an idea, not related with this PR. We can move this discussion to an issue if we want to talk more about it.

The only "problem" in this PR is this line:

val ktElement = element.getNonStrictParentOfType<KtElement>()!!

The use of !! is not needed but reading the Psi documentation it should never fail. This will help to move Entity.ktElement to not-null in the future if we want to. I don't know the "rules of the library" in this cases: we can be safe or follow the documentation and force the non-null.

[arturbosch]

Right now Entity.from forces you to send a non-null element so we don't need to use ?..

Move Entity.ktElement to non-null was just an idea, not related with this PR. We can move this discussion to an issue if we want to talk more about it.

The only "problem" in this PR is this line:

val ktElement = element.getNonStrictParentOfType<KtElement>()!!

The use of !! is not needed but reading the Psi documentation it should never fail. This will help to move Entity.ktElement to not-null in the future if we want to. I don't know the "rules of the library" in this cases: we can be safe or follow the documentation and force the non-null.

Ah right!
Hm the travis is realy not related.
Edit: It's due to a change in cyclomatic complexity we made for ComplexMethod.