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

Conversation

cobr123
Copy link
Contributor

@cobr123 cobr123 commented Sep 18, 2022

@unkarjedy
Copy link
Member

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
Copy link
Member

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:
image

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:
image

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
Copy link
Member

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

image

@unkarjedy
Copy link
Member

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

image

@unkarjedy unkarjedy self-requested a review September 19, 2022 16:29
@cobr123
Copy link
Contributor Author

cobr123 commented Sep 20, 2022

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

@nafg
Copy link

nafg commented Sep 20, 2022 via email

@cobr123
Copy link
Contributor Author

cobr123 commented Sep 20, 2022

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

@unkarjedy
Copy link
Member

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
Copy link

nafg commented Sep 20, 2022

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
Copy link
Member

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

@unkarjedy
Copy link
Member

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

@cobr123
Copy link
Contributor Author

cobr123 commented Sep 22, 2022

@unkarjedy, I finished

@cobr123
Copy link
Contributor Author

cobr123 commented Sep 22, 2022

@unkarjedy, now finished for sure

@nafg
Copy link

nafg commented Sep 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants