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

[cobr123]

https://youtrack.jetbrains.com/issue/SCL-9153/Warn-about-calling-toString-on-Option

[unkarjedy]

Thanks for the contribution!

I have some doubts of making this inspection enabled by default. (see comment https://youtrack.jetbrains.com/issue/SCL-9153/Warn-about-calling-toString-on-Option#focus=Comments-27-6452872.0-0)

[unkarjedy]

Also, the issue is actually broader and isn't only about explicit call of .toString. There are many cases when .toString is used indirectly. These cases are very widespread, not less then direct .toString usage:

//noinspection ScalaDeprecation
object Main {
  def main(args: Array[String]): Unit = {
    val value = Option("42")
    //val value = "42"
    //val value = Array("42")

    //directly calling toString
    println(value.toString)

    //using string concatenation
    println("prefix: " + value)

    //using string interpolation
    println(s"prefix: $value")

    //using various format-ed methods
    println("formatted: %s".formatted(value))
    println(value.formatted("formatted: %s"))
    println(String.format("formatted: %s", value))
    //etc...

    //passing to println and other print* methods
    println(value)
    //etc..

    //using string builder from java
    val builder1 = new java.lang.StringBuilder
    builder1.append(value)
    println(builder1)

    //using string builder from scala
    val builder2 = new scala.collection.mutable.StringBuilder
    builder2.append(value)
    println(builder2)

    //etc...
  }
}

And current fix doesn't detect them:

These cases are handled for Array.toString inspection. See org.jetbrains.plugins.scala.codeInspection.collections.MakeArrayToStringInspection. Though, I now see that it's also done imperfectly and it also can be improved:

Anyway it would be nice to have at least same level of handling as in org.jetbrains.plugins.scala.codeInspection.collections.MakeArrayToStringInspection. Ideally extracting some code to reusable utility.

[unkarjedy]

Also, I don't think that we should provide a "quick fix" which leads to a runtime error. As was mentioned Option.toString has quite meaningful toString representation. Replacing it with exception throwing doesn't look good. Note that calling of Option.toString might be caused not only by "accidental" replacement of nullable value with optional, but intentionally. And in nullable version NullPointerException is only thrown if toString is called explicitly on null value.

Maybe we should use .mkString method similarly to MakeArrayToStringInspection. None would be transformed to empty string in this case.

It won't be the same as with nullable value, because it most of the cases from previous comment null will be handled and "null" string literal will be used instead. But again, we don't need to emulate that 1-to-1 because we can't be sure that Option.toString method was introduced after refactoring from nullable to Option

[unkarjedy]

Also it should be consistent with MakeArrayToStringInspection in which range to highlight:

[cobr123]

NoSuchElementException was used because of main use case is when toString accidentally called on sys.env.get("host") and then failing at runtime.

[nafg]

That might be your use case but that doesn't mean it's the right way to implement it

On Tue Sep 20, 2022, 07:55 AM GMT, Roman Tabulov @.***> wrote:

NoSuchElementException was used because of main use case is when toString accidentally called on sys.env.get("host") and then failing at runtime. — Reply to this email directly, view it on GitHub https://github.com/JetBrains/intellij-scala/pull/631#issuecomment-1251979428, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYAUCECS4KNORJSZD22V3V7FUXLANCNFSM6AAAAAAQPMZWKM. You are receiving this because you are subscribed to this thread.Message ID: @.***>

[cobr123]

It`s not clear how to use mkString on Option. There is no point to emulate toString.

[unkarjedy]

NoSuchElementException was used because of main use case is when toString accidentally called on sys.env.get("host") and then failing at runtime.

It's one of the many many possible use cases. And even in this particular it's not obvious how to deal with it. In many applications there are properties/environment variables which are optional and throwing exception would not be desirable. Anyway toString, Option and nullable values are too basic concepts which are used all over the places, not only in sys.env.get

It`s not clear how to use mkString on Option

Like this:

Option(42).mkString //equals to "42"
None.mkString //equalst to empty string ""

There is no point to emulate toString.

Sorry, don't think I understood you. What do you mean by "emulate toString" and why "no point"?

[nafg]

mkString is not necessarily the best answer either. Maybe the current type has a good toString and later it changes to another type that doesn't. Besides, None usually shouldn't be an empty string.

Why not use fold? Usually when I need to convert an Option to a String that's what I end up doing, e.g. myOption.fold("none")(_.summaryString)

[unkarjedy]

Or it could be consistent with nullable values and be "null" when empty. Less magic constants would be introduced

[unkarjedy]

@cobr123 I see new commits incoming. Please ping me when you finish with the changes, I will have another look.

[cobr123]

@unkarjedy, I finished

[cobr123]

@unkarjedy, now finished for sure

[nafg]

I really don't consider mkString the best option. At I explained if the Option is None it will print nothing which will usually look weird and isn't what you want.

The second issue is that it hides the fact that it's essentially calling toString on whatever's inside the Option. This is often as bad as calling toString on an Option, except that it's not explicit so the IDE can't help and to change it requires replacing the mkString.

This could be solved with fold, as I said. The equivalent of mkString with fold is .fold("")(_.toString). If you think fold might be unfamiliar too many people an equivalent would be .map(_.toString).getOrElse("").

Personally I almost always use fold, though.