Optional.toString() should throw error

[Priyadarshanvijay]

Before this PR

Any usages of Optional.toString() are allowed and cause hard to debug runtime bugs where the actual serialized string is either of:

Optional[ACTUAL_VALUE]
Optional.empty

depending on the value contained by the optional.

After this PR

Usages of Optional.toString() will be discouraged and users will be prompted to use Optional.get().toString() instead.

==COMMIT_MSG==
Optional.toString() should throw error
==COMMIT_MSG==

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Optional.toString() should throw error

Check the box to generate changelog(s)

• Generate changelog entry

[pkoenig10]

I don't think we want to suggest Optional.get().toString(). This is not always appropriate, as the optional may be empty.

Even if we do want to suggest a fix, the right way to do this is with Error Prone's suggested fixes, rather than in the description, so that the fixes can be applied automatically.

[Priyadarshanvijay]

Agreed, but not sure how we can use Error Prone's suggested fixes here since like you highlighted, the actual fix depends on the context

[pkoenig10]

If we want this, we need to devise a roll out plan, since there may be a lot of existing code that does this and we don't want Baseline upgrades to be blocked.

Typically we do this by automating the fix, but that seems more difficult here since the correct fix is context dependnent.

[Priyadarshanvijay]

Would changing SeverityLevel to WARNING instead of error unblock the upgrades? Or should we suggest people to add SuppressWarnings if this is the behavior they want?
I think if there are many places in existing codebases where people are using this then this probably points to some runtime error in their code and shouldn't it be better to fix it manually?

[carterkozak]

cause hard to debug runtime bugs

Can you provide some examples? Most of the uses of Optional#toString I'm aware of are within toString implementations where I'd argue they're entirely reasonable. Runtime behavior generally shouldn't depend on calling Object.toString.

[carterkozak]

Optional#toString does stringify the contained value, and decorates the result with Optional[<delegate string value>] to make it clear that the value is an optional.

[Priyadarshanvijay]

cause hard to debug runtime bugs

Can you provide some examples? Most of the uses of Optional#toString I'm aware of are within toString implementations where I'd argue they're entirely reasonable. Runtime behavior generally shouldn't depend on calling Object.toString.

This typically happens when returning string values from functions and asserting over them/storing them in a datastore. We caught this issue in a few of our ETE tests while making changes and it was difficult to debug why things were failing. Once we found the issue, we thought it would be better if our IDE/gradle checks could warn us about this since we almost always want the stringified value of the object wrapped in Optional rather than the decorated string.

If it's not a common occurance, can we add this as an Experimental check which needs to be turned on manually? (Not sure if this is even possible)

[pkoenig10]

returning string values from functions

Can you clarify what you mean by this? Are you describing a Java method that returns String? Or something else?

If you are describing a Java method that returns String, what does that have to do with Optional.toString()?

storing them in a datastore

we almost always want the stringified value of the object

Why are we storing stringified values in a database? toString is generally a one way conversion that has no backwards compatibility guarantees. We should almost always be persisting data in a structured format (ex. JSON, etc.).