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

ForbiddenAnnotation rule #2719 #5515

Merged
merged 20 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,23 @@ style:
ExpressionBodySyntax:
active: false
includeLineWrapping: false
ForbiddenAnnotation:
active: false
annotations:
- reason: 'it is a java annotation. Use `Suppress` instead.'
value: 'java.lang.SuppressWarnings'
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
- reason: 'it is a java annotation. Use `kotlin.Deprecated` instead.'
value: 'java.lang.Deprecated'
- reason: 'it is a java annotation. Use `kotlin.annotation.MustBeDocumented` instead.'
value: 'java.lang.annotation.Documented'
- reason: 'it is a java annotation. Use `kotlin.annotation.Target` instead.'
value: 'java.lang.annotation.Target'
- reason: 'it is a java annotation. Use `kotlin.annotation.Retention` instead.'
value: 'java.lang.annotation.Retention'
- reason: 'it is a java annotation. Use `kotlin.annotation.Repeatable` instead.'
value: 'java.lang.annotation.Repeatable'
- reason: 'Kotlin does not support @Inherited annotation, see https://youtrack.jetbrains.com/issue/KT-22265'
value: 'java.lang.annotation.Inherited'
ForbiddenComment:
active: true
values:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Debt
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Location
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import io.gitlab.arturbosch.detekt.api.internal.RequiresTypeResolution
import io.gitlab.arturbosch.detekt.api.valuesWithReason
import io.gitlab.arturbosch.detekt.rules.fqNameOrNull
import org.jetbrains.kotlin.name.FqName
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtTypeReference
import org.jetbrains.kotlin.psi.psiUtil.endOffset
import org.jetbrains.kotlin.resolve.BindingContext
import org.jetbrains.kotlin.types.KotlinType

/**
* This rule allows to set a list of forbidden annotations. This can be used to discourage the use
* of language annotations which do not require explicit import.
*
* <noncompliant>
* @@SuppressWarnings("unused")
* class SomeClass()
* </noncompliant>
Comment on lines +30 to +33
Copy link
Member

Choose a reason for hiding this comment

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

I would keep both a compliant and a noncompliant block here.
Maybe show the @Deprecated one or others

*
* <compliant>
* @@Suppress("unused")
* class SomeClass()
* </compliant>
*/
@RequiresTypeResolution
class ForbiddenAnnotation(config: Config = Config.empty) : Rule(config) {

override val issue = Issue(
javaClass.simpleName,
Severity.Style,
"Avoid using this annotation.",
Debt.FIVE_MINS
)

@Configuration(
"List of fully qualified annotation classes which are forbidden. " +
"For example, `kotlin.jvm.Transient`."
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved
)
private val annotations: Map<String, Forbidden> by config(
valuesWithReason(
"java.lang.SuppressWarnings" to "it is a java annotation. Use `Suppress` instead.",
"java.lang.Deprecated" to "it is a java annotation. Use `kotlin.Deprecated` instead.",
"java.lang.annotation.Documented" to "it is a java annotation. " +
"Use `kotlin.annotation.MustBeDocumented` instead.",
"java.lang.annotation.Target" to "it is a java annotation. Use `kotlin.annotation.Target` instead.",
"java.lang.annotation.Retention" to "it is a java annotation. Use `kotlin.annotation.Retention` instead.",
"java.lang.annotation.Repeatable" to "it is a java annotation. Use `kotlin.annotation.Repeatable` instead.",
"java.lang.annotation.Inherited" to "Kotlin does not support @Inherited annotation, " +
"see https://youtrack.jetbrains.com/issue/KT-22265",
)
) { list ->
list.associate { it.value to Forbidden(it.value, it.reason) }
}
TWiStErRob marked this conversation as resolved.
Show resolved Hide resolved

override fun visitAnnotationEntry(annotation: KtAnnotationEntry) {
super.visitAnnotationEntry(annotation)
if (annotations.isEmpty()) {
return
}

annotation.typeReference?.fqNameOrNull()?.let {
check(annotation, it)
}
}

override fun visitExpression(expression: KtExpression) {
super.visitExpression(expression)

if (annotations.isEmpty()) {
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this permature optimization? (same in above method) why would this rule be active if there are no configured annotations. @BraisGabin /@cortinico is there a way to easily check this during configuration phase? Can ConfigValidator be on a rule level? (btw. ConfigValidator has no docs.)

Copy link
Member

Choose a reason for hiding this comment

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

I agree, there's not need for this if. I don't care that much about it either... but if we have this ifs we should also have a test to check what happen with annotations is empty. So I would say that it's better to remove it. But if you, @ov7a, prefer to keep it just add a test for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just removed it. I agree with @TWiStErRob that it would be nice to have some example of config validation.

expression.expressionTypeOrNull()?.fqNameOrNull()?.let {
check(expression, it)
}
}

private fun check(element: KtElement, fqName: FqName) {
val forbidden = annotations[fqName.asString()]

if (forbidden != null) {
val message = if (forbidden.reason != null) {
"The annotation `${forbidden.name}` has been forbidden: ${forbidden.reason}"
} else {
"The annotation `${forbidden.name}` has been forbidden in the detekt config."
}
val location = Location.from(element).let { location ->
location.copy(
text = location.text.copy(
end = element.children.firstOrNull()?.endOffset ?: location.text.end
)
)
}
report(CodeSmell(issue, Entity.from(element, location), message))
}
}

private data class Forbidden(val name: String, val reason: String?)

private fun KtTypeReference.fqNameOrNull(): FqName? {
return if (bindingContext != BindingContext.EMPTY) {
bindingContext[BindingContext.TYPE, this]?.fqNameOrNull()
} else {
null
}
}

private fun KtExpression.expressionTypeOrNull(): KotlinType? {
return if (bindingContext != BindingContext.EMPTY) {
bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type
} else {
null
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@RequiresTypeResolution guarantees non-empty binding context. @BraisGabin this looks like another one for the rule-authors ruleset.

Suggested change
private fun KtTypeReference.fqNameOrNull(): FqName? {
return if (bindingContext != BindingContext.EMPTY) {
bindingContext[BindingContext.TYPE, this]?.fqNameOrNull()
} else {
null
}
}
private fun KtExpression.expressionTypeOrNull(): KotlinType? {
return if (bindingContext != BindingContext.EMPTY) {
bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type
} else {
null
}
}
private fun KtTypeReference.fqNameOrNull(): FqName? =
bindingContext[BindingContext.TYPE, this]?.fqNameOrNull()
private fun KtExpression.expressionTypeOrNull(): KotlinType? =
bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type

Copy link
Member

Choose a reason for hiding this comment

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

leaving it open, curious what @BraisGabin thinks

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that any check of bindingContext != BindingContext.EMPTY or bindingContext == BindingContext.EMPTY should be flagged as a code smell. That should be an easy rule to implement. @TWiStErRob do you want to create the issue for it (or the PR)

}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
NoTabs(config),
EqualsOnSignatureLine(config),
EqualsNullCall(config),
ForbiddenAnnotation(config),
ForbiddenComment(config),
ForbiddenImport(config),
ForbiddenMethodCall(config),
Expand Down