diff --git a/be2-scala/.scalafix.conf b/be2-scala/.scalafix.conf index 480ce48d8b..4a40c1abdf 100644 --- a/be2-scala/.scalafix.conf +++ b/be2-scala/.scalafix.conf @@ -1,6 +1,17 @@ rules = [ - DisableSyntax, - OrganizeImports, - RedundantSyntax + ArraysInFormat, + CatchNpe, + ComparingFloatingTypes, + EmptyInterpolatedString, + IllegalFormatString, + ImpossibleOptionSizeCondition, + IncorrectlyNamedExceptions, + IncorrectNumberOfArgsToFormat, + LonelySealedTrait, + MapGetAndGetOrElse, + NanComparison, + OptionGet, + OptionSize, + StripMarginOnRegex, + TryGet ] -OrganizeImports.removeUnused = false diff --git a/be2-scala/build.sbt b/be2-scala/build.sbt index b838ba4b0c..4602569d02 100644 --- a/be2-scala/build.sbt +++ b/be2-scala/build.sbt @@ -7,16 +7,16 @@ scalaVersion := "3.3.1" // Recommended 2.13 Scala flags (https://nathankleyn.com/2019/05/13/recommended-scalac-flags-for-2-13) slightly adapted for PoP scalacOptions ++= Seq( - "-deprecation", // Emit warning and location for usages of deprecated APIs. - "-explain", // Explain type errors in more detail. - "-feature", // Emit warning and location for usages of features that should be imported explicitly. - "-language:existentials", // Existential types (besides wildcard types) can be written and inferred - "-language:experimental.macros", // Allow macro definition (besides implementation and application) - "-language:higherKinds", // Allow higher-kinded types - "-language:implicitConversions", // Allow definition of implicit functions called views - "-unchecked", // Enable additional warnings where generated code depends on assumptions. - "-Ysafe-init", // Wrap field accessors to throw an exception on uninitialized access. - "-Xfatal-warnings" // Fail the compilation if there are any warnings. + "-deprecation", // Emit warning and location for usages of deprecated APIs. + "-explain", // Explain type errors in more detail. + "-feature", // Emit warning and location for usages of features that should be imported explicitly. + "-language:existentials", // Existential types (besides wildcard types) can be written and inferred + "-language:experimental.macros", // Allow macro definition (besides implementation and application) + "-language:higherKinds", // Allow higher-kinded types + "-language:implicitConversions", // Allow definition of implicit functions called views + "-unchecked", // Enable additional warnings where generated code depends on assumptions. + "-Ysafe-init", // Wrap field accessors to throw an exception on uninitialized access. + "-Xfatal-warnings" // Fail the compilation if there are any warnings. ) // Reload changes automatically @@ -30,9 +30,9 @@ Compile / run / connectInput := true // This allows the system properties (-D...) to be passed to the fork // Code from: https://stackoverflow.com/a/54326800 Compile / run / javaOptions ++= { - sys.props.toList.map { - case (key, value) => s"-D$key=$value" - } + sys.props.toList.map { + case (key, value) => s"-D$key=$value" + } } // Make test execution synchronized @@ -41,23 +41,23 @@ Test / test / parallelExecution := false // Create task to copy the protocol folder to resources lazy val copyProtocolTask = taskKey[Unit]("Copy protocol to resources") copyProtocolTask := { - val log = streams.value.log - log.info("Executing Protocol folder copy...") - val scalaDest = "be2-scala" - baseDirectory.value.name - - if (!baseDirectory.value.name.equals(scalaDest)) { - log.error(s"Please make sure you working dir is $scalaDest !") - } else { - val source = new File("../protocol") - val dest = new File("./src/main/resources/protocol") - Try(IO.copyDirectory(source, dest, overwrite = true)) match { - case Success(_) => log.info("Copied protocol folder in ./src/main/resources") - case Failure(exception) => - log.error("Could not copy protocol to resource folder") - exception.printStackTrace() + val log = streams.value.log + log.info("Executing Protocol folder copy...") + val scalaDest = "be2-scala" + baseDirectory.value.name + + if (!baseDirectory.value.name.equals(scalaDest)) { + log.error(s"Please make sure you working dir is $scalaDest !") + } else { + val source = new File("../protocol") + val dest = new File("./src/main/resources/protocol") + Try(IO.copyDirectory(source, dest, overwrite = true)) match { + case Success(_) => log.info("Copied protocol folder in ./src/main/resources") + case Failure(exception) => + log.error("Could not copy protocol to resource folder") + exception.printStackTrace() + } } - } } // Add the copyProtocolTask to compile and test scopes @@ -76,54 +76,51 @@ Compile / run / mainClass := Some("ch.epfl.pop.Server") Compile / packageBin / mainClass := Some("ch.epfl.pop.Server") lazy val scoverageSettings = Seq( - Compile / coverageEnabled := true, - Test / coverageEnabled := true, - packageBin / coverageEnabled := false + Compile / coverageEnabled := true, + Test / coverageEnabled := true, + packageBin / coverageEnabled := false ) -// Scalafix -semanticdbEnabled := true - // Configure Sonar sonarProperties := Map( - "sonar.organization" -> "dedis", - "sonar.projectKey" -> "dedis_popstellar_be2", - "sonar.sources" -> "src/main/scala", - "sonar.tests" -> "src/test/scala", - "sonar.sourceEncoding" -> "UTF-8", - "sonar.scala.version" -> "3.3.1", - // Paths to the test and coverage reports - "sonar.scala.coverage.reportPaths" -> "./target/scala-3.3.1/scoverage-report/scoverage.xml", - "sonar.scala.scapegoat.reportPaths" -> "./target/scala-3.3.1/scapegoat-report/scapegoat.xml" + "sonar.organization" -> "dedis", + "sonar.projectKey" -> "dedis_popstellar_be2", + "sonar.sources" -> "src/main/scala", + "sonar.tests" -> "src/test/scala", + "sonar.sourceEncoding" -> "UTF-8", + "sonar.scala.version" -> "3.3.1", + // Paths to the test and coverage reports + "sonar.scala.coverage.reportPaths" -> "./target/scala-3.3.1/scoverage-report/scoverage.xml", + "sonar.scala.scapegoat.reportPaths" -> "./target/scala-3.3.1/scapegoat-report/scapegoat.xml" ) assembly / assemblyMergeStrategy := { - case PathList("module-info.class") => MergeStrategy.discard - case PathList("reference.conf") => MergeStrategy.concat - case PathList("META-INF", "MANIFEST.MF") => MergeStrategy.discard - case PathList("META-INF", "LICENSE") => MergeStrategy.concat - case PathList("META-INF", "INDEX.LIST") => MergeStrategy.concat - case PathList("META-INF", "NOTICE") => MergeStrategy.discard - case PathList("META-INF", "services", "com.fasterxml.jackson.core.ObjectCodec") => MergeStrategy.first - case PathList("META-INF", "services", "com.fasterxml.jackson.core.JsonFactory") => MergeStrategy.first - case PathList("META-INF", "versions", "9", "module-info.class") => MergeStrategy.first - - case PathList("google", "protobuf", "api.proto") => MergeStrategy.first - case PathList("google", "protobuf", "struct.proto") => MergeStrategy.first - case PathList("google", "protobuf", "field_mask.proto") => MergeStrategy.first - case PathList("google", "protobuf", "duration.proto") => MergeStrategy.first - case PathList("google", "protobuf", "timestamp.proto") => MergeStrategy.first - case PathList("google", "protobuf", "source_context.proto") => MergeStrategy.first - case PathList("google", "protobuf", "empty.proto") => MergeStrategy.first - case PathList("google", "protobuf", "descriptor.proto") => MergeStrategy.first - case PathList("google", "protobuf", "wrappers.proto") => MergeStrategy.first - case PathList("google", "protobuf", "any.proto") => MergeStrategy.first - case PathList("google", "protobuf", "type.proto") => MergeStrategy.first - - // exclude digital signatures because the merging process can invalidate them - case PathList(ps @ _*) if Seq(".SF", ".DSA", ".RSA").exists(ps.last.endsWith(_)) => - MergeStrategy.discard - case _ => MergeStrategy.defaultMergeStrategy("") + case PathList("module-info.class") => MergeStrategy.discard + case PathList("reference.conf") => MergeStrategy.concat + case PathList("META-INF", "MANIFEST.MF") => MergeStrategy.discard + case PathList("META-INF", "LICENSE") => MergeStrategy.concat + case PathList("META-INF", "INDEX.LIST") => MergeStrategy.concat + case PathList("META-INF", "NOTICE") => MergeStrategy.discard + case PathList("META-INF", "services", "com.fasterxml.jackson.core.ObjectCodec") => MergeStrategy.first + case PathList("META-INF", "services", "com.fasterxml.jackson.core.JsonFactory") => MergeStrategy.first + case PathList("META-INF", "versions", "9", "module-info.class") => MergeStrategy.first + + case PathList("google", "protobuf", "api.proto") => MergeStrategy.first + case PathList("google", "protobuf", "struct.proto") => MergeStrategy.first + case PathList("google", "protobuf", "field_mask.proto") => MergeStrategy.first + case PathList("google", "protobuf", "duration.proto") => MergeStrategy.first + case PathList("google", "protobuf", "timestamp.proto") => MergeStrategy.first + case PathList("google", "protobuf", "source_context.proto") => MergeStrategy.first + case PathList("google", "protobuf", "empty.proto") => MergeStrategy.first + case PathList("google", "protobuf", "descriptor.proto") => MergeStrategy.first + case PathList("google", "protobuf", "wrappers.proto") => MergeStrategy.first + case PathList("google", "protobuf", "any.proto") => MergeStrategy.first + case PathList("google", "protobuf", "type.proto") => MergeStrategy.first + + // exclude digital signatures because the merging process can invalidate them + case PathList(ps @ _*) if Seq(".SF", ".DSA", ".RSA").exists(ps.last.endsWith(_)) => + MergeStrategy.discard + case _ => MergeStrategy.defaultMergeStrategy("") } // ------------------------ DEPENDENCIES ------------------------ 77 @@ -134,14 +131,14 @@ val AkkaVersion = "2.9.1" val AkkaHttpVersion = "10.6.0" libraryDependencies ++= Seq( - "com.typesafe.akka" %% "akka-stream-typed" % AkkaVersion, // Akka streams (Graph) - "com.typesafe.akka" %% "akka-http" % AkkaHttpVersion, // Akka http (WebSockets) - "com.typesafe.akka" %% "akka-cluster-tools" % AkkaVersion, // Akka distributed publish/subscribe cluster - - "ch.qos.logback" % "logback-classic" % "1.4.14" % Runtime, // Akka logging library - "com.typesafe.akka" %% "akka-testkit" % AkkaVersion % Test, // Akka actor test kit (akka actor testing library) - "com.typesafe.akka" %% "akka-stream-testkit" % AkkaVersion, // Akka stream test kit - "com.typesafe.akka" %% "akka-http-testkit" % AkkaHttpVersion // Akka http test kit + "com.typesafe.akka" %% "akka-stream-typed" % AkkaVersion, // Akka streams (Graph) + "com.typesafe.akka" %% "akka-http" % AkkaHttpVersion, // Akka http (WebSockets) + "com.typesafe.akka" %% "akka-cluster-tools" % AkkaVersion, // Akka distributed publish/subscribe cluster + + "ch.qos.logback" % "logback-classic" % "1.4.14" % Runtime, // Akka logging library + "com.typesafe.akka" %% "akka-testkit" % AkkaVersion % Test, // Akka actor test kit (akka actor testing library) + "com.typesafe.akka" %% "akka-stream-testkit" % AkkaVersion, // Akka stream test kit + "com.typesafe.akka" %% "akka-http-testkit" % AkkaHttpVersion // Akka http test kit ) // LevelDB database @@ -173,3 +170,11 @@ libraryDependencies += "com.google.zxing" % "core" % "3.5.1" libraryDependencies += "com.auth0" % "java-jwt" % "4.4.0" conflictManager := ConflictManager.latestCompatible + +inThisBuild( + List( + semanticdbEnabled := true, + semanticdbVersion := scalafixSemanticdb.revision, + scalafixDependencies += "io.github.dedis" %% "scapegoat-scalafix" % "1.0" // Import custom rules + ) +) diff --git a/be2-scala/linter/build.sbt b/be2-scala/linter/build.sbt index 4740c2e8f5..2b2c100518 100644 --- a/be2-scala/linter/build.sbt +++ b/be2-scala/linter/build.sbt @@ -1,12 +1,20 @@ lazy val V = _root_.scalafix.sbt.BuildInfo -lazy val rulesCrossVersions = Seq(V.scala213, V.scala212) +lazy val rulesCrossVersions = Seq(V.scala213) lazy val scala3Version = "3.3.1" inThisBuild( List( - organization := "dedis", + organization := "io.github.dedis", + organizationName := "dedis", + organizationHomepage := Some(url("https://dedis.epfl.ch")), + homepage := Some(url("https://github.com/dedis/popstellar")), + licenses := List("AGPL 3.0" -> url("https://www.gnu.org/licenses/agpl-3.0.en.html")), + developers := List(Developer("t1b00", "Thibault Czarniak", "thibault.czarniak@epfl.ch", url("https://www.linkedin.com/in/thcz/"))), semanticdbEnabled := true, - semanticdbVersion := scalafixSemanticdb.revision + semanticdbVersion := scalafixSemanticdb.revision, + scmInfo := Some(ScmInfo(url("https://github.com/dedis/popstellar"), "scm:git@github:dedis/popstellar.git")), + version := "1.0", + versionScheme := Some("pvp"), ) ) @@ -20,14 +28,13 @@ lazy val `popstellar` = (project in file(".")) .settings( publish / skip := true ) - lazy val rules = projectMatrix .settings( - moduleName := "scalafix", - libraryDependencies += "ch.epfl.scala" %% "scalafix-core" % V.scalafixVersion + moduleName := "scapegoat-scalafix", + libraryDependencies += "ch.epfl.scala" % "scalafix-core_2.13" % V.scalafixVersion, ) .defaultAxes(VirtualAxis.jvm) - .jvmPlatform(rulesCrossVersions) + .jvmPlatform(rulesCrossVersions :+ scala3Version) lazy val input = projectMatrix .settings( @@ -52,6 +59,7 @@ lazy val testsAggregate = Project("tests", file("target/testsAggregate")) lazy val tests = projectMatrix .settings( publish / skip := true, + scalaVersion := V.scala213, scalafixTestkitOutputSourceDirectories := TargetAxis .resolve(output, Compile / unmanagedSourceDirectories) @@ -70,20 +78,16 @@ lazy val tests = projectMatrix .defaultAxes( rulesCrossVersions.map(VirtualAxis.scalaABIVersion) :+ VirtualAxis.jvm: _* ) - .jvmPlatform( - scalaVersions = Seq(V.scala212), - axisValues = Seq(TargetAxis(scala3Version)), - settings = Seq() - ) .jvmPlatform( scalaVersions = Seq(V.scala213), axisValues = Seq(TargetAxis(V.scala213)), settings = Seq() ) .jvmPlatform( - scalaVersions = Seq(V.scala212), - axisValues = Seq(TargetAxis(V.scala212)), + scalaVersions = Seq(V.scala213), + axisValues = Seq(TargetAxis(scala3Version)), settings = Seq() ) + .dependsOn(rules) .enablePlugins(ScalafixTestkitPlugin) diff --git a/be2-scala/linter/input/src/main/scala/fix/ArraysInFormat.scala b/be2-scala/linter/input/src/main/scala/fix/ArraysInFormat.scala index 9e6f63a8d5..0677aff59d 100644 --- a/be2-scala/linter/input/src/main/scala/fix/ArraysInFormat.scala +++ b/be2-scala/linter/input/src/main/scala/fix/ArraysInFormat.scala @@ -17,6 +17,8 @@ object ArraysInFormat { Array passed to format / interpolate string */ + String.format("Here are my cool elements %d", Array.empty[Int]) // assert: ArraysInFormat + "Here are my cool elements %d".format(13) // scalafix: ok; } diff --git a/be2-scala/linter/input/src/main/scala/fix/EmptyInterpolatedString.scala b/be2-scala/linter/input/src/main/scala/fix/EmptyInterpolatedString.scala index 7f5646334d..47482e9683 100644 --- a/be2-scala/linter/input/src/main/scala/fix/EmptyInterpolatedString.scala +++ b/be2-scala/linter/input/src/main/scala/fix/EmptyInterpolatedString.scala @@ -11,5 +11,6 @@ object EmptyInterpolatedString { val str = "I'm hungry!" str.format() // assert: EmptyInterpolatedString "I'm hungry!".format() // assert: EmptyInterpolatedString + "Test %s".format("test") // scalafix: ok; } } diff --git a/be2-scala/linter/input/src/main/scala/fix/IllegalFormatString.scala b/be2-scala/linter/input/src/main/scala/fix/IllegalFormatString.scala index d209d14033..228b45d53a 100644 --- a/be2-scala/linter/input/src/main/scala/fix/IllegalFormatString.scala +++ b/be2-scala/linter/input/src/main/scala/fix/IllegalFormatString.scala @@ -10,6 +10,8 @@ object IllegalFormatString { val illegalFormatString1 = "%s is %d years old, %d" String.format(illegalFormatString1, name, age) // assert: IllegalFormatString + String.format("%d is %d years old", age, name) // assert: IllegalFormatString + illegalFormatString1.format(name, age) // assert: IllegalFormatString "%s is %d years old, %d".format(name, age) // assert: IllegalFormatString diff --git a/be2-scala/linter/input/src/main/scala/fix/StripMarginOnRegex.scala b/be2-scala/linter/input/src/main/scala/fix/StripMarginOnRegex.scala index d60007cf5f..aa4b9841f6 100644 --- a/be2-scala/linter/input/src/main/scala/fix/StripMarginOnRegex.scala +++ b/be2-scala/linter/input/src/main/scala/fix/StripMarginOnRegex.scala @@ -6,6 +6,8 @@ package fix object StripMarginOnRegex { def test() = { val regex = "match|this".stripMargin.r // assert: StripMarginOnRegex + val myRegex = "match|this" + myRegex.stripMargin.r // assert: StripMarginOnRegex "match|that".stripMargin.r // assert: StripMarginOnRegex "match_this".stripMargin.r // scalafix: ok; "match|this".r // scalafix: ok; diff --git a/be2-scala/linter/project/plugins.sbt b/be2-scala/linter/project/plugins.sbt index 43d80c6195..b0733a9099 100644 --- a/be2-scala/linter/project/plugins.sbt +++ b/be2-scala/linter/project/plugins.sbt @@ -1,3 +1,4 @@ resolvers += Resolver.sonatypeRepo("releases") -addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.12.0") +addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.12.1") addSbtPlugin("com.eed3si9n" % "sbt-projectmatrix" % "0.9.2") +addSbtPlugin("com.geirsson" % "sbt-ci-release" % "1.5.7") diff --git a/be2-scala/linter/readme.md b/be2-scala/linter/readme.md index 3f70235a62..7e83de51c6 100644 --- a/be2-scala/linter/readme.md +++ b/be2-scala/linter/readme.md @@ -1,9 +1,133 @@ # Scalafix rules for PoPStellar -To develop rule: +## Versions: + +* **Scala 3**: +[![maven](https://img.shields.io/maven-central/v/io.github.dedis/scapegoat-scalafix_3)](https://search.maven.org/artifact/io.github.dedis/scapegoat-scalafix_3) + + +* **Scala 2.13**: +[![maven](https://img.shields.io/maven-central/v/io.github.dedis/scapegoat-scalafix_2.13)](https://search.maven.org/artifact/io.github.dedis/scapegoat-scalafix_2.13) + +[version]: 1.0 +## Description +This subfolder contains a set of custom linter rules developed for the Scala backend of [PopStellar](https://github.com/dedis/popstellar). +They are built on top of the [Scalafix](https://scalacenter.github.io/scalafix/) framework. +Since these rules can also be used by other projects that use Scalafix, they are published on Maven Central, for Scala 2.13 and Scala 3. + +## Installation + +To install the rules, simply add the following to your `build.sbt` file: +``` +ThisBuild / scalafixDependencies += "io.github.dedis" %% "scapegoat-scalafix" % "1.0" +``` + +**The rules are compatible with Scala 2.13 and Scala 3 (tested for Scala 3.3.1).** + +To check proper installation, run `scalafix OptionGet` which should execute the OptionGet rule and succeed if everything went well. + +### Note +You might need to add the following lines: +``` +inThisBuild( + List( + semanticdbEnabled := true, + semanticdbVersion := scalafixSemanticdb.revision + ) +) +``` + +This is necessary to enable the SemanticDB, which is required for the rules to work. Only add these lines if SemanticDB is not already enabled in the `build.sbt`. + +## Rule list +1. **ArraysInFormat**: Checks if arrays are used in the format method of a string. +2. **CatchNpe**: Checks if a catch block catches a NullPointerException. +3. **ComparingFloatingPointTypes**: Checks if floating point types (float or double) are compared with == or !=. +4. **EmptyInterpolatedString**: Checks if an interpolated string is empty. +5. **IllegalFormatString**: Checks for illegal format strings. +6. **ImpossibleOptionSizeCondition**: Checks if an Option is compared with a size. +7. **IncorrectlyNamedExceptions**: Checks if exceptions are named correctly (end with Exception). +8. **IncorrectNumberOfArgsToFormat**: Checks if the number of arguments in a format string matches the number of arguments in the format method. +9. **LonelySealedTrait**: Checks if a sealed trait has only one implementation. +10. **MapGetGetOrElse**: Checks if a map.get is followed by a getOrElse. +11. **NanComparison**: Checks if a floating point number (float or double) is compared with Double.NaN. +12. **OptionGet**: Checks if an Option is used with get. +13. **OptionSize**: Checks if an Option is used with size. +14. **StripMarginOnRegex**: Checks if a regex is used with stripMargin. +15. **TryGet**: Checks if a Try is used with get. + +## Usage + +After installation, to run any of these rules, simply call: +``` +sbt scalafix RuleName +``` + +You can also create a `.scalafix.conf` file and enable rules in them. Here is an example with all of the rules enabled: ``` -sbt ~tests/test -# edit rules/src/main/scala/fix/Popstellar.scala +rules = [ + ArraysInFormat, + CatchNpe, + ComparingFloatingPointTypes, + EmptyInterpolatedString, + IllegalFormatString, + ImpossibleOptionSizeCondition, + IncorrectlyNamedExceptions, + IncorrectNumberOfArgsToFormat, + LonelySealedTrait, + MapGetGetOrElse, + NanComparison, + OptionGet, + OptionSize, + StripMarginOnRegex, + TryGet +] ``` -Output folder is ignored since this is a linter +With this, you can simply run all the rules in the configuration file by calling: +``` +sbt scalafix +``` + +## Usage + +To run the rules, simply execute the following command: +``` +sbt scalafix +``` + +## Modifying and adding rules + +Scalafix provides some documentation on how to write rules, following their [tutorial](https://scalacenter.github.io/scalafix/docs/developers/tutorial.html) is recommended. + +To add a rule, you need to +* Create rule test cases in the `input/src/main/scala/fix` folder +* Create the rule in the `rules/src/main/scala/fix` folder +* Add the rule to the list of rules in `rules/src/main/resources/META-INF/services/scalafix.v1.ScalafixRule` + + +Output folder is ignored since this is a linter. + +## Testing + +To test the rules run: +``` +sbt test +``` + +## Publishing + +To publish the rules, follow the tutorial on the Scalafix website. +In short you need to: +* Generate a gpg key to sign +* Publish the GPG key to https://keyserver.ubuntu.com +* Create an account on Maven central (see [tutorial](https://central.sonatype.org/register/central-portal/#choosing-a-namespace)) +* Verify namespace access for io.github.dedis (see [tutorial](https://central.sonatype.org/register/namespace/#create-an-account)) +* Modify the version in `build.sbt` +* Run `sbt publishSigned` +* ZIP the `target/sonatype-staging/VERSION/io/` folder (only include starting from io) and publish it to Maven: +simply click "Publish Component" on Sonatype, set the name to "io.github.dedis:scapegoat-scalafix:VERSION" and upload the ZIP file. + +_Don't forget to update the version in the installation section_ + + diff --git a/be2-scala/linter/rules/src/main/scala/fix/ArraysInFormat.scala b/be2-scala/linter/rules/src/main/scala/fix/ArraysInFormat.scala index b47c47a238..1a2ba55685 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/ArraysInFormat.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/ArraysInFormat.scala @@ -8,35 +8,26 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class ArraysInFormatDiag(array: Tree) extends Diagnostic { - override def message: String = "Array passed to format / interpolate string" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "An Array passed to String.format or interpolated string might result in an incorrect formatting" - - override def position: Position = array.pos -} - class ArraysInFormat extends SemanticRule("ArraysInFormat") { - private def rule(args: List[Stat])(implicit doc: SemanticDocument) = { + private def diag(pos: Position) = Diagnostic("", "Array passed to format / interpolate string", pos, "An Array passed to String.format or interpolated string might result in an incorrect formatting", LintSeverity.Error) + + def rule(args: List[Stat])(implicit doc: SemanticDocument): Patch = { args.collect { - case t @ Term.Name(_) => - t.symbol.info match { - case Some(symInfo) => symInfo.signature match { - case ValueSignature(TypeRef(_, symbol, _)) if SymbolMatcher.exact("scala/Array#").matches(symbol) => Patch.lint(ArraysInFormatDiag(t)) - case _ => Patch.empty - } - case _ => Patch.empty - } - } + case a if Util.matchType(a, "scala/Array", "scala/Array.empty") => Patch.lint(diag(a.pos)) + case _ => Patch.empty + }.asPatch } override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { + // Corresponds to String.format(formatString, args), myStr.format(args) and "str".format(args) case Term.Apply.After_4_6_0(Term.Select(_, Term.Name("format")), Term.ArgClause(args, _)) => rule(args) - case Term.Interpolate(_, _, args) => rule(args.flatMap { case Term.Block(stats) => stats }) // Args in Term.Interpolate come in Term.Block requiring to extract the stats - }.flatten.asPatch + // Corresponds to f"str $args" and s"str $args" + // Args in Term.Interpolate come in Term.Block requiring to extract the stats + case Term.Interpolate(_, _, args) => args.collect { case Term.Block(stats) => rule(stats) }.asPatch + case _ => Patch.empty + }.asPatch } + } diff --git a/be2-scala/linter/rules/src/main/scala/fix/CatchNpe.scala b/be2-scala/linter/rules/src/main/scala/fix/CatchNpe.scala index cdd59972ae..7dc6bbbf44 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/CatchNpe.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/CatchNpe.scala @@ -8,22 +8,16 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class CatchNpeDiag(catch_tree: Tree) extends Diagnostic { - override def message: String = "Catching NPE" - - override def severity: LintSeverity = LintSeverity.Error +class CatchNpe extends SemanticRule("CatchNpe") { - override def explanation: String = "Avoid using null at all cost and you shouldn't need to catch NullPointerExceptions. Prefer Option to indicate potentially missing values and use Try to materialize exceptions thrown by any external libraries." + private def diag(pos: Position) = Diagnostic("", "Catching NPE", pos, "Avoid using null at all cost and you shouldn't need to catch NullPointerExceptions. Prefer Option to indicate potentially missing values and use Try to materialize exceptions thrown by any external libraries.", LintSeverity.Error) - override def position: Position = catch_tree.pos -} - -class CatchNpe extends SemanticRule("CatchNpe") { override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { + // Corresponds to try { ... } catch { case e: NullPointerException => ... } case Term.Try(_, catches, _) => catches.collect { case Case(pat, _, _) => pat match { - case Pat.Typed(_, tpe) if tpe.toString().equals("NullPointerException") => Patch.lint(CatchNpeDiag(pat)) + case Pat.Typed(_, tpe) if tpe.toString().equals("NullPointerException") => Patch.lint(diag(pat.pos)) case _ => Patch.empty } } diff --git a/be2-scala/linter/rules/src/main/scala/fix/ComparingFloatingTypes.scala b/be2-scala/linter/rules/src/main/scala/fix/ComparingFloatingTypes.scala index 5756839fe8..e3316b0e76 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/ComparingFloatingTypes.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/ComparingFloatingTypes.scala @@ -8,18 +8,10 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class ComparingFloatingTypesDiag(floats: Tree) extends Diagnostic { - override def message: String = "Floating type comparison" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Due to minor rounding errors, it is not advisable to compare floating-point numbers using the == operator. Either use a threshold based comparison, or switch to a BigDecimal." - - override def position: Position = floats.pos -} - class ComparingFloatingTypes extends SemanticRule("ComparingFloatingTypes") { + private def diag(pos: Position) = Diagnostic("", "Floating type comparison", pos, "Due to minor rounding errors, it is not advisable to compare floating-point numbers using the == operator. Either use a threshold based comparison, or switch to a BigDecimal.", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { def isFloatOrDouble(term: Term): Boolean = { @@ -27,21 +19,14 @@ class ComparingFloatingTypes extends SemanticRule("ComparingFloatingTypes") { } doc.tree.collect { - case t @ Term.ApplyInfix.After_4_6_0(lhs, op, _, Term.ArgClause(List(right), _)) => - if (isFloatOrDouble(lhs) && isFloatOrDouble(right)) { - op match { - case Term.Name("==") | Term.Name("!=") => Patch.lint(ComparingFloatingTypesDiag(t)) - case _ => Patch.empty - } - } else { - Patch.empty - } - case t @ Term.Apply.After_4_6_0(Term.Select(lhs, Term.Name("equals")), Term.ArgClause(List(right), _)) => - if (isFloatOrDouble(lhs) && isFloatOrDouble(right)) { - Patch.lint(ComparingFloatingTypesDiag(t)) - } else { - Patch.empty - } + // Corresponds to lhs == rhs or lhs != rhs + // We then check if both of the operands are a Float or a Double and if so, we lint + case t @ Term.ApplyInfix.After_4_6_0(lhs, Term.Name("==") | Term.Name("!="), _, Term.ArgClause(List(right), _)) + if isFloatOrDouble(lhs) && isFloatOrDouble(right) => Patch.lint(diag(t.pos)) + // Corresponds to lhs.equals(rhs) + case t @ Term.Apply.After_4_6_0(Term.Select(lhs, Term.Name("equals")), Term.ArgClause(List(right), _)) + if isFloatOrDouble(lhs) && isFloatOrDouble(right) => Patch.lint(diag(t.pos)) + case _ => Patch.empty } }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/EmptyInterpolatedString.scala b/be2-scala/linter/rules/src/main/scala/fix/EmptyInterpolatedString.scala index e722477660..ad8e1442c7 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/EmptyInterpolatedString.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/EmptyInterpolatedString.scala @@ -8,21 +8,18 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class EmptyInterpolatedStringDiag(string: Tree) extends Diagnostic { - override def message: String = "Empty interpolated / format string" - - override def severity: LintSeverity = LintSeverity.Error +class EmptyInterpolatedString extends SemanticRule("EmptyInterpolatedString") { - override def explanation: String = "String declared as interpolated but has no parameters or usage of String.format with no parameters can be turned into a regular string." + private def diag(pos: Position) = Diagnostic("", "Empty interpolated / format string", pos, "String declared as interpolated but has no parameters or usage of String.format with no parameters can be turned into a regular string.", LintSeverity.Error) - override def position: Position = string.pos -} - -class EmptyInterpolatedString extends SemanticRule("EmptyInterpolatedString") { override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { - case t @ Term.Apply.After_4_6_0(Term.Select(_, Term.Name("format")), Term.ArgClause(List(Lit.String(_)), _) | Term.ArgClause(Nil, _)) => Patch.lint(EmptyInterpolatedStringDiag(t)) - case t @ Term.Interpolate(_, _, Nil) => Patch.lint(EmptyInterpolatedStringDiag(t)) + // Corresponds to String.format(formatString, _) and String.format(), str.format() + case t @ Term.Apply.After_4_6_0(Term.Select(Term.Name("String"), Term.Name("format")), Term.ArgClause(List(Lit.String(_)) | Nil, _)) => Patch.lint(diag(t.pos)) + // Corresponds to "str".format() + case t @ Term.Apply.After_4_6_0(Term.Select(Lit.String(_) | Term.Name(_), Term.Name("format")), Term.ArgClause(Nil, _)) => Patch.lint(diag(t.pos)) + // Corresponds to f"str" and s"str" + case t @ Term.Interpolate(_, _, Nil) => Patch.lint(diag(t.pos)) }.asPatch } } diff --git a/be2-scala/linter/rules/src/main/scala/fix/IllegalFormatString.scala b/be2-scala/linter/rules/src/main/scala/fix/IllegalFormatString.scala index ae985feca3..9e93a7576b 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/IllegalFormatString.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/IllegalFormatString.scala @@ -11,29 +11,23 @@ import scalafix.v1._ import java.util.{IllegalFormatException, MissingFormatArgumentException, UnknownFormatConversionException} -case class IllegalFormatStringDiag(string: Tree) extends Diagnostic { - override def message: String = "Illegal format string" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "An unchecked exception will be thrown when a format string contains an illegal syntax or a format specifier that is incompatible with the given arguments" - - override def position: Position = string.pos -} - class IllegalFormatString extends SemanticRule("IllegalFormatString") { + private def diag(pos: Position) = Diagnostic("", "Illegal format string", pos, "An unchecked exception will be thrown when a format string contains an illegal syntax or a format specifier that is incompatible with the given arguments", LintSeverity.Error) + + // Rule tries to format the string to check if it ends in an exception, and if so lints //Term parameter is simply used to display the rule at the correct place - private def rule(term: Term, value: String, args: List[Any]): Patch = { + private def rule(t: Term, value: String, args: List[Any]): Patch = { try value.format(args: _*) catch { - case _: IllegalFormatException | _: MissingFormatArgumentException | _: UnknownFormatConversionException => return Patch.lint(IllegalFormatStringDiag(term)) + case _: IllegalFormatException | _: MissingFormatArgumentException | _: UnknownFormatConversionException => return Patch.lint(diag(t.pos)) } Patch.empty } override def fix(implicit doc: SemanticDocument): Patch = { + // Method to get the args with their corresponding definitions def getMappedArgs(args: List[Term]): List[Any] = { (findDefinitionsOrdered(doc.tree, args) ++ args).collect { case Lit(value) => value } } diff --git a/be2-scala/linter/rules/src/main/scala/fix/ImpossibleOptionSizeCondition.scala b/be2-scala/linter/rules/src/main/scala/fix/ImpossibleOptionSizeCondition.scala index a0d8aa8cc4..90b6ab7666 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/ImpossibleOptionSizeCondition.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/ImpossibleOptionSizeCondition.scala @@ -8,26 +8,20 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class ImpossibleOptionSizeConditionDiag(option: Tree) extends Diagnostic { - override def message: String = "Impossible Option.size condition" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Option.size > 1 can never be true, did you mean to use Option.nonEmpty instead?" - - override def position: Position = option.pos -} - class ImpossibleOptionSizeCondition extends SemanticRule("ImpossibleOptionSizeCondition") { + private def diag(pos: Position) = Diagnostic("", "Impossible Option.size condition", pos, "Option.size > 1 can never be true, did you mean to use Option.nonEmpty instead?", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { + // Corresponds to Option(_).size > 1 or Option(_).size >= 1, or o.size > 1 or o.size >= 1 with o a variable case t @ Term.ApplyInfix.After_4_6_0(Term.Select(qual, Term.Name("size")), Term.Name(">") | Term.Name(">="), _, Term.ArgClause(List(comparedValue), _)) if Util.matchType(qual, "scala/Option", "scala/Some") => + // We then check if the value is greater than or equal to 1 and if so, we lint comparedValue match { - case Lit.Int(actualValue) if actualValue >= 1 => Patch.lint(ImpossibleOptionSizeConditionDiag(t)) + case Lit.Int(actualValue) if actualValue >= 1 => Patch.lint(diag(t.pos)) case _ => Patch.empty } case _ => Patch.empty diff --git a/be2-scala/linter/rules/src/main/scala/fix/IncorrectNumberOfArgsToFormat.scala b/be2-scala/linter/rules/src/main/scala/fix/IncorrectNumberOfArgsToFormat.scala index b617d6cda2..d88553ae06 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/IncorrectNumberOfArgsToFormat.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/IncorrectNumberOfArgsToFormat.scala @@ -9,48 +9,47 @@ import scalafix.v1._ import scala.meta._ -case class IncorrectNumberOfArgsToFormatDiag(string: Tree) extends Diagnostic { - override def message: String = "Incorrect number of arguments to format" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "The number of arguments passed to String.format doesn't correspond to the number of fields in the format string." - - override def position: Position = string.pos -} - class IncorrectNumberOfArgsToFormat extends SemanticRule("IncorrectNumberOfArgsToFormat") { + private def diag(pos: Position) = Diagnostic("", "Incorrect number of arguments to format", pos, "The number of arguments passed to String.format doesn't correspond to the number of fields in the format string.", LintSeverity.Error) + private val argRegex = "%((\\d+\\$)?[-#+ 0,(\\<]*?\\d?(\\.\\d+)?[tT]?[a-zA-Z]|%)".r private def rule(format: String, args: List[Any], t: Term): Patch = { + // Compare expected argument count with the actual one, ignoring the format specifiers that don't take arguments val argCount = argRegex .findAllIn(format) .matchData .count(m => !doesNotTakeArguments(m.matched)) - if (argCount != args.size) Patch.lint(IncorrectNumberOfArgsToFormatDiag(t)) else Patch.empty + if (argCount != args.size) Patch.lint(diag(t.pos)) else Patch.empty } override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { case t @ Term.Apply.After_4_6_0(Term.Select(qual, Term.Name("format")), Term.ArgClause(args, _)) => qual match { + // Corresponds to String.format(formatString, args) case Term.Name("String") => + // The args is a list containing first the format string and then the arguments args match { + // Either the format string is a literal string or a val/var string case Lit.String(format) :: rest => rule(format, rest, t) case (head @ Term.Name(_)) :: rest => + // If the format string is a val/var, we need to find its definition val format = findDefinition(doc.tree, head) format match { case Lit.String(value) => rule(value, rest, t) } case _ => Patch.empty } + // Corresponds to a string declared as a val or var and used in a format call, e.g. myStr.format(args) case strName @ Term.Name(_) => val str = findDefinition(doc.tree, strName) str match { case Lit.String(value) => rule(value, args, t) case _ => Patch.empty } + // Corresponds to format directly applied on a string, e.g. "str".format(args) case Lit.String(value) => rule(value, args, t) } diff --git a/be2-scala/linter/rules/src/main/scala/fix/IncorrectlyNamedExceptions.scala b/be2-scala/linter/rules/src/main/scala/fix/IncorrectlyNamedExceptions.scala index a2eaf1e8cf..0f2f6ef4e9 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/IncorrectlyNamedExceptions.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/IncorrectlyNamedExceptions.scala @@ -8,18 +8,11 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class IncorrectlyNamedExceptionsDiag(exception: Tree) extends Diagnostic { - override def message: String = "Incorrectly named exceptions" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Class named exception does not derive from Exception / class derived from Exception is not named *Exception." - - override def position: Position = exception.pos -} - class IncorrectlyNamedExceptions extends SemanticRule("IncorrectlyNamedExceptions") { + private def diag(pos: Position) = Diagnostic("", "Incorrectly named exceptions", pos, "Class named exception does not derive from Exception / class derived from Exception is not named *Exception.", LintSeverity.Error) + + // Helper function to check if a class inherits from Exception, going through the ancestors private def inheritsFromException(symbol: Symbol)(implicit doc: SemanticDocument): Boolean = { symbol.info match { case Some(info) => @@ -41,11 +34,14 @@ class IncorrectlyNamedExceptions extends SemanticRule("IncorrectlyNamedException override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { + // In this rule, we check if there is an exception class that does not inherit from Exception + // Corresponds to a class declaration case cl @ Defn.Class.After_4_6_0(_, Type.Name(name), _, _, _) => cl.symbol.info.get.signature match { case ClassSignature(_, parents, _, _) => + // We then check the parents: either its direct parent is an Exception or one of its ancestors is an Exception if (!name.contains("Exception") && parents.map(_.asInstanceOf[TypeRef].symbol).exists(inheritsFromException)) - Patch.lint(IncorrectlyNamedExceptionsDiag(cl)) + Patch.lint(diag(cl.pos)) else Patch.empty case _ => Patch.empty } diff --git a/be2-scala/linter/rules/src/main/scala/fix/LonelySealedTrait.scala b/be2-scala/linter/rules/src/main/scala/fix/LonelySealedTrait.scala index 4d54a72053..b3ff676291 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/LonelySealedTrait.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/LonelySealedTrait.scala @@ -10,21 +10,17 @@ import scalafix.v1._ import scala.collection.mutable -case class LonelySealedTraitDiag(t: Tree) extends Diagnostic { - override def message: String = "Lonely sealed trait" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "A sealed trait that is not extended is considered dead code." - - override def position: Position = t.pos -} - class LonelySealedTrait extends SemanticRule("LonelySealedTrait") { + // This method finds the base classes for a given sealed trait / class, since we have collected the hierarchy of + // sealed traits / classes with our bottom-up traversal + private def diag(pos: Position) = Diagnostic("", "Lonely sealed trait", pos, "A sealed trait that is not extended is considered dead code.", LintSeverity.Error) + private def findBaseClasses(parent: String, sealedTraitsHierarchy: mutable.Map[String, Set[String]]): Set[String] = { sealedTraitsHierarchy.get(parent) match { case Some(baseClasses) => + // We recursively find the base classes of the base classes: we first get the direct children and then look + // at the children of the children baseClasses ++ baseClasses.flatMap(findBaseClasses(_, sealedTraitsHierarchy)) case None => Set.empty[String] @@ -33,7 +29,7 @@ class LonelySealedTrait extends SemanticRule("LonelySealedTrait") { override def fix(implicit doc: SemanticDocument): Patch = { - val sealedTraits = mutable.Map[String, Defn]() + val sealedTraits = mutable.Map[String, Defn]() // Map to store the Defn and position the warning correctly val parents = mutable.Set[String]() val sealedTraitsHierarchy = mutable.Map[String, Set[String]]() @@ -41,6 +37,10 @@ class LonelySealedTrait extends SemanticRule("LonelySealedTrait") { // Use display name to handle the case A[B] where A is a trait and B is a type parameter def prettyInits(inits: List[Init]): List[String] = inits.collect { case i if i.symbol.info.isDefined => i.symbol.info.get.displayName } + /* Function to handle base class collection + * It works by adding the passed sealed class as a child to its parents children, i.e. bottom-up traversal. + * Getting the base classes from the parent would be much harder and would require a tree traversal since the parents do not store their base classes. + * However, children store their parents so a bottom-up solution is easy. */ def handleSealedTrait(inits: List[Init], cl: Defn, clname: String): Unit = { if(inits.isEmpty) sealedTraitsHierarchy += (clname -> Set()) else { @@ -49,22 +49,30 @@ class LonelySealedTrait extends SemanticRule("LonelySealedTrait") { sealedTraits += (clname -> cl) } - + /* We first traverse the doc, collecting the sealed traits and classes. + * In this traversal, we also store the base classes of the parents of the sealed traits and classes + * For the non-sealed classes and objects, we collect their parents to later match sealed and non sealed */ doc.tree.traverse { + // Corresponds to a sealed trait declaration case tr @ Defn.Trait.After_4_6_0(mods, name, _, _, Template.After_4_4_0(_, inits, _, _, _)) if mods.exists(m => m.toString.equals("sealed")) => handleSealedTrait(inits, tr, name.value) + // Corresponds to a sealed class declaration case cls @ Defn.Class.After_4_6_0(mods, name, _, _, Template.After_4_4_0(_, inits, _, _, _)) if mods.exists(m => m.toString.equals("sealed")) => handleSealedTrait(inits, cls, name.value) + // Corresponds to a class declaration (not sealed) case cl @ Defn.Class.After_4_6_0(_, _, _, _, _) => cl.symbol.info.get.signature match { case ClassSignature(_, pars, _, _) => pars.foreach(p => parents += p.toString()) case _ => () // Class should have class signature } + // Corresponds to an object declaration (cannot be sealed) case Defn.Object(_, _, Template.After_4_4_0(_, inits, _, _, _)) => parents ++= prettyInits(inits) } + // Now that we know the sealed traits and classes and their children, we can check if any of their child classes + // have an implementation in the same file. If not, the sealed trait / class is not extended and is considered dead code sealedTraits.collect({ case (name, cl) if !parents.contains(name) - && parents.intersect(findBaseClasses(name,sealedTraitsHierarchy)).isEmpty => Patch.lint(LonelySealedTraitDiag(cl)) + && parents.intersect(findBaseClasses(name,sealedTraitsHierarchy)).isEmpty => Patch.lint(diag(cl.pos)) //Either this sealed trait is directly extended or one of its sealed base classes / traits is being extended case _ => Patch.empty }).asPatch diff --git a/be2-scala/linter/rules/src/main/scala/fix/MapGetAndGetOrElse.scala b/be2-scala/linter/rules/src/main/scala/fix/MapGetAndGetOrElse.scala index 01bb8a8f8d..ca0ebe3c2c 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/MapGetAndGetOrElse.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/MapGetAndGetOrElse.scala @@ -8,23 +8,17 @@ import scalafix.lint.LintSeverity import scala.meta._ import scalafix.v1._ -case class MapGetAndGetOrElseDiag(map: Tree) extends Diagnostic { - override def message: String = "Using of Map.get().getOrElse instead of Map.getOrElse()" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Map.get(key).getOrElse(value) can be replaced with Map.getOrElse(key, value), which is more concise." - - override def position: Position = map.pos -} - class MapGetAndGetOrElse extends SemanticRule("MapGetAndGetOrElse") { + private def diag(pos: Position) = Diagnostic("", "Using of Map.get().getOrElse instead of Map.getOrElse()", pos, "Map.get(key).getOrElse(value) can be replaced with Map.getOrElse(key, value), which is more concise.", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { + // Corresponds to map.get(key).getOrElse(value) case Term.Apply.After_4_6_0(Term.Select(Term.Apply.After_4_6_0(Term.Select(qual, Term.Name("get")), _), Term.Name("getOrElse")), _) + // Maps can either be immutable, mutable or simply a Map(k,v) from Predef that we use as Map(k,v).get(key).getOrElse(value) if Util.matchType(qual, "scala/collection/immutable/Map", "scala/collection/mutable/Map", "scala/Predef.Map") - => Patch.lint(MapGetAndGetOrElseDiag(qual)) + => Patch.lint(diag(qual.pos)) case _ => Patch.empty }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/NanComparison.scala b/be2-scala/linter/rules/src/main/scala/fix/NanComparison.scala index 36b83a72a3..5a028a8b61 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/NanComparison.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/NanComparison.scala @@ -7,18 +7,10 @@ import scalafix.lint.LintSeverity import scalafix.v1._ import scala.meta._ -case class NanComparisonDiag(tree: Tree) extends Diagnostic { - override def message: String = "Comparison with NaN" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "NaN comparison will always fail. Use value.isNaN instead." - - override def position: Position = tree.pos -} - class NanComparison extends SemanticRule("NanComparison") { + def diag(pos: Position) = Diagnostic("", "Comparison with NaN", pos, "NaN comparison will always fail. Use value.isNaN instead.", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { def matcher(l: Any, r: Any, t: Tree): Patch = { @@ -26,24 +18,29 @@ class NanComparison extends SemanticRule("NanComparison") { // => matcher function avoids repetition. Type of arguments is any since findDefinition will return Any, // but it doesn't matter since we do pattern matching (l, r) match { - case (Term.Select(Term.Name("Double"), Term.Name("NaN")), _) | (_, Term.Select(Term.Name("Double"), Term.Name("NaN"))) => Patch.lint(NanComparisonDiag(t)) + case (Term.Select(Term.Name("Double"), Term.Name("NaN")), _) | (_, Term.Select(Term.Name("Double"), Term.Name("NaN"))) => Patch.lint(diag(t.pos)) case _ => Patch.empty } } def rule(lhs: Term, rhs: Term, t: Term): Patch = { (lhs,rhs) match { + // Either we have a comparison between two variables (e.g. g == f) or a variable and a literal (e.g. g == Double.NaN) + // In the former case, we must find the definitions of the variables to see if one of them is Double.NaN case (Term.Name(_), Term.Name(_)) => Util.findDefinitions(doc.tree, Set(lhs, rhs)) match { case List((_, ld), (_, rd)) => matcher(ld, rd, t) case _ => Patch.empty } - // Extract definitions and match in the case both are variables + // In the latter case, we can directly match the terms case _ => matcher(lhs, rhs, t) } } doc.tree.collect { + // Corresponds to myVar == Double.Nan, Double.Nan == myVar, myVar != Double.Nan, Double.Nan != myVar + // We don't consider a comparison between Nan and a literal since it doesn't make sense in code case t @ Term.ApplyInfix.After_4_6_0(lhs, Term.Name("==") | Term.Name("!="), _, Term.ArgClause(List(rhs), _)) => rule(lhs, rhs, t) + // Corresponds to myVar.equals(Double.Nan) case t @ Term.Apply.After_4_6_0(Term.Select(lhs, Term.Name("equals")), Term.ArgClause(List(right), _)) => rule(lhs, right, t) }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/OptionGet.scala b/be2-scala/linter/rules/src/main/scala/fix/OptionGet.scala index d53d889e91..f9cb1fc039 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/OptionGet.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/OptionGet.scala @@ -6,22 +6,14 @@ package fix import scalafix.lint.LintSeverity import scalafix.v1._ import scala.meta._ - -case class OptionGetDiag(tree: Tree) extends Diagnostic { - override def message: String = "Use of Option.Get" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Using Option.get defeats the purpose of using Option in the first place. Use the following instead: Option.getOrElse, Option.fold, pattern matching or don't take the value out of the container and map over it to transform it" - - override def position: Position = tree.pos -} - class OptionGet extends SemanticRule("OptionGet") { + private def diag(pos: Position) = Diagnostic("", "Use of Option.Get", pos, "Using Option.get defeats the purpose of using Option in the first place. Use the following instead: Option.getOrElse, Option.fold, pattern matching or don't take the value out of the container and map over it to transform it", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { - case t @ Term.Select(qual, Term.Name("get")) if Util.matchType(qual, "scala/Option") => Patch.lint(OptionGetDiag(t)) + // Corresponds to Option(_).get or o.get + case t @ Term.Select(qual, Term.Name("get")) if Util.matchType(qual, "scala/Option") => Patch.lint(diag(t.pos)) case _ => Patch.empty }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/OptionSize.scala b/be2-scala/linter/rules/src/main/scala/fix/OptionSize.scala index 6715995b06..b0905e2c35 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/OptionSize.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/OptionSize.scala @@ -7,21 +7,14 @@ import scalafix.lint.LintSeverity import scalafix.v1._ import scala.meta._ -case class OptionSizeDiag(tree: Tree) extends Diagnostic { - override def message: String = "Prefer Option.isDefined instead of Option.size" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Prefer to use Option.isDefined, Option.isEmpty or Option.nonEmpty instead of Option.size." - - override def position: Position = tree.pos -} - class OptionSize extends SemanticRule("OptionSize") { + private def diag(pos: Position) = Diagnostic("", "Use of Option.Size", pos, "Prefer to use Option.isDefined, Option.isEmpty or Option.nonEmpty instead of Option.size.", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { - case t @ Term.Select(qual, Term.Name("size")) if Util.matchType(qual, "scala/Option") => Patch.lint(OptionSizeDiag(t)) + // Corresponds to Option(_).size or o.size + case t @ Term.Select(qual, Term.Name("size")) if Util.matchType(qual, "scala/Option") => Patch.lint(diag(t.pos)) case _ => Patch.empty }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/StripMarginOnRegex.scala b/be2-scala/linter/rules/src/main/scala/fix/StripMarginOnRegex.scala index 694ddab256..92a1a83303 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/StripMarginOnRegex.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/StripMarginOnRegex.scala @@ -7,21 +7,22 @@ import scalafix.lint.LintSeverity import scalafix.v1._ import scala.meta._ -case class StripMarginOnRegexDiag(tree: Tree) extends Diagnostic { - override def message: String = "Strip margin on regex" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Strip margin will strip | from regex - possible corrupted regex." - - override def position: Position = tree.pos -} - class StripMarginOnRegex extends SemanticRule("StripMarginOnRegex") { + private def diag(pos: Position) = Diagnostic("", "Strip margin on regex", pos, "Strip margin will strip | from regex - possible corrupted regex.", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { - case t @ Term.Select(Term.Select(Lit.String(string), Term.Name("stripMargin")), Term.Name("r")) if string.contains('|') => Patch.lint(StripMarginOnRegexDiag(t)) + // Corresponds to "regex".stripMargin.r or myRegex.stripMargin.r + case t @ Term.Select(Term.Select(qual, Term.Name("stripMargin")), Term.Name("r")) => + qual match { + case name @ Term.Name(_) => Util.findDefinition(doc.tree, name) match { + case Lit.String(value) if value.contains('|') => Patch.lint(diag(t.pos)) + case _ => Patch.empty + } + case Lit.String(value) if value.contains('|') => Patch.lint(diag(t.pos)) + case _ => Patch.empty + } case _ => Patch.empty }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/TryGet.scala b/be2-scala/linter/rules/src/main/scala/fix/TryGet.scala index 8e07f1fa2f..6b1a3f6188 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/TryGet.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/TryGet.scala @@ -7,21 +7,14 @@ import scalafix.lint.LintSeverity import scalafix.v1._ import scala.meta._ -case class TryGetDiag(tree: Tree) extends Diagnostic { - override def message: String = "Use of Try.Get" - - override def severity: LintSeverity = LintSeverity.Error - - override def explanation: String = "Using Try.get is unsafe because it will throw the underlying exception in case of a Failure. Use the following instead: Try.getOrElse, Try.fold, pattern matching or don't take the value out of the container and map over it to transform it." - - override def position: Position = tree.pos -} - class TryGet extends SemanticRule("TryGet") { + private def diag(pos: Position) = Diagnostic("", "Use of Try.Get", pos, "Using Try.get defeats the purpose of using Try in the first place. Use the following instead: Try.getOrElse, Try.fold, pattern matching or don't take the value out of the container and map over it to transform it", LintSeverity.Error) + override def fix(implicit doc: SemanticDocument): Patch = { doc.tree.collect { - case t @ Term.Select(qual, Term.Name("get")) if Util.matchType(qual, "scala/util/Try") => Patch.lint(TryGetDiag(t)) + // Corresponds to Try{_}.get or t.get + case t @ Term.Select(qual, Term.Name("get")) if Util.matchType(qual, "scala/util/Try") => Patch.lint(diag(t.pos)) case _ => Patch.empty }.asPatch } diff --git a/be2-scala/linter/rules/src/main/scala/fix/Util.scala b/be2-scala/linter/rules/src/main/scala/fix/Util.scala index bd55e2f27c..fb57a0d5da 100644 --- a/be2-scala/linter/rules/src/main/scala/fix/Util.scala +++ b/be2-scala/linter/rules/src/main/scala/fix/Util.scala @@ -4,8 +4,17 @@ import scalafix.v1._ import scala.meta._ +/** Utilities class for linter */ object Util { - def getType(term: Term)(implicit doc: SemanticDocument): Symbol = { + + /** Get the type of a val/var. + * @param term The term / stat to get the type of + * @param doc The document to get the semantic information from + * @return The type of the val/var, or Symbol.None if not found + */ + // Type information is stored in the ValueSignature of the Term if it is a val/var. + // We pass the term a Stat as they have the same information, we can thus handle more cases and Term is a child of Stat + def getType(term: Stat)(implicit doc: SemanticDocument): Symbol = { term.symbol.info match { case Some(symInfo) => symInfo.signature match { case ValueSignature(TypeRef(_, symbol, _)) => symbol @@ -15,13 +24,26 @@ object Util { } } - def matchType(term: Term, symbols: String*)(implicit doc: SemanticDocument): Boolean = { + /** + * Compare the type of a term with the passed symbols + * @param term The term to compare + * @param symbols The symbols to compare with + * @param doc The document to get the semantic information from + * @return Whether or not the term matches one of the symbols + */ + def matchType(term: Stat, symbols: String*)(implicit doc: SemanticDocument): Boolean = { val symbolMatcher = SymbolMatcher.normalized(symbols: _*) symbolMatcher.matches(term.symbol) || symbolMatcher.matches(getType(term)) // Checks the term symbol matches that of the symbol (i.e. when we use the type directly), // or if the type of the variable matches. } + /** Find definition of val / var. + * @param tree The tree to search in + * @param name The name of the val / var to search for + * @return The definition of the val / var if found, null otherwise + */ + // We simply explore the tree and check the definitions and return if a name matches def findDefinition(tree: Tree, name: Term): Any = { tree.collect { case Defn.Val(_, List(Pat.Var(varName)), _, value) @@ -31,6 +53,12 @@ object Util { }.headOption.orNull } + /** Find multiple definitions in one tree traversal. + * @param tree The tree to search in + * @param nameSet The set of names to search for + * @return An unordered list of definitions found + */ + // Takes a set as argument as we will have to do many lookups def findDefinitions(tree: Tree, nameSet: Set[Term]): List[(Term, Any)] = { tree.collect { case Defn.Val(_, List(Pat.Var(varName)), _, value) if nameSet.exists(_.toString().equals(varName.value)) => nameSet.find(_.toString().equals(varName.value)).get -> value @@ -38,11 +66,15 @@ object Util { } } - // Finds multiple definitions in one tree traversal - def findDefinitionsMap(tree: Tree, nameSet: Set[Term]): Map[Term, Any] = { - findDefinitions(tree, nameSet).toMap - } - + /** + * Finds multiple definitions in one tree traversal, ordered with the order in the list + * @param tree The tree to search in + * @param nameSet The set of names to search for + * @return A list of the definitions in the order of the names + */ + /* Since findDefinitions take a set for efficiency, the order is lost. We use this function to restore it. + * It is still more efficient than passing a list to findDefinitions as here we have an algorithm that depends on the + * number of variables to find definitions, not on the number of lookups */ def findDefinitionsOrdered(tree: Tree, nameSet: List[Term]): List[Any] = { findDefinitions(tree, nameSet.toSet).sortBy { case (term, _) => nameSet.indexOf(term) }.map { case (_, value) => value } } diff --git a/be2-scala/project/plugins.sbt b/be2-scala/project/plugins.sbt index 9f9888b085..f2b020d81d 100644 --- a/be2-scala/project/plugins.sbt +++ b/be2-scala/project/plugins.sbt @@ -3,4 +3,4 @@ addSbtPlugin("com.sonar-scala" % "sbt-sonar" % "2.3.0") addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "2.1.5") addSbtPlugin("com.rallyhealth.sbt" % "sbt-git-versioning" % "1.6.0") addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.4.6") -addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.11.1") +addSbtPlugin("ch.epfl.scala" % "sbt-scalafix" % "0.12.1")