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.

Sign up for GitHub

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

Jump to bottom

Suppress remove curly braces from template #157 #263

Merged
merged 12 commits into from
Nov 11, 2019
Merged

Suppress remove curly braces from template #157 #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5ec6dbd
WIP
AleksandrSl Apr 1, 2018
035685f
WIP
AleksandrSl Apr 1, 2018
ef90195
Implement top-down scenario
AleksandrSl Apr 2, 2018
ca32657
Fix cherry-pick consequences
AleksandrSl Jul 28, 2018
bb39bde
Simplify code
AleksandrSl Jul 29, 2018
fca3b6c
Regard SuppressWarnings also
AleksandrSl Jul 30, 2018
b7733b5
Clean tests
AleksandrSl May 3, 2019
a2a4ab1
Remove unused import from KtLint.kt
AleksandrSl May 3, 2019
0f000f9
Cleanup suppress annotations related code
AleksandrSl May 3, 2019
fe8e310
Clarify test meaning for suppress annotations
AleksandrSl May 3, 2019
4ee8253
Fix typo in comments
AleksandrSl May 17, 2019
a0f3a4b
Improve code readability by extracting function for suppress annotati…
Nov 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions ktlint-core/src/main/kotlin/com/pinterest/ktlint/core/KtLint.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,11 @@ import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.TreeCopyHandler
import org.jetbrains.kotlin.config.CompilerConfiguration
import org.jetbrains.kotlin.idea.KotlinLanguage
import org.jetbrains.kotlin.psi.KtAnnotated
import org.jetbrains.kotlin.psi.KtAnnotationEntry
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.psiUtil.endOffset
import org.jetbrains.kotlin.psi.psiUtil.startOffset
import sun.reflect.ReflectionFactory

object KtLint {
Expand Down Expand Up @@ -466,6 +470,14 @@ object KtLint {
}
}
}
// Extract all Suppress annotations and create SuppressionHints
val psi = node.psi
if (psi is KtAnnotated) {
createSuppressionHintFromAnnotations(psi, suppressAnnotations, suppressAnnotationRuleMap)
?.let {
result.add(it)
}
}
}
result.addAll(
open.map {
Expand All @@ -490,6 +502,42 @@ object KtLint {
comment.replace(Regex("\\s"), " ").replace(" {2,}", " ").split(" ")

private fun <T> List<T>.tail() = this.subList(1, this.size)

private val suppressAnnotationRuleMap = mapOf(
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 think this map could be moved to some config file, to ease addition of new suppression
rules

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure about having this mapping:

  • "RemoveCurlyBracesFromTemplate" just becomes an alias to "string-template" rule
  • This alias is detached from the rule itself
  • Overall this PR goes into direction of adding another way for suppress rules

To be consistent, I would propose to use following approach:

@Suppress("ktlint-string-template")

to disable rule for the block of code and remove this map.

Copy link
Contributor Author

@AleksandrSl AleksandrSl Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that this map looks weird, however original purpose for this PR was to respect annotations like @Suppress("RemoveCurlyBracesFromTemplate") to disable both IntellijIDEA and ktlint checks at the same time. Otherwise we will have to add both annotations

@Suppress("ktlint-string-template")
@Suppress("RemoveCurlyBracesFromTemplate")
println("Not found: ${notFound}")

and this way all sense in introduction of support for @Suppress annotation seems lost to me, cause it becomes specific for ktlint, and as you mentioned we already have ways to suppress ktlint checks

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that this map looks weird, however original purpose for this PR was to respect annotations like @Suppress("RemoveCurlyBracesFromTemplate") to disable both IntellijIDEA and ktlint checks at the same time.

Sorry, missed that. Then, as intermediate step, having this map looks ok. In the future would be better that rule provides a list of suppress names aliases.

"RemoveCurlyBracesFromTemplate" to "string-template"
)

private val suppressAnnotations = setOf("Suppress", "SuppressWarnings")

/**
* Creates [SuppressionHint] from annotations of given [psi]
* Returns null if no [targetAnnotations] present or no mapping exists
* between annotations' values and ktlint rules
*/
private fun createSuppressionHintFromAnnotations(
psi: KtAnnotated,
targetAnnotations: Collection<String>,
annotationValueToRuleMapping: Map<String, String>
): SuppressionHint? {

return psi.annotationEntries
.filter {
it.calleeExpression?.constructorReferenceExpression
?.getReferencedName() in targetAnnotations
}.flatMap(KtAnnotationEntry::getValueArguments)
.mapNotNull { it.getArgumentExpression()?.text?.removeSurrounding("\"") }
.mapNotNull(annotationValueToRuleMapping::get)
.let { suppressedRules ->
if (suppressedRules.isNotEmpty()) {
SuppressionHint(
IntRange(psi.startOffset, psi.endOffset),
suppressedRules.toSet()
)
} else {
null
}
}
}
}
}

Expand Down
32 changes: 31 additions & 1 deletion ktlint-ruleset-standard/src/test/resources/spec/string-template/lint.kt.spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,45 @@ fun main() {

println("${h["x-forwarded-proto"] ?: "http"}")
println("${if (diff > 0) "expanded" else if (diff < 0) "shrank" else "changed"}")

@Suppress("RemoveCurlyBracesFromTemplate")
println("${s0}")
@Suppress("RemoveCurlyBracesFromTemplate", "Unused")
println("${s0}")
@Suppress("RemoveCurlyBracesFromTemplate")
val t = "${s0}"
}

class B(val k: String) {
override fun toString(): String = "${super.toString()}, ${super.hashCode().toString()}, k=$k"

@Suppress("RemoveCurlyBracesFromTemplate")
val a
get() = "${s0}"
}

@Suppress("RemoveCurlyBracesFromTemplate")
class C {
override fun toString(): String = "${s0}"
}

// Ensure that suppression scope is as wide as it should be
class D {
@Suppress("RemoveCurlyBracesFromTemplate")
override fun toString(): String = "${s0}"

fun test() = "${s0}"
}

@SuppressWarnings("RemoveCurlyBracesFromTemplate")
class E {
override fun toString(): String = "${s0}"
}

// expect
// 2:29:Redundant "toString()" call in string template
// 3:28:Redundant "toString()" call in string template
// 6:15:Redundant curly braces
// 7:15:Redundant curly braces
// 21:79:Redundant "toString()" call in string template
// 28:79:Redundant "toString()" call in string template
// 45:20:Redundant curly braces