-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
4745529
to
d23269a
Compare
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 | ||
() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
Scala 2 has |
d23269a
to
d7efc38
Compare
There was a problem hiding this 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) |
There was a problem hiding this comment.
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".
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 whichtoString
is used for interpolation.