Either lint unused or warn discarded or warn pure

[som-snytt]

If they ask for -Wnonunit-statement, warn; otherwise, if they ask for -Wvalue-discard, warn for that.

If they don't ask, warn about pure expressions that aren't useful.

Move the value discard messaging to refchecks, in order to coordinate. Follow up to #8995.

Scala 3 won't mitigate warning value discard under expr: Unit syntax, so don't advertise it.

The mitigation is not removed, for convenience, and because no one writes normal code like that.

To see how to suppress warnings, use -Wconf:any:warning-verbose in Scala 2 and -Wconf:any:verbose in Scala 3.

Extracted from stalled #10476

[SethTisue]

Is there a different recommendation we could substitute?

[som-snytt]

Every warning is nowarnable. Unfortunately, Scala 3 has different syntax and categories and messages, so there is no specific advice that cross-compiles.

[SethTisue]

What is the recommended mitigation in Scala 3?

I'm asking for a couple reasons, one is that I'd like the PR description to document the current situation for users, the other is that maybe we could do something on the Scala 3 side.

[som-snytt]

scala -Wvalue-discard -Wnonunit-statement -Wconf:any:warning-verbose
Welcome to Scala 2.13.12 (OpenJDK 64-Bit Server VM, Java 21.0.2).
Type in expressions for evaluation. Or try :help.

scala> def f = 42
def f: Int

scala> def g: Unit = f
                     ^
       warning: discarded non-Unit value of type Int
       Applicable -Wconf / @nowarn filters for this warning: msg=<part of the message>, cat=w-flag-value-discard, site=g
                     ^
       warning: unused value of type Int (add `: Unit` to discard silently)
       Applicable -Wconf / @nowarn filters for this warning: msg=<part of the message>, cat=other-pure-statement, site=g
def g: Unit

scala>

and

scala -Wvalue-discard -Wnonunit-statement -Wconf:any:verbose
Welcome to Scala 3.3.1 (21.0.2, Java OpenJDK 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> def f = 42
def f: Int

scala> def g: Unit = f
1 warning found
-- [E175] Potential Issue Warning: -------------------------------------------------------------------------------------
1 |def g: Unit = f
  |              ^
  |              discarded non-Unit value of type (f : => Int)
Matching filters for @nowarn or -Wconf:
  - id=E175
  - name=ValueDiscarding
def g: Unit

TIL dotty doesn't warn nonunit-statement for value discard. (The conversion moves a result expression to statement position, even the user did not "write it like that".)

[lrytz]

Would be good if we could limit it to one warning. -Wvalue-discard is in typer, -Wnonunit-statement in refchecks. Every discarded value will lead to nonunit statement, or?

[som-snytt]

Well, that may be true now. IIRC they used to have slightly different triggers.

scala -Wvalue-discard -Wnonunit-statement
Welcome to Scala 2.13.12 (OpenJDK 64-Bit Server VM, Java 21.0.2).
Type in expressions for evaluation. Or try :help.

scala> def f(): Unit = 42
                       ^
       warning: discarded non-Unit value of type Int(42)
                       ^
       warning: unused value of type Int(42) (add `: Unit` to discard silently)
def f(): Unit

scala> val b = collection.mutable.ListBuffer.empty[Int]
val b: scala.collection.mutable.ListBuffer[Int] = ListBuffer()

scala> def f(): Unit = b += 42
def f(): Unit

scala>

[SethTisue]

I don't mean to be difficult, but I don't feel comfortable merging this unless we can, at the same time, provide users extremely clear guidance about how to proceed, including in cross-built projects. Changing the behavior without providing that seems like churn to me.

[som-snytt]

@SethTisue they will have to supply version-specific -Wconf as shown above. But that is already true now.

This PR doesn't change behavior, only the "advice".

[som-snytt]

Relatedly, proposal to try -Wnonunit-statement then -Wvalue-discard then "pure expr in statement position" on dotty at scala/scala3#20408.

That behavior is adopted in refchecks, per Lukas's review comment above.

The previous message for the case of "discarded pure expression" is amended. It used to be a separate warning, but after the move from typer to refchecks, appears in the wrong order (because top-down traversal).

[som-snytt]

TIL I also need to partest --srcpath async --update-check.

[lrytz]

LGTM otherwise, thank you!

[lrytz]

there's getAndRemoveAttachment[DiscardedValue.type].exists

[som-snytt]

I think I was avoiding Option. This corner of the API is bothersome. Edit: but I would welcome guidance or reassurance.

[som-snytt]

Actually, retronym had already tired of the API, so I will use his improvement. getAndRemove isn't very efficient, aside from allocating. It would be nice to have remove(element): Boolean with just an indicator that it was removed. I did not touch the calls to update, which remove before adding (presumably to ensure only one attachment of a given type).

[lrytz]

also getAndRemoveAttachment

[som-snytt]

Squashed, rebased, used retronym API for getAndRemove.

Also realized I don't have a workflow for removing ! syntax from my title, which winds up in the merge commit.

[lrytz]

👍 maybe we can improve the attachments API so that the convenient thing doesn't come with caveats