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

Add description to inspections. #281

Merged
merged 21 commits into from
Mar 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
91f697a
WIP Add description to inspections.
mwz Mar 5, 2020
d9108cc
Add description to the remaining collection inspections.
mwz Mar 7, 2020
e9f04ea
Add description to control flow, empty, exception, imports, inference…
mwz Mar 8, 2020
93856f6
Add description to the math inspections.
mwz Mar 8, 2020
7b46f76
Add description to naming, nulls, option and string inspections.
mwz Mar 8, 2020
17effc8
Add description to style, unnecessary, unsafe and other uncategorised…
mwz Mar 9, 2020
9c8a885
Fix tests.
mwz Mar 9, 2020
9d579ae
Merge branch 'master' into add-descriptions
mwz Mar 9, 2020
d0808c6
Fix trailing comma.
mwz Mar 9, 2020
6f0daca
Bring back code snippets to console output.
mwz Mar 10, 2020
3e2c5eb
Bump up sbt.
mwz Mar 10, 2020
a96a79b
Remove 2.13 from the build matrix.
mwz Mar 11, 2020
99cf7e5
Include code snippets in XML reports.
mwz Mar 11, 2020
b083f63
Merge branch 'master' into add-descriptions
mwz Mar 11, 2020
8e82076
Merge branch 'master' into add-descriptions
mccartney Mar 12, 2020
99b14fa
Update src/main/scala/com/sksamuel/scapegoat/inspections/LonelySealed…
mccartney Mar 12, 2020
509131c
Update src/main/scala/com/sksamuel/scapegoat/inspections/collections/…
mccartney Mar 12, 2020
56be483
Update src/main/scala/com/sksamuel/scapegoat/inspections/collections/…
mccartney Mar 12, 2020
0db44a6
Update src/main/scala/com/sksamuel/scapegoat/inspections/collections/…
mccartney Mar 12, 2020
b5fa092
Update src/main/scala/com/sksamuel/scapegoat/inspections/collections/…
mccartney Mar 12, 2020
f120a66
Update src/main/scala/com/sksamuel/scapegoat/inspections/collections/…
mccartney Mar 12, 2020
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
2 changes: 0 additions & 2 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ scala:
- 2.12.8
- 2.12.9
- 2.12.10
- 2.13.0
- 2.13.1
script:
- sbt clean test
Expand All @@ -24,4 +23,3 @@ cache:
directories:
- $HOME/.ivy2/cache
- $HOME/.sbt/boot/

2 changes: 1 addition & 1 deletion build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def check(code: String) = {
// runner.reporter.reset
val c = runner compileCodeSnippet code
val feedback = c.scapegoat.feedback
feedback.warnings map (x => "%-40s %s".format(x.text, x.snippet getOrElse "")) foreach println
feedback.warnings map (x => "%-40s %s %s".format(x.text, x.explanation, x.snippet.getOrElse(""))) foreach println
feedback
}
"""
Expand Down
31 changes: 24 additions & 7 deletions src/main/scala/com/sksamuel/scapegoat/Feedback.scala
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,15 @@ class Feedback(consoleOutput: Boolean, reporter: Reporter, sourcePrefix: String,
def warns = warnings(Levels.Warning)
def warnings(level: Level): Seq[Warning] = warnings.filter(_.level == level)

def warn(pos: Position, inspection: Inspection, snippet: Option[String] = None): Unit = {
def warn(
pos: Position,
inspection: Inspection,
snippet: Option[String] = None,
adhocExplanation: Option[String] = None
): Unit = {
val level = inspection.defaultLevel
val text = inspection.text
val snippetText = inspection.explanation.orElse(snippet)
val explanation = adhocExplanation.getOrElse(inspection.explanation)
val adjustedLevel = (levelOverridesByInspectionSimpleName.get("all"), levelOverridesByInspectionSimpleName.get(inspection.getClass.getSimpleName)) match {
case (Some(l), _) => l
case (None, Some(l)) => l
Expand All @@ -32,11 +37,21 @@ class Feedback(consoleOutput: Boolean, reporter: Reporter, sourcePrefix: String,

val sourceFileFull = pos.source.file.path
val sourceFileNormalized = normalizeSourceFile(sourceFileFull)
val warning = Warning(text, pos.line, adjustedLevel, sourceFileFull, sourceFileNormalized, snippetText, inspection.getClass.getCanonicalName)
val warning = Warning(
text = text,
line = pos.line,
level = adjustedLevel,
sourceFileFull = sourceFileFull,
sourceFileNormalized = sourceFileNormalized,
snippet = snippet,
explanation = explanation,
inspection = inspection.getClass.getCanonicalName
)
warningsBuffer.append(warning)
if (shouldPrint(warning)) {
println(s"[${warning.level.toString.toLowerCase}] $sourceFileNormalized:${warning.line}: $text")
snippetText.foreach(s => println(s" $s"))
println(s" $explanation")
snippet.foreach(s => println(s" $s"))
println()
}

Expand All @@ -55,14 +70,16 @@ class Feedback(consoleOutput: Boolean, reporter: Reporter, sourcePrefix: String,
}
}

case class Warning(text: String,
case class Warning(
text: String,
line: Int,
level: Level,
sourceFileFull: String,
sourceFileNormalized: String,
snippet: Option[String],
inspection: String) {

explanation: String,
inspection: String
) {
def hasMinimalLevelOf(minimalLevel: Level): Boolean = {
minimalLevel match {
case Levels.Info => true
Expand Down
30 changes: 19 additions & 11 deletions src/main/scala/com/sksamuel/scapegoat/Inspection.scala
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import scala.reflect.internal.util.Position
import scala.tools.nsc.Global

/** @author Stephen Samuel */
abstract class Inspection(val text: String, val defaultLevel: Level, val explanation: Option[String] = None) {
abstract class Inspection(
val text: String,
val defaultLevel: Level,
val description: String,
val explanation: String
) {

val self = this

def this(text: String, defaultLevel: Level, explanation: String) =
this(text, defaultLevel, Option(explanation))

def inspector(context: InspectionContext): Inspector
}

Expand Down Expand Up @@ -40,13 +42,19 @@ abstract class Inspector(val context: InspectionContext) {

case class InspectionContext(global: Global, feedback: Feedback) {

def warn(pos: Position, inspection: Inspection, snippet: String): Unit = {
feedback.warn(pos, inspection, Some(snippet))
}

def warn(pos: Position, inspection: Inspection): Unit = {
feedback.warn(pos, inspection)
}
def warn(pos: Position, inspection: Inspection): Unit =
feedback.warn(pos, inspection, None, None)

def warn(pos: Position, inspection: Inspection, snippet: String): Unit =
feedback.warn(pos, inspection, Some(snippet), None)

def warn(
pos: Position,
inspection: Inspection,
snippet: String,
adhocExplanation: String
): Unit =
feedback.warn(pos, inspection, Some(snippet), Some(adhocExplanation))

trait Traverser extends global.Traverser {

Expand Down

This file was deleted.

This file was deleted.

18 changes: 10 additions & 8 deletions src/main/scala/com/sksamuel/scapegoat/inspections/AnyUse.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,27 @@ import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}
import scala.reflect.internal.Flags

/** @author Stephen Samuel */
class AnyUse extends Inspection("AnyUse", Levels.Info) {
class AnyUse extends Inspection(
text = "Use of Any",
defaultLevel = Levels.Info,
description = "Checks for code returning Any.",
explanation = "Code returning Any is most likely an indication of a programming error."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {

import context.global._

def warn(tree: Tree) = {
context.warn(tree.pos, AnyUse.this,
"Use of Any should be avoided: " + tree.toString().take(200))
}

override def inspect(tree: Tree): Unit = {
tree match {
case DefDef(mods, _, _, _, _, _) if mods.isSynthetic =>
case DefDef(mods, _, _, _, _, _) if mods.hasFlag(Flags.SetterFlags) =>
case DefDef(mods, _, _, _, _, _) if mods.hasFlag(Flags.GetterFlags) =>
case ValDef(_, _, tpt, _) if tpt.tpe =:= typeOf[Any] => warn(tree)
case DefDef(_, _, _, _, tpt, _) if tpt.tpe =:= typeOf[Any] => warn(tree)
case ValDef(_, _, tpt, _) if tpt.tpe =:= typeOf[Any] =>
context.warn(tree.pos, self, tree.toString.take(200))
case DefDef(_, _, _, _, tpt, _) if tpt.tpe =:= typeOf[Any] =>
context.warn(tree.pos, self, tree.toString.take(200))
case _ => continue(tree)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}
import scala.runtime.{RichInt, RichLong}

/** @author Stephen Samuel */
class AvoidToMinusOne extends Inspection("Avoid To Minus One", Levels.Info) {
class AvoidToMinusOne extends Inspection(
text = "Avoid (j to k - 1)",
defaultLevel = Levels.Info,
description = "Checks for ranges using (j to k - 1).",
explanation = "A range in the following format (j to k - 1) can be simplified to (j until k)."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {
Expand All @@ -24,9 +29,9 @@ class AvoidToMinusOne extends Inspection("Avoid To Minus One", Levels.Info) {
override def inspect(tree: Tree): Unit = {
tree match {
case Apply(TypeApply(Select(Apply(Select(lhs, To),
List(Apply(Select(loopvar, Minus), List(Literal(Constant(1)))))), Foreach), _), _) if isIntegral(lhs) && isIntegral(loopvar) =>
context.warn(tree.pos, self,
"j to k - 1 can be better written as j until k: " + tree.toString().take(200))
List(Apply(Select(loopvar, Minus), List(Literal(Constant(1)))))), Foreach), _), _)
if isIntegral(lhs) && isIntegral(loopvar) =>
context.warn(tree.pos, self, tree.toString.take(200))
case _ => continue(tree)
}
}
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ package com.sksamuel.scapegoat.inspections
import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}

/** @author Stephen Samuel */
class DoubleNegation extends Inspection("Double negation", Levels.Info) {
class DoubleNegation extends Inspection(
text = "Double negation",
defaultLevel = Levels.Info,
description = "Checks for code like !(!b).",
explanation = "Double negation can be removed, e.g. !(!b) it equal to just b."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {
Expand All @@ -15,8 +20,7 @@ class DoubleNegation extends Inspection("Double negation", Levels.Info) {
override def inspect(tree: Tree): Unit = {
tree match {
case Select(Select(_, Bang), Bang) =>
context.warn(tree.pos, self,
"Double negation can be removed: " + tree.toString().take(200))
context.warn(tree.pos, self, tree.toString.take(200))
case _ => continue(tree)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@ package com.sksamuel.scapegoat.inspections
import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}

/** @author Stephen Samuel */
class EmptyCaseClass extends Inspection("Empty case class", Levels.Info,
"Empty case class can be rewritten as a case object") {
class EmptyCaseClass extends Inspection(
text = "Empty case class",
defaultLevel = Levels.Info,
description = "Checks for empty case classes like, e.g. case class Faceman().",
explanation = "An empty case class can be rewritten as a case object."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@ package com.sksamuel.scapegoat.inspections
import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}

class FinalModifierOnCaseClass extends Inspection(
"Missing final modifier on case class", Levels.Info, "Case classes should have final modifier") {
text = "Missing final modifier on case class",
defaultLevel = Levels.Info,
description = "Checks for case classes without a final modifier.",
explanation = "Using case classes without final modifier can lead to surprising breakage."
Copy link
Collaborator

Choose a reason for hiding this comment

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

That's very vague. I guess we can improve it later (in a separate PR)

) {

override def inspector(context: InspectionContext): Inspector = new Inspector(context) {

Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}
import scala.collection.mutable

/** @author Stephen Samuel */
class LonelySealedTrait extends Inspection("Lonely sealed trait", Levels.Error) {
class LonelySealedTrait extends Inspection(
text = "Lonely sealed trait",
defaultLevel = Levels.Error,
description = "Checks for sealed traits without any classes extending it.",
explanation = "A sealed trait that is not extended is considered dead code."
) {

override def inspector(context: InspectionContext): Inspector = new Inspector(context) {

Expand All @@ -17,7 +22,7 @@ class LonelySealedTrait extends Inspection("Lonely sealed trait", Levels.Error)
override def postInspection(): Unit = {
for ((name, cdef) <- sealedClasses) {
if (!implementedClasses.contains(name)) {
context.warn(cdef.pos, self, s"Sealed trait ${cdef.name} has no implementing classes")
context.warn(cdef.pos, self)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ package com.sksamuel.scapegoat.inspections
import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}

/** @author Stephen Samuel */
class MaxParameters extends Inspection("Max parameters", Levels.Info) {
class MaxParameters extends Inspection(
text = "Max parameters",
defaultLevel = Levels.Info,
description = "Checks for methods that have over 10 parameters.",
explanation = "Methods having a large number of parameters are more difficult to reason about, consider refactoring this code."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {
Expand All @@ -21,8 +26,7 @@ class MaxParameters extends Inspection("Max parameters", Levels.Info) {
case DefDef(_, name, _, _, _, _) if name == nme.CONSTRUCTOR =>
case DefDef(mods, _, _, _, _, _) if mods.isSynthetic =>
case DefDef(_, name, _, vparamss, _, _) if countExceeds(vparamss, 10) =>
context.warn(tree.pos, self,
s"Method $name has ${count(vparamss)} parameters. Consider refactoring to a containing instance.")
context.warn(tree.pos, self)
case _ => continue(tree)
}
}
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,12 @@ package com.sksamuel.scapegoat.inspections
import com.sksamuel.scapegoat.{Inspection, InspectionContext, Inspector, Levels}

/** @author Stephen Samuel */
class NoOpOverride extends Inspection("No op Override", Levels.Info) {
class NoOpOverride extends Inspection(
text = "Noop override",
defaultLevel = Levels.Info,
description = "Checks for code that overrides parent method but simply calls super.",
explanation = "This method is overridden yet only calls super."
) {

def inspector(context: InspectionContext): Inspector = new Inspector(context) {
override def postTyperTraverser = Some apply new context.Traverser {
Expand All @@ -22,8 +27,7 @@ class NoOpOverride extends Inspection("No op Override", Levels.Info) {
tree match {
case DefDef(_, name, _, vparamss, _, Apply(Select(Super(This(_), _), name2), args))
if name == name2 && vparamss.size == 1 && argumentsMatch(vparamss.head, args) =>
context.warn(tree.pos, self,
"This method is overridden yet only calls super: " + tree.toString().take(200))
context.warn(tree.pos, self, tree.toString.take(200))
case _ => continue(tree)
}
}
Expand Down
Loading