Fix "a pure expression does nothing" in synthetic REPL code

[eed3si9n]

So this was originally reported to sbt as sbt/sbt#5733.

When you invoke repl with "-Wconf:any:error" currently you get

           $line3.$read.INSTANCE.$iw
                                 ^
<synthetic>:6: error: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses

(To diagnose errors in synthetic code, try adding `// show` to the end of your input.)

I'm guessing this goes back all the way to 2006? (01443e4#diff-6eef86d28757da64bbaee0b568952750R332) but it's now surfaced.

This fixes that by putting it into val _.

This fixes this behavior by exempting synthetic-looking things from the "pure expression" warnings.

[dwijnand]

Using #7563 you can just ascribe the expression, which avoids a needless field.

[eed3si9n]

val _ = gets the jobs done. I don't see forcing value discard to be that much better of a solution.

[som-snytt]

This is a bug in the "pure expr for warning" check. It doesn't look at the selected path.

scala> object X { println("X!") ; val x = 42 }
object X

scala> def n = { X.x ; 27 }
                   ^
       warning: a pure expression does nothing in statement position; multiline expressions might require enclosing parentheses
def n: Int

The difference in behavior is that ordinarily, warnings in the synthetic cruft are ignored. So maybe it's also undesirable for -Wconf to kick in.

If an "unused value in statement position" lint is desirable, it would be different from "pure expr" and "value discard". It would be potentially noisier. This comes up frequently because people expect def x to also warn as "unused".

There's that -Wmacros option which might be better suited to a -Wconf filter. That would be a third issue, an enhancement.

It looks like this is a local block, so val _ is relatively harmless.

[eed3si9n]

This is a bug in the "pure expr for warning" check. It doesn't look at the selected path.

For the "warning purposes" treating X.x as pure sounds reasonable to me.

The difference in behavior is that ordinarily, warnings in the synthetic cruft are ignored.

Yea. I think it would be better if we could tag these as synthetic somehow and skip them in the warning.

[dwijnand]

Are both additions necessary for $fullAccessPath?

[eed3si9n]

For this particular one only the hackier one sym.decodedName.startsWith("$") is needed. I'm trying to exempt all synthetic symbols though.

[dwijnand]

We should do something more fine-grained than that. It's $line I think? There's already

    val INTERPRETER_IMPORT_WRAPPER     = "$iw"
    val INTERPRETER_WRAPPER            = "$read"

perhaps we just need to add "$line" to that? There's also isLineReadVal, which it looks like needs some attention from being merged from the 2.12 repl to the 2.13 repl.

[eed3si9n]

REPL is not the only code that generates synthetic code with dollar sign tho. I'd rather fix "pure expression in any synthetic looking" code problem than this narrow REPL situation.

[dwijnand]

That would be good, but sym.decodedName.startsWith("$") is an overreach IMO.

[eed3si9n]

How so? "pure expression does nothing" is a linting warning at best, and by convention $ variable names are not used by normal human-written programs.

[dwijnand]

Is every tree symbol a variable name? Does every tree symbol with a $ make it not human-facing?

I've been looking at parts of the exhaustivity checker and there's a 1001 ways where the check can bail out and give no heads-up. So I'm keen that we don't reach for the biggest tool in the toolbox here.

To be fair the problem is the REPL should take care of communicating its synthetic code, rather than having to reverse-engineer it here. Have you seen wrapperCleanup? Could we do something like that to address the original issue?

[eed3si9n]

Is every tree symbol a variable name? Does every tree symbol with a $ make it not human-facing?

I'd claim that for the purpose of warning (this method is nested in isPureExprForWarningPurposes), sure it's ok to ignore $.

It would be ideal if the symbols created by the REPLs are automatically flagged as synthetic. I guess from my point of view, I sent in quick fixes for the bug that was reported. It's a lint, and likely not going to break anyone's code. I'd rather not spend further time on this, so feel free to send in another PR I guess.

[lrytz]

I took the liberty to push -f the : Unit solution, I think that's the best way forward here.

This is a bug in the "pure expr for warning" check. It doesn't look at the selected path.

I think that should stay the way it is. To initialize a module, refer to the module in a statement, not to one of its fields. A statement reference to a module field is almost certainly a bug and the warning is helpful.

[lrytz]

refer to the module in a statement, not to one of its fields

(makes me wonder why the generated repl code references a field in statement position...)

[som-snytt]

Is this where the field used to be the lazily loaded object?

Is this issue why I noticed to close scala/bug#10209 about foo.bar?

[lrytz]

Went back to the val _ = ... solution that was green also for corner cases like no-predef (and it doesn't generate a field as som pointed out earlier)