diff --git a/scala/scala-impl/resources/META-INF/scala-plugin-common.xml b/scala/scala-impl/resources/META-INF/scala-plugin-common.xml index afc801d2f0b..802b0d6ae14 100644 --- a/scala/scala-impl/resources/META-INF/scala-plugin-common.xml +++ b/scala/scala-impl/resources/META-INF/scala-plugin-common.xml @@ -1769,6 +1769,12 @@ groupPathKey="grouppath.scala.collections" groupKey="group.options" shortName="IfElseToFilterdOption" level="WARNING" enabledByDefault="true" language="Scala"/> + + +

Reports and replaces toString with map(_.toString).getOrElse("").

+ +

map(_.toString).getOrElse("") will prevent from getting string "Some(a)" instead of expected string "a".

+ +

Example:

+

+  Option(a).toString
+
+

After the quick-fix is applied:

+

+  Option(a).map(_.toString).getOrElse("")
+
+ + + \ No newline at end of file diff --git a/scala/scala-impl/resources/messages/ScalaInspectionBundle.properties b/scala/scala-impl/resources/messages/ScalaInspectionBundle.properties index e0030f3e873..9a246503fd2 100644 --- a/scala/scala-impl/resources/messages/ScalaInspectionBundle.properties +++ b/scala/scala-impl/resources/messages/ScalaInspectionBundle.properties @@ -133,6 +133,7 @@ displayname.map.and.getorelse.false.to.exists=Map and getOrElse(false) to exists displayname.getorelse.null.to.ornull=GetOrElse(null) to orNull displayname.emulated.option.x=Emulated Option(x) displayname.change.to.filter=Change to filter +displayname.change.tostring.on.option=Change toString to getOrElse("") displayname.some.to.option=Some to Option displayname.filter.after.sort=Filter after sort displayname.redundant.collection.conversion=Redundant collection conversion @@ -307,6 +308,9 @@ fold.true.and.hint=Replace fold with forall ### org/jetbrains/plugins/scala/codeInspection/collections/GetGetOrElseInspection.scala get.getOrElse.hint=Replace with getOrElse(key, defaultValue) +### org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspection.scala +option.toString.hint=Format with map(_.toString).getOrElse("") + ### org/jetbrains/plugins/scala/codeInspection/collections/GetOrElseNullInspection.scala getOrElse.null.hint=Replace getOrElse(null) with orNull diff --git a/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspection.scala b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspection.scala index cb456872bea..d8e994066cd 100644 --- a/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspection.scala +++ b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspection.scala @@ -2,60 +2,19 @@ package org.jetbrains.plugins.scala package codeInspection package collections -import org.jetbrains.plugins.scala.lang.psi.api.base.ScInterpolatedStringLiteral import org.jetbrains.plugins.scala.lang.psi.api.expr.ScExpression import scala.collection.immutable.ArraySeq -class MakeArrayToStringInspection extends OperationOnCollectionInspection { +class MakeArrayToStringInspection extends OperationOnCollectionInspection { override def possibleSimplificationTypes: ArraySeq[SimplificationType] = ArraySeq(MakeArrayToStringInspection) } object MakeArrayToStringInspection extends SimplificationType { override def hint: String = ScalaInspectionBundle.message("format.with.mkstring") - private val `print` = unqualifed(Set("print", "println")).from(ArraySeq("scala.Predef", "java.io.PrintStream")) - private val `.print` = invocation(Set("print", "println")).from(ArraySeq("scala.Predef", "java.io.PrintStream")) - private val `+` = invocation(Set("+")) + private def mkString(arr: ScExpression): String = """mkString("Array(", ", ", ")")""" - private val mkString = """mkString("Array(", ", ", ")")""" - - override def getSimplification(expr: ScExpression): Option[Simplification] = { - expr match { - // TODO infix notation? - case `.toString`(array) if isArray(array) => - // array.toString - Some(replace(expr).withText(invocationText(array, mkString)).highlightFrom(array)) - case someString `+` array if isString(someString) && isArray(array) => - // "string" + array - Some(replace(array).withText(invocationText(array, mkString)).highlightFrom(array)) - case array `+` someString if isString(someString) && isArray(array) => - // array + "string" - Some(replace(array).withText(invocationText(array, mkString)).highlightFrom(array)) - case _ if isArray(expr) => - def result: SimplificationBuilder = replace(expr).withText(invocationText(expr, mkString)).highlightFrom(expr) - - expr.getParent match { - case _: ScInterpolatedStringLiteral => - // s"start $array end" - Some(result.wrapInBlock()) - case null => None - case parent => - parent.getParent match { - case `.print`(_, args@_*) if args.contains(expr) => - // System.out.println(array) - Some(result) - case `print` (args@_*) if args.contains(expr) => - // println(array) - Some(result) - case _: ScInterpolatedStringLiteral => - // s"start ${array} end" - Some(result) - case _ => None - } - } - case _ => - None - } - } + override def getSimplification(expr: ScExpression): Option[Simplification] = + getToStringSimplification(expr, isArray, mkString, replace) } diff --git a/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspection.scala b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspection.scala new file mode 100644 index 00000000000..449cb92f101 --- /dev/null +++ b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspection.scala @@ -0,0 +1,53 @@ +package org.jetbrains.plugins.scala.codeInspection.collections + +import org.jetbrains.plugins.scala.codeInspection.ScalaInspectionBundle +import org.jetbrains.plugins.scala.lang.psi.api.expr.ScExpression +import org.jetbrains.plugins.scala.lang.psi.types.ScParameterizedType +import org.jetbrains.plugins.scala.lang.psi.types.api.designator.{ScDesignatorType, ScProjectionType} + +import scala.collection.immutable.ArraySeq + +class OptionToStringInspection extends OperationOnCollectionInspection { + override def possibleSimplificationTypes: ArraySeq[SimplificationType] = ArraySeq(OptionToStringInspection) +} + +object OptionToStringInspection extends SimplificationType { + + override def hint: String = ScalaInspectionBundle.message("option.toString.hint") + + override def getSimplification(expr: ScExpression): Option[Simplification] = + getToStringSimplification(expr, isOption, mkString, replace) + + private def mkString(option: ScExpression): String = { + option match { + case scalaNone() => """getOrElse("null")""" + case _ => + option.getTypeAfterImplicitConversion().tr match { + case Right(t: ScParameterizedType) if isStringInOption(t) => onString + case Right(t: ScDesignatorType) if isStringInOption(t) => onString + case Right(t: ScProjectionType) if isStringInOption(t) => onString + case _ => onNotString + } + } + } + + private def isStringInOption(t: ScParameterizedType): Boolean = t.typeArguments.forall(SizeToLength.isString) + + private def isStringInOption(t: ScDesignatorType): Boolean = { + t.extractDesignatorSingleton match { + case Some(t: ScParameterizedType) if isStringInOption(t) => true + case _ => false + } + } + + private def isStringInOption(t: ScProjectionType): Boolean = { + t.extractDesignatorSingleton match { + case Some(t: ScParameterizedType) if isStringInOption(t) => true + case _ => false + } + } + + private def onString: String = """getOrElse("")""" + + private def onNotString: String = """map(_.toString).getOrElse("")""" +} diff --git a/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/package.scala b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/package.scala index 0fe2d1ddec3..8d568d4baa0 100644 --- a/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/package.scala +++ b/scala/scala-impl/src/org/jetbrains/plugins/scala/codeInspection/collections/package.scala @@ -9,7 +9,7 @@ import org.jetbrains.plugins.scala.extensions._ import org.jetbrains.plugins.scala.lang.psi.ScalaPsiUtil import org.jetbrains.plugins.scala.lang.psi.api.ScalaRecursiveElementVisitor import org.jetbrains.plugins.scala.lang.psi.api.base.patterns.ScCaseClauses -import org.jetbrains.plugins.scala.lang.psi.api.base.{ScLiteral, ScReference} +import org.jetbrains.plugins.scala.lang.psi.api.base.{ScInterpolatedStringLiteral, ScLiteral, ScReference} import org.jetbrains.plugins.scala.lang.psi.api.expr._ import org.jetbrains.plugins.scala.lang.psi.api.statements.params.ScParameter import org.jetbrains.plugins.scala.lang.psi.api.statements.{ScFunction, ScFunctionDefinition, ScValue, ScVariable} @@ -512,5 +512,67 @@ package object collections { def start: Int = elem.getTextRange.getStartOffset def end: Int = elem.getTextRange.getEndOffset } + + private val `print` = unqualifed(Set("print", "println")).from(ArraySeq("scala.Predef", "java.io.PrintStream")) + private val `.print` = invocation(Set("print", "println")).from(ArraySeq("scala.Predef", "java.io.PrintStream")) + private val `.formatted` = invocation("formatted").from(ArraySeq("scala.Predef.StringFormat", "java.lang.String")) + private val `.format` = invocation("format").from(ArraySeq("java.lang.String", "scala.collection.StringOps")) + private val `.appendOnStringBuilder` = invocation("append").from(ArraySeq("java.lang.StringBuilder", "scala.collection.mutable.StringBuilder")) + + private[collections] def getToStringSimplification(expr: ScExpression, isThing: ScExpression => Boolean, thingToString: ScExpression => String, replace: ScExpression => SimplificationBuilder): Option[Simplification] = { + expr match { + // TODO infix notation? + case `.toString`(thing) if isThing(thing) => + // thing.toString + Some(replace(expr).withText(invocationText(thing, thingToString(thing))).highlightFrom(thing)) + case someString `+` thing if isString(someString) && isThing(thing) => + // "string" + thing + Some(replace(thing).withText(invocationText(thing, thingToString(thing))).highlightFrom(thing)) + case thing `+` someString if isString(someString) && isThing(thing) => + // thing + "string" + Some(replace(thing).withText(invocationText(thing, thingToString(thing))).highlightFrom(thing)) + case thing `.formatted` someString if isString(someString) && isThing(thing) => + // thing.formatted("%s") + val thingText = thing.getText + val toStringThing = thingToString(thing) + Some(replace(expr).withText(invocationText(someString, s"format($thingText.$toStringThing)")).highlightElem(thing)) + case _ if isThing(expr) => + def result: SimplificationBuilder = replace(expr).withText(invocationText(expr, thingToString(expr))).highlightFrom(expr) + + expr.getParent match { + case _: ScInterpolatedStringLiteral => + // s"start $thing end" + Some(result.wrapInBlock()) + case null => None + case parent => + parent.getParent match { + case `.print`(_, args@_*) if args.contains(expr) => + // System.out.println(thing) + Some(result) + case `print`(args@_*) if args.contains(expr) => + // println(thing) + Some(result) + case `.format`(_, args@_*) if args.contains(expr) => + // String.format("%s", thing) + // "%s".format(thing) + Some(result) + case `.formatted`(_, args@_*) if args.contains(expr) => + // "%s".formatted(thing) + Some(result) + case `.appendOnStringBuilder`(_, args@_*) if args.contains(expr) + && args.exists(_.smartExpectedType().exists(_.isAny)) => + // new java.lang.StringBuilder.append(thing) + // new scala.collection.mutable.StringBuilder.append(thing) + Some(result) + case _: ScInterpolatedStringLiteral => + // s"start ${thing} end" + Some(result) + case _ => None + } + } + case _ => + None + } + } } diff --git a/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspectionTest.scala b/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspectionTest.scala index b53954de13c..47bb13af696 100644 --- a/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspectionTest.scala +++ b/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/MakeArrayToStringInspectionTest.scala @@ -2,6 +2,8 @@ package org.jetbrains.plugins.scala package codeInspection package collections +import org.junit.Assert.assertEquals + class MakeArrayToStringInspectionTest extends OperationsOnCollectionInspectionTest { override protected val classOfInspection: Class[_ <: OperationOnCollectionInspection] = @@ -114,4 +116,108 @@ class MakeArrayToStringInspectionTest extends OperationsOnCollectionInspectionTe |"test" + a.mkString("Array(", ", ", ")") """.stripMargin) } + + def testStringFormat(): Unit = { + doTest( + s""" + |String.format("formatted: %s", ${START}Array(1)$END) + """.stripMargin, + """ + |String.format("formatted: %s", Array(1)) + """.stripMargin, + """ + |String.format("formatted: %s", Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } + + def testFormatOnString(): Unit = { + doTest( + s""" + |"formatted: %s".format(${START}Array(1)$END) + """.stripMargin, + """ + |"formatted: %s".format(Array(1)) + """.stripMargin, + """ + |"formatted: %s".format(Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } + + def testFormattedOnString(): Unit = { + doTest( + s""" + |"formatted: %s".formatted(${START}Array(1)$END) + """.stripMargin, + """ + |"formatted: %s".formatted(Array(1)) + """.stripMargin, + """ + |"formatted: %s".formatted(Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } + + def testFormatted(): Unit = { + doTest( + s""" + |${START}Array(1)$END.formatted("formatted: %s") + """.stripMargin, + """ + |Array(1).formatted("formatted: %s") + """.stripMargin, + """ + |"formatted: %s".format(Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } + + def testAppendNotAnyOnJavaStringBuilder(): Unit = { + try { + doTest( + s""" + |val b = new java.lang.StringBuilder + |b.append(${START}Array('1')$END) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Array('1')) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Array('1')) + """.stripMargin) + } catch { + case e: AssertionError => assertEquals(e.getMessage, """Highlights not found: Format with .mkString("Array(", ", ", ")")""") + } + } + + def testAppendAnyOnJavaStringBuilder(): Unit = { + doTest( + s""" + |val b = new java.lang.StringBuilder + |b.append(${START}Array(1)$END) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Array(1)) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } + + def testAppendOnScalaStringBuilder(): Unit = { + doTest( + s""" + |val b = new scala.collection.mutable.StringBuilder + |b.append(${START}Array(1)$END) + """.stripMargin, + """ + |val b = new scala.collection.mutable.StringBuilder + |b.append(Array(1)) + """.stripMargin, + """ + |val b = new scala.collection.mutable.StringBuilder + |b.append(Array(1).mkString("Array(", ", ", ")")) + """.stripMargin) + } } diff --git a/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspectionTest.scala b/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspectionTest.scala new file mode 100644 index 00000000000..9ce9f188c39 --- /dev/null +++ b/scala/scala-impl/test/org/jetbrains/plugins/scala/codeInspection/collections/OptionToStringInspectionTest.scala @@ -0,0 +1,265 @@ +package org.jetbrains.plugins.scala.codeInspection.collections + +import org.jetbrains.plugins.scala.codeInspection.ScalaInspectionBundle + +class OptionToStringInspectionTest extends OperationsOnCollectionInspectionTest { + + override protected val classOfInspection: Class[_ <: OperationOnCollectionInspection] = + classOf[OptionToStringInspection] + + override protected val hint: String = ScalaInspectionBundle.message("option.toString.hint") + + def testNone(): Unit = { + doTest( + s"""None.${START}toString$END""", + """None.toString""", + """None.getOrElse("null")""" + ) + } + + def testOptionVal(): Unit = { + doTest( + s"""val i = Option("hello"); i.${START}toString$END""", + """val i = Option("hello"); i.toString""", + """val i = Option("hello"); i.getOrElse("")""" + ) + } + + def testOptionValInObject(): Unit = { + doTest( + s"""object Test { val i = Option("hello"); i.${START}toString$END }""", + """object Test { val i = Option("hello"); i.toString }""", + """object Test { val i = Option("hello"); i.getOrElse("") }""" + ) + } + + def testFunctionExpression(): Unit = { + doTest( + s"""sys.env.get("VARIABLE").${START}toString$END""", + """sys.env.get("VARIABLE").toString""", + """sys.env.get("VARIABLE").getOrElse("")""" + ) + } + + def testSomeConstant(): Unit = { + doTest( + s"""Some("constant").${START}toString$END""", + """Some("constant").toString""", + """Some("constant").getOrElse("")""" + ) + } + + def testOptionConstant(): Unit = { + doTest( + s"""Option("constant").${START}toString$END""", + """Option("constant").toString""", + """Option("constant").getOrElse("")""" + ) + } + + def testOptionValNotString(): Unit = { + doTest( + s"""val i = Option(1); i.${START}toString$END""", + """val i = Option(1); i.toString""", + """val i = Option(1); i.map(_.toString).getOrElse("")""" + ) + } + + def testFunctionExpressionNotString(): Unit = { + doTest( + s"""def getSomeOne():Option[Int] = Some(1); getSomeOne().${START}toString$END""", + """def getSomeOne():Option[Int] = Some(1); getSomeOne().toString""", + """def getSomeOne():Option[Int] = Some(1); getSomeOne().map(_.toString).getOrElse("")""" + ) + } + + def testSomeConstantNotString(): Unit = { + doTest( + s"""Some(1).${START}toString$END""", + """Some(1).toString""", + """Some(1).map(_.toString).getOrElse("")""" + ) + } + + def testOptionConstantNotString(): Unit = { + doTest( + s"""Option(1).${START}toString$END""", + """Option(1).toString""", + """Option(1).map(_.toString).getOrElse("")""" + ) + } + + def testPrint(): Unit = { + doTest( + s""" + |println(${START}Option(3)$END) + """.stripMargin, + """ + |println(Option(3)) + """.stripMargin, + """ + |println(Option(3).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testSystemPrint(): Unit = { + doTest( + s""" + |System.out.println(${START}Option(3)$END) + """.stripMargin, + """ + |System.out.println(Option(3)) + """.stripMargin, + """ + |System.out.println(Option(3).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testInterpolatedString(): Unit = { + doTest( + s""" + |val a = Option(3) + |s"before $$${START}a$END after" + """.stripMargin, + """ + |val a = Option(3) + |s"before $a after" + """.stripMargin, + """ + |val a = Option(3) + |s"before ${a.map(_.toString).getOrElse("")} after" + """.stripMargin) + } + + def testBlockInInterpolatedString(): Unit = { + doTest( + s""" + |val a = Option(3) + |s"before $${${START}a$END} after" + """.stripMargin, + """ + |val a = Option(3) + |s"before ${a} after" + """.stripMargin, + """ + |val a = Option(3) + |s"before ${a.map(_.toString).getOrElse("")} after" + """.stripMargin) + } + + def testAny2String(): Unit = { + doTest( + s""" + |val a = Option(3) + |${START}a$END + "test" + """.stripMargin, + """ + |val a = Option(3) + |a + "test" + """.stripMargin, + """ + |val a = Option(3) + |a.map(_.toString).getOrElse("") + "test" + """.stripMargin) + } + + def testAddToString(): Unit = { + doTest( + s""" + |val a = Option(3) + |"test" + ${START}a$END + """.stripMargin, + """ + |val a = Option(3) + |"test" + a + """.stripMargin, + """ + |val a = Option(3) + |"test" + a.map(_.toString).getOrElse("") + """.stripMargin) + } + + def testStringFormat(): Unit = { + doTest( + s""" + |String.format("formatted: %s", ${START}Option(1)$END) + """.stripMargin, + """ + |String.format("formatted: %s", Option(1)) + """.stripMargin, + """ + |String.format("formatted: %s", Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testFormatOnString(): Unit = { + doTest( + s""" + |"formatted: %s".format(${START}Option(1)$END) + """.stripMargin, + """ + |"formatted: %s".format(Option(1)) + """.stripMargin, + """ + |"formatted: %s".format(Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testFormattedOnString(): Unit = { + doTest( + s""" + |"formatted: %s".formatted(${START}Option(1)$END) + """.stripMargin, + """ + |"formatted: %s".formatted(Option(1)) + """.stripMargin, + """ + |"formatted: %s".formatted(Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testFormatted(): Unit = { + doTest( + s""" + |${START}Option(1)$END.formatted("formatted: %s") + """.stripMargin, + """ + |Option(1).formatted("formatted: %s") + """.stripMargin, + """ + |"formatted: %s".format(Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testAppendOnJavaStringBuilder(): Unit = { + doTest( + s""" + |val b = new java.lang.StringBuilder + |b.append(${START}Option(1)$END) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Option(1)) + """.stripMargin, + """ + |val b = new java.lang.StringBuilder + |b.append(Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } + + def testAppendOnScalaStringBuilder(): Unit = { + doTest( + s""" + |val b = new scala.collection.mutable.StringBuilder + |b.append(${START}Option(1)$END) + """.stripMargin, + """ + |val b = new scala.collection.mutable.StringBuilder + |b.append(Option(1)) + """.stripMargin, + """ + |val b = new scala.collection.mutable.StringBuilder + |b.append(Option(1).map(_.toString).getOrElse("")) + """.stripMargin) + } +}