-Wunused false negative in for-comprehension

[myazinn]

Compiler version

3.3.0, 3.3.1-RC4

Minimized code

With -Wunused:all

@main def run(): Unit =
  val dummy = 1
  for x <- Option(1)
  yield dummy

or

@main def run(): Unit =
  for
    x <- Option(1)
    y <- Option(2)
  yield y

Output

The code compiles without warnings

Expectation

The code produces an unused local definition warning for an x variable.

[mbovel]

This issue was picked for the Scala Issue Spree of tomorrow, July 2nd. @dwijnand and @aherlihy will be working on it! If you have any insight into the issue or guidance on how to fix it, please leave it here.

[som-snytt]

Motivated by the linked duplicate, I started work on the scala 2 ticket scala/bug#10287

IIRC, it's just the tupling operation that has to be "special-cased" as not "consuming". For the rewrite in parser, I gave duplicated Ident an attachment to reference the user-written Ident. (I don't know offhand if that is helpful for scala 3.)

I haven't submitted that scala 2 PR yet, but I have started a PR for scala 3 on unused imports.

[mbovel]

Cool!

Is your PR on unused imports in Scala 3 related to this issue? (—should I assign Anna and Dale to another issue?)

[som-snytt]

Hopefully it won't conflict much? #20894

That's just a heads-up.

I think I thought that if I fixed it on scala 2 I would circle around here, but the solutions may be different anyway; also, there is the impending improvement to for-comprehensions, which may or may not alter the approach.

[som-snytt]

The scala 2 fix has a long tail. After also spending time with Scala 3 CheckUnused, but without looking how for comprehensions are expanded or the plan for revised translation, I'm not sure I learned anything useful.

The fix was to add an attachment to patvars before the rewrite in parser which indicates the "original tree". (There are no symbols yet.) When a pattern bind is duplicated, it carries the attachment with it, so that "usages" or referenced can be charged against the original symbol later.

The tricky bits were to copy the attachment when a new tree is conjured for a var, and also not to count tuplings as usages.

My previous comment reflects my erstwhile optimism.

[aherlihy]

From my digging it seems there would need to be a nontrivial set of changes to address this issue, particularly to special case handling the tupled values so the compiler can even recognize that they are "unused". We cannot directly reuse the Scala 2 fix, and any new changes would be also be at risk for needing to be rewritten as soon as the for-comprehension changes kick in (scala/improvement-proposals#79), so I will pause on this ticket for the moment and we will add a 'community help wanted' label.

[som-snytt]

As an update, the revised Scala 2 approach is just to identify name bindings by their positions, which are preserved by tree duplication. (Then details of desugaring or other tree manipulation don't matter. Edit: that can't be true, as the tupling of value defs is special, and fresh terms for _ <- G etc.) (I have not compared Scala 3 yet.)