Skip to content

Check OrType in interpolated toString lint #23365

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

Merged
merged 2 commits into from
Jul 22, 2025

Conversation

som-snytt
Copy link
Contributor

Fixes #23361

If an arg to a interpolator is a conditional or match, the expression type may be an OrType, so check both parts for non-Strings for which toString is used for interpolation.

@som-snytt som-snytt force-pushed the issue/23361-tostring-warn branch from 4745529 to d23269a Compare July 11, 2025 23:58
@som-snytt som-snytt marked this pull request as ready for review July 11, 2025 23:58
@Gedochao Gedochao requested a review from tgodzik July 14, 2025 09:30
Comment on lines 231 to 246
def check(tp: Type): Boolean = tp.widen match
case OrType(tp1, tp2) =>
check(tp1) || check(tp2)
case tp =>
if tp =:= defn.StringType then
false
else if tp =:= defn.UnitType then
warningAt(CC)("interpolated Unit value")
true
else if !tp.isPrimitiveValueType then
warningAt(CC)("interpolation uses toString")
true
else
false
if ctx.settings.Whas.toStringInterpolated && kind == StringXn && check(arg) then
()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def check(tp: Type): Boolean = tp.widen match
case OrType(tp1, tp2) =>
check(tp1) || check(tp2)
case tp =>
if tp =:= defn.StringType then
false
else if tp =:= defn.UnitType then
warningAt(CC)("interpolated Unit value")
true
else if !tp.isPrimitiveValueType then
warningAt(CC)("interpolation uses toString")
true
else
false
if ctx.settings.Whas.toStringInterpolated && kind == StringXn && check(arg) then
()
def check(tp: Type): Boolean = tp.widen match
case OrType(tp1, tp2) =>
check(tp1) || check(tp2)
case tp =>
if tp =:= defn.UnitType then
warningAt(CC)("interpolated Unit value")
else if !tp.isPrimitiveValueType then
warningAt(CC)("interpolation uses toString")
if ctx.settings.Whas.toStringInterpolated && kind == StringXn then
check(arg)

do we need the boolean at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same with compiler/src/dotty/tools/dotc/transform/localopt/StringInterpolatorOpt.scala

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably it was to avoid discarding the boolean result in the if setting then check. That result is useful in the OrType branch to avoid warning twice.

Usually I would (condition && { sideeffect(); true } but I guess I didn't have room on my whiteboard that day to work it out.

I don't remember if -Wnonunit-statement warns here, and whether check(arg): Unit is enough to say that discard was intentional. if without else is inherently value-discarding, but does it warn about it?

check || check could be expressed as a test of state: check(); if !reporter.hasWarning then checkAgain(). Or to push it further, I can check if a tree type isErroneous, but not if it isDubious (i.e. it causes a warning).

For some reason, warnings aren't captured in trees, perhaps because they were deemed lightweight side effects. But if x is deprecated and there is a rewrite to y, then it's useful (in a code editor) to test tree.isDubious. Currently we emit red squiggles by the position of errors or yellow for warnings, IIUC.

I'll reconsider the idiom here and also the name check.

@som-snytt
Copy link
Contributor Author

-Wvalue-discard warns but not -Wnonunit-statement

10 |    check()
   |    ^^^^^^^
   |discarded non-Unit value of type Boolean. Add `: Unit` to discard silently.

Scala 2 has -Wnonunit-if for that behavior, IIRC.

@som-snytt som-snytt force-pushed the issue/23361-tostring-warn branch from d23269a to d7efc38 Compare July 22, 2025 14:31
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@@ -105,13 +105,25 @@ class StringInterpolatorOpt extends MiniPhase:
lintToString(elem)
concat(elem)
val str = stri.next()
if !str.const.stringValue.isEmpty then concat(str)
if !str.const.stringValue.isEmpty then
concat(str)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is due to @sjrd asking please don't put side effects to the right. Now I need it more left-adjusted, "front and center".

@som-snytt som-snytt enabled auto-merge (rebase) July 22, 2025 15:16
@som-snytt som-snytt merged commit 2887c0a into scala:main Jul 22, 2025
31 checks passed
@som-snytt som-snytt deleted the issue/23361-tostring-warn branch July 25, 2025 01:35
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.

-Wtostring-interpolated warns on an if expression of type String
2 participants