Improve "Line starts with an operator" messaging to be either more vague or more precise, and less misleading

[jrudolph]

In akka-http parsing we have a lot of backquoted identifiers and there are some warnings because of that. E.g. for this code:

  // this specific ordering PREVENTS that next rule is allowed to parse `*/xyz` as a valid media range
  def `media-range-def` = rule {
    "*/*" ~ push("*") ~ push("*") |
      '*' ~ push("*") ~ push("*") |
      `type` ~ '/' ~ ('*' ~ !tchar ~ push("*") | subtype)
  }

we get this warning:

[warn] akka-http/akka-http-core/src/main/scala/akka/http/impl/model/parser/AcceptHeader.scala:41:7: Line starts with an operator that in future
[warn] will be taken as an infix expression continued from the previous line.
[warn] To force the previous interpretation as a separate statement,
[warn] add an explicit `;`, add an empty line, or remove spaces after the operator.
[warn]       `type` ~ '/' ~ ('*' ~ !tchar ~ push("*") | subtype)
[warn]       ^

In this case, we can use type only backquoted because it's a keyword otherwise.

[warn] akka-http/akka-http-core/src/main/scala/akka/http/impl/model/parser/SimpleHeaders.scala:110:7: Line starts with an operator that in future
[warn] will be taken as an infix expression continued from the previous line.
[warn] To force the previous interpretation as a separate statement,
[warn] add an explicit `;`, add an empty line, or remove spaces after the operator.
[warn]       `Cookie` {
[warn]       ^

Here, we use the backquotes for consistency because other header classes contain dashes and need the backquotes.

I don't quite understand if the warning is correct or if this code will indeed stop to parse in the future.

In those above cases, the identifiers are in operand-position of an infix expression. Would it be ambiguous in a future version how that should be parsed or would the compiler still figure out that we have a chain of infix operations that have a fixed pattern of operand operator operand operator operand ...? If that's the case, the warning should be suppressed if a backquoted identifier is found in operand position.

[som-snytt]

The message just means a NL would not be inferred, and the limited messaging has been improved in dotty under -source 3.0-migration:

-- Migration Warning: t12071.scala:8:6 -----------------------------------------
8 |      + 1
  |      ^
  |Rest of line starts with an operator;
  |it is now treated as a continuation of the previous expression in parentheses,
  |not as a separate statement.
-- Migration Warning: t12071.scala:11:6 ----------------------------------------
11 |      add_! x
   |      ^
   |Rest of line starts with an operator;
   |it is now treated as a continuation of the previous expression in parentheses,
   |not as a separate statement.
2 warnings found

I don't see that it's documented that backquoted idents and op-trailing idents like add_! are candidate tokens, in addition to operators.

It's not very easy to be helpful, because scalac here shows errors before warnings:

object Test extends App {
  implicit class adds(n: Int) {
    def add_! (i: Int) = n + i
  }
  class C {
    def x = 42
      + 1

    def y = 42
      add_! x
  }

  println {
    new C().y
  }
}

results in

t12071.scala:11: error: not found: value add_!
      add_! x
      ^
t12071.scala:11: error: postfix operator x needs to be enabled
by making the implicit value scala.language.postfixOps visible.
This can be achieved by adding the import clause 'import scala.language.postfixOps'
or by setting the compiler option -language:postfixOps.
See the Scaladoc for value scala.language.postfixOps for a discussion
why the feature needs to be explicitly enabled.
      add_! x
            ^
2 errors

or the warning for the other expression is a deprecation which must be enabled, otherwise it's hidden as a summary and the "pure expression" warning is foregrounded:

t12071.scala:8: warning: Line starts with an operator that in future
will be taken as an infix expression continued from the previous line.
To force the previous interpretation as a separate statement,
add an explicit `;`, add an empty line, or remove spaces after the operator.
      + 1
      ^
t12071.scala:8: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses
      + 1
      ^
2 warnings

The message in question doesn't benefit from parser context; it doesn't say if the difference makes a difference.

A workaround is to use parens instead of braces to enclose the single-expression argument.

[som-snytt]

@SethTisue might be interested that add_! counts for infix.

There should be a club named backquotes where anything goes. "What goes in backquotes stays in backquotes." People who frequent backquotes would recognize each other in the street by using air quotes made with one finger on each hand and tilted to the side.

[jrudolph]

The message just means a NL would not be inferred, and the limited messaging has been improved in dotty under -source 3.0-migration:

Interesting. The difference in my example is that it's even more unclear if the warning makes sense:

  def `test-1`: Int = 23
  def `test-2`: Int = 42

  println {
    1 +
      `test-2` + `test-1`
  }

still gives the warning:

[warn] -- Migration Warning: /home/johannes/git/opensource/user-code-review/dotty-cross/src/main/scala/Main.scala:16:6 
[warn] 16 |      `test-2` + `test-1`
[warn]    |      ^
[warn]    |Rest of line starts with an operator;
[warn]    |it is now treated as a continuation of the previous expression in parentheses,
[warn]    |not as a separate statement.
[warn] one warning found

I don't think the semantics of that particular expression ever changed or is planned to change in the future.

(Not sure about whether the scanner actually has enough context or not to make that distinction. Somehow it knows that it is a continuation about a previous line, so maybe it could also now that the last token before was an operator itself.)

There should be a club named backquotes where anything goes. "What goes in backquotes stays in backquotes." People who frequent backquotes would recognize each other in the street by using air quotes made with one finger on each hand and tilted to the side.

Oh yes, and when they meet the quasiquotes in the streets a situation ensues where the backquotes will try to apply whatever they are training that cannot be talked about. The quasiquotes will then try to weasel themselves out by confusing the others starting a meta discussion about whether the situation is happening at runtime right now or whether it already happened at a former stage...

[SethTisue]

just as I argued in the case of scala/scala#9026, I think the warning is wrong, the check should be Chars.isOperatorPart on the first character only

[eed3si9n]

I think the warning is wrong, the check should be Chars.isOperatorPart on the first character only

I'm not sure if using different criteria from Dotty would improve the situation. I've tried to summarize what's going on as follows.

Given the following source

object C {
  def x = 42
    + 1

  def y = 1 +
    `test-1` + `test-2`

  def z = 2
    `compareTo` (2 - 1)

  def `test-1`: Int = 23
  def `test-2`: Int = 42
  def compareTo(x: Int) = println("lol")
}

Scala 2.13.3 with -deprecation

sbt:root> console
[info] Compiling 2 Scala sources to /private/tmp/hello/target/scala-2.13/classes ...
[warn] /private/tmp/hello/C.scala:3:5: Line starts with an operator that in future
[warn] will be taken as an infix expression continued from the previous line.
[warn] To force the previous interpretation as a separate statement,
[warn] add an explicit `;`, add an empty line, or remove spaces after the operator.
[warn]     + 1
[warn]     ^
[warn] /private/tmp/hello/C.scala:6:5: Line starts with an operator that in future
[warn] will be taken as an infix expression continued from the previous line.
[warn] To force the previous interpretation as a separate statement,
[warn] add an explicit `;`, add an empty line, or remove spaces after the operator.
[warn]     `test-1` + `test-2`
[warn]     ^
[warn] /private/tmp/hello/C.scala:9:5: Line starts with an operator that in future
[warn] will be taken as an infix expression continued from the previous line.
[warn] To force the previous interpretation as a separate statement,
[warn] add an explicit `;`, add an empty line, or remove spaces after the operator.
[warn]     `compareTo` (2 - 1)
[warn]     ^
[warn] three warnings found
[info] Starting scala interpreter...
Welcome to Scala 2.13.3 (OpenJDK 64-Bit Server VM, Java 1.8.0_232).
Type in expressions for evaluation. Or try :help.

scala> (C.x, C.y, C.z)
lol
val res0: (Int, Int, Int) = (42,66,2)

Dotty 0.26.0-RC1 with -deprecation -source 3.0-migration

sbt:dotty-simple> console
[info] Compiling 1 Scala source to /private/tmp/hello-dotty/target/scala-0.26/classes ...
[warn] -- Migration Warning: /private/tmp/hello-dotty/src/main/scala/Main.scala:3:4 ---
[warn] 3 |    + 1
[warn]   |    ^
[warn]   |Rest of line starts with an operator;
[warn]   |it is now treated as a continuation of the previous expression in parentheses,
[warn]   |not as a separate statement.
[warn] -- Migration Warning: /private/tmp/hello-dotty/src/main/scala/Main.scala:6:4 ---
[warn] 6 |    `test-1` + `test-2`
[warn]   |    ^
[warn]   |Rest of line starts with an operator;
[warn]   |it is now treated as a continuation of the previous expression in parentheses,
[warn]   |not as a separate statement.
[warn] -- Migration Warning: /private/tmp/hello-dotty/src/main/scala/Main.scala:9:4 ---
[warn] 9 |    `compareTo` (2 - 1)
[warn]   |    ^
[warn]   |Rest of line starts with an operator;
[warn]   |it is now treated as a continuation of the previous expression in parentheses,
[warn]   |not as a separate statement.
[warn] three warnings found

scala> (C.x, C.y, C.z)
val res0: (Int, Int, Int) = (43,66,1)

observation

1. The infix newline can cause two sources to compile and evaluate to different values (42,66,2) vs (43,66,1) either using symbolic or backtick.
2. The false positive has to do with previous line ending in + and therefore it's already part of the previous line.

expectation

Keep the first and last warning that changed from 42 -> 43, 2 -> 1, but eliminate the second one.

[SethTisue]

I'm not sure if using different criteria from Dotty would improve the situation

do you think the Dotty criteria are both intentional and correct? I wonder if they're even intentional (and they don't seem correct to me)

[eed3si9n]

do you think the Dotty criteria are both intentional and correct?

Yes. The Dotty criteria outlined in Martin's Scaladoc comment in scala/scala3#7031 I think was intentional:

A leading symbolic or backquoted identifier is treated as an infix operator if it is followed by at least one ' ' and a token on the same line that can start an expression.

In a sense scala/scala3#7031 is creating a new language behavior so the fact that backtick now is treated specially can be said correct in the Scala 3.x world.

What's buggy IMO is the fact that "Line starts with an operator" (2.13.3) or "Rest of line starts with an operator" (0.26.0-RC1) are displayed in the situation where the line is already part of the previous line.

This should be warned:

  def z = 2
    `compareTo` (2 - 1)

But the following should NOT be warned:

  def y = 1 +
    `test-1` + `test-2`

Regardless of infix criteria backtick, Scala 2.x already considers the line to be part of the previous line because the previous line ended in +, and the warning is false-positive, and end up sounding nonsensical. Same applies to Johannes's original example.

[jrudolph]

Thanks a lot for the summary, @eed3si9n.

In a sense lampepfl/dotty#7031 is creating a new language behavior so the fact that backtick now is treated specially can be said correct in the Scala 3.x world.

Yes, in any case that decision has been made for dotty by now. I don't think it's particularly likely that that behavior change would introduce bugs in many cases because an identifier usually isn't defined to be used as a standalone term to start an expression and as an operator, so that only one of the interpretations would compile correctly after all.

Regardless of infix criteria backtick, Scala 2.x already considers the line to be part of the previous line because the previous line ended in +, and the warning is false-positive, and end up sounding nonsensical. Same applies to Johannes's original example.

👍, that was my main gripe that I'd like to see fixed.

[jxnu-liguobin]

@SethTisue So, the result of this discussion is to eliminate the second infix expression warning?

object C {
  def x = 42
    + 1

  def y = 1 +
    `test-1` + `test-2`

  def z = 2
    `compareTo` (2 - 1)

  def `test-1`: Int = 23
  def `test-2`: Int = 42
  def compareTo(x: Int) = println("lol")
}