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

SCL-9153 Warn about calling .toString() on Option #631

Open
wants to merge 19 commits into
base: idea222.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions scala/scala-impl/resources/META-INF/scala-plugin-common.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,12 @@
groupPathKey="grouppath.scala.collections" groupKey="group.options"
shortName="IfElseToFilterdOption" level="WARNING"
enabledByDefault="true" language="Scala"/>
<localInspection implementationClass="org.jetbrains.plugins.scala.codeInspection.collections.OptionToStringInspection"
bundle="messages.ScalaInspectionBundle"
key="displayname.change.tostring.on.option"
groupPathKey="grouppath.scala.collections" groupKey="group.options"
shortName="OptionToString" level="WARNING"
enabledByDefault="false" language="Scala"/>
<localInspection implementationClass="org.jetbrains.plugins.scala.codeInspection.collections.SomeToOptionInspection"
bundle="messages.ScalaInspectionBundle"
key="displayname.some.to.option"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<html>
<body>
<p>Reports and replaces <code>toString</code> with <code>map(_.toString).getOrElse("")</code>.</p>

<p><code>map(_.toString).getOrElse("")</code> will prevent from getting string <code>"Some(a)"</code> instead of expected string <code>"a"</code>.</p>

<p><b>Example:</b></p>
<pre><code>
Option(a).toString
</code></pre>
<p>After the quick-fix is applied:</p>
<pre><code>
Option(a).map(_.toString).getOrElse("")
</code></pre>
<!-- tooltip end -->
</body>
</html>
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Original file line number Diff line number Diff line change
@@ -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("")"""
}
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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
}
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -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] =
Expand Down Expand Up @@ -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)
}
}
Loading