Implement -Wvalue-discard warning #15975

cb372 · 2022-09-05T19:02:36Z

Warn when an expression e with non-Unit type is adapted by embedding it into a block { e; () } because the expected type is Unit.

In Scala 2.13 (relevant code in Typers.scala) there are two special cases:

The warning can be disabled by explicitly annotating an expression with : Unit.
- In this PR I've elected not to implement this, as IMO it's not necessary now we have @nowarn. It's just as easy to annotate with @nowarn as with : Unit.
No warning is raised if the expression is an application of a function that returns this.type, because this is a very common operation when using mutable collections.
- The heuristic is actually slightly more subtle than that. See the warn-value-discard.scala test for some examples.
- I have implemented this special case by copying the relevant code from the 2.13.x branch in scala/scala, so the behaviour should be equivalent to Scala 2.13.

Warn when an expression `e` with non-Unit type is adapted by embedding it into a block `{ e; () }` because the expected type is Unit.

som-snytt · 2022-09-05T21:09:42Z

It's documented somewhere, but Scala 2 avoids warning for methods that return this.type.

def append(x: X): Unit = buf.addOne(x)

The user can "annotate" that discarding is desired:

def f(): Unit = x.m(): Unit

That convention was devised before @nowarn and -Wconf, so I don't know what would be best now; the "explicit annotation" exception means a warning is never generated, instead of generating a warning that must be post-processed.

I'd suggest not supporting ancient compiler option aliases (the original -Ywarn).

If they want to support -Xlint (or -Wlint) as an option, it's worth considering calling it -Xlint:value-discard, where the distinction between -W and -Xlint is that -W warns about dubious language features or interactions (-Wnumeric-widen), whereas -Xlint warns about code that may be reasonable. (Maybe -Wvalue-discard is the better name under that criterion, who knows?)

odersky · 2022-09-06T09:18:52Z

This is a fine line to walk since lots of parts of the Scala library do return something that can safely be ignored. For instance
put or remove on mutable collections, or the many functions that return this.

To make the test bullet-proof it would need to be augmented with effect checking. It makes indeed no sense to discard the value of a pure expression, but for side-effecting expressions it's common practice.

SethTisue · 2022-09-06T18:07:12Z

This is a fine line to walk since lots of parts of the Scala library do return something that can safely be ignored

Note that in Scala 2 these warnings aren't enabled by default, and they aren't enabled by -Xlint either. Users must explicitly opt-in, and it's expected that pure-functional coders are the main audience. Those coders are already avoiding "parts of the Scala library" such as mutable collections. (And in places where they must use those parts, they don't mind a little extra ceremony.)

the many functions that return this

I agree with Som that these are important to exempt.

som-snytt · 2022-09-07T00:31:35Z

Relatedly, -Wnonunit-statement resulted in narrowing the mutators of StringBuilder to this.type in scala/scala#10047 which suggests that adding an annoying tool to the tool belt may result in useful behavior changes to avoid the annoyance.

My recent interaction with ConcurrentMap and the Java collection wrappers was a reminder that the half-life of a collections API may be half as long as previously estimated.

gvolpe · 2022-11-18T08:54:18Z

Apologies for rekindling this discussion, but I am currently finishing writing a Scala 3 book focused on purely functional programming, and I was wondering if there were any plans for Scala 3 to add support for -Ywarn-value-discard (the Scala 2 equivalent). This flag is enabled by default in sbt-tpolecat, a plugin used by most FP applications.

I've been following the work on #16157, but it seems this work is out of scope. Then Chris made me aware of this draft PR. I've also been pointed to the zerowaste plugin, ~~but unfortunately it doesn't seem to work~~ EDIT: After reaching out to the author, he made some fixes and it now works! More testing on other projects still required, though, if anyone would like to help test it.

I've been told this feature is planned to be implemented after 3.3, but I couldn't find any discussion about it. Is there any link related to this I can refer to in my book?

This flag is a must-have for production systems, and I believe it currently is the biggest blocker to Scala 3 adoption in FP shops.

I copied all the relevant code over from `scala/scala:2.13.x`, so the behaviour should be equivalent to Scala 2.13.

cb372 · 2022-11-23T14:21:42Z

I've ported over from Scala 2 the special-case handling of functions that return this.type, so I think this is now ready for a review.

KacperFKorban

I tested the PR and it seems to work as expected (the same as in Scala 2). Added some suggestions/questions about the code.

KacperFKorban · 2022-12-14T14:06:22Z

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+   *      * targs = Nil
+   *      * argss = List(List(arg11, arg12...), List(arg21, arg22, ...))
+   */
+  final class Applied(val tree: Tree) {


Can we possibly reuse some of the existing utility methods e.g. allArguments, methPart, stripApply, etc.?
Or else make those methods top-level? (Some of them seem unnecessary for this case)

Thanks for the pointer. When I was copying swathes of code from Scala 2, I didn't noticed those almost-identical methods already existed. I've refactored Applied to make use of them.

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

TreeInfo already provided helpers that were almost identical to some of the methods in `Applied` that I copied over from Scala 2. Refactored `Applied` to make use of the existing methods. Moved `termArgss` from `TypedTreeInfo` up to `TreeInfo` so I could make use of it in `Applied`. Also moved `typeArgss`, just for consistency.

smarter · 2022-12-21T16:01:11Z

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

+  /** Applications in Scala can have one of the following shapes:
+   *
+   *    1) naked core: Ident(_) or Select(_, _) or basically anything else
+   *    2) naked core with targs: TypeApply(core, targs) or AppliedTypeTree(core, targs)


In Scala 3 a TypeApply can also wrap an Apply because of how extension methods are desugared and soon because of #14019

(And an AppliedTypeTree can wrap another AppliedTypeTree, e.g. type Foo[X] = [Y] =>> Y; val x: Foo[Int][String] = "")

KacperFKorban

@cb372
Thanks for all your work on this feature ❤️

Since we want this feature to be in the next branched-out release, I will merge this PR now and address the remaining review comment in a separate PR immediately following this one (#16637)

cb372 · 2023-01-09T12:26:25Z

Thank you! I was planning to address that comment but it was gradually moving further down my todo list 😄 so I'm happy for you to take over from here.

cb372 added 2 commits September 5, 2022 19:42

Implement -Wvalue-discard warning

9dc1404

Warn when an expression `e` with non-Unit type is adapted by embedding it into a block `{ e; () }` because the expected type is Unit.

Add a .check file

fae048c

cb372 added 5 commits November 22, 2022 20:08

Merge remote-tracking branch 'upstream/main' into warn-value-discard

ef34c69

Remove -Ywarn-value-discard alias

5e05468

Add special-case logic for function calls returning this.type

cb99f7c

I copied all the relevant code over from `scala/scala:2.13.x`, so the behaviour should be equivalent to Scala 2.13.

Add tests for assignment operator

702c485

Remove null checks

83a01b8

cb372 marked this pull request as ready for review November 23, 2022 14:18

Merge remote-tracking branch 'upstream/main' into warn-value-discard

592dccf

dwijnand assigned KacperFKorban Nov 25, 2022

Kordyjan added this to the 3.3.0-RC1 milestone Dec 12, 2022

KacperFKorban reviewed Dec 14, 2022

View reviewed changes

KacperFKorban assigned cb372 and unassigned KacperFKorban Dec 14, 2022

cb372 added 2 commits December 21, 2022 11:33

Merge remote-tracking branch 'upstream/main' into warn-value-discard

8185517

smarter reviewed Dec 21, 2022

View reviewed changes

KacperFKorban approved these changes Jan 9, 2023

View reviewed changes

KacperFKorban merged commit 667d904 into scala:main Jan 9, 2023

cb372 deleted the warn-value-discard branch January 9, 2023 15:20

carlosedp mentioned this pull request Jun 9, 2023

Support -Wunused:all and -Wvalue-discard on Scala > 3.3 DavidGregory084/mill-tpolecat#24

Open

Kordyjan modified the milestones: 3.3.0-RC1, 3.3.0 Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement -Wvalue-discard warning #15975

Implement -Wvalue-discard warning #15975

cb372 commented Sep 5, 2022 •

edited

Loading

som-snytt commented Sep 5, 2022 •

edited

Loading

odersky commented Sep 6, 2022

SethTisue commented Sep 6, 2022

som-snytt commented Sep 7, 2022

gvolpe commented Nov 18, 2022 •

edited

Loading

cb372 commented Nov 23, 2022

KacperFKorban left a comment

KacperFKorban Dec 14, 2022

cb372 Dec 21, 2022

smarter Dec 21, 2022

smarter Dec 21, 2022

KacperFKorban left a comment •

edited

Loading

cb372 commented Jan 9, 2023

Implement -Wvalue-discard warning #15975

Implement -Wvalue-discard warning #15975

Conversation

cb372 commented Sep 5, 2022 • edited Loading

som-snytt commented Sep 5, 2022 • edited Loading

odersky commented Sep 6, 2022

SethTisue commented Sep 6, 2022

som-snytt commented Sep 7, 2022

gvolpe commented Nov 18, 2022 • edited Loading

cb372 commented Nov 23, 2022

KacperFKorban left a comment

Choose a reason for hiding this comment

KacperFKorban Dec 14, 2022

Choose a reason for hiding this comment

cb372 Dec 21, 2022

Choose a reason for hiding this comment

smarter Dec 21, 2022

Choose a reason for hiding this comment

smarter Dec 21, 2022

Choose a reason for hiding this comment

KacperFKorban left a comment • edited Loading

Choose a reason for hiding this comment

cb372 commented Jan 9, 2023

cb372 commented Sep 5, 2022 •

edited

Loading

som-snytt commented Sep 5, 2022 •

edited

Loading

gvolpe commented Nov 18, 2022 •

edited

Loading

KacperFKorban left a comment •

edited

Loading