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")
}

[SethTisue]

I haven't revisited this lately, anybody else able to respond to that? (@som-snytt, perhaps?)

[eed3si9n]

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

That's what I claimed to be the "expectation" of this bug report - #12071 (comment)
and original reporter (Johannes) agreed, so yeah, if you can fix that behavior that will fix what's reported in this issue.

[jxnu-liguobin]

I don't know if I understand it correctly: at present, only if the character at the end of the previous line is an operator and the first character of the newline is a back quotation mark, we need eliminate the warning?
And I'm not familiar with Scanner at present. It seems that pre and next can't be used directly?

[som-snytt]

I'm about to PR some improvements in the feature. Dotty had a couple of tweaks since the original change.

The reason dotty also warns in the disputed case is that it's just a lexing thing.

-- Migration Warning: test/files/neg/t12071.scala:28:4 -------------------------
28 |    `test-1` + `test-2`
   |    ^
   |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.

Not sure yet if parser can suppress the warning.

One improvement is to add the future advice to the postfix error here:

  def f = c
    `c c` i

That is, if you forget that leading infix is not enabled by default and you forget that scala 2 has postfix, it should still remind you that enabling -Xsource:3 makes it work.

[som-snytt]

@jxnu-liguobin sample https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/parsing/Scanners.scala#L378

[jxnu-liguobin]

@som-snytt Thanks, but I don't know about dotty, and it seems that dotty code is quite different from scala2.
I just guess we need to implement similar code in scala2. However, due to structural differences, I am not sure whether there is a way to replace LookaheadScanner. What does it seem to be looking for?

 def assumeStartsExpr(lexeme: TokenData) =
          canStartExprTokens.contains(lexeme.token)
          && (!lexeme.isOperator || nme.raw.isUnary(lexeme.name))
        val lookahead = LookaheadScanner()
        lookahead.allowLeadingInfixOperators = false
          // force a NEWLINE a after current token if it is on its own line
        lookahead.nextToken()
        assumeStartsExpr(lookahead)
        || lookahead.token == NEWLINE && assumeStartsExpr(lookahead.next)

[som-snytt]

WIP scala/scala#9567 without rewording or addressing the false positive. I'll take a second look at "what is a leading infix". The latest rule excludes

`x'` += 1

because += doesn't look like an arg. But unary ops are OK. So IDK yet what does it do with

`type` ~ something

oh, glancing above, I see ~ is the op, of course.

[dwijnand]

Looking at scala/scala3#7031, while Eugene is right that the warning in Scala 3 (under -source 3.0-migration) and in Scala 2 is in line with the scaladoc, looking at the tests it seems like it's accidentally implemented and described as such. Seeing as you need to opt-in to the Scala 3 warning while you just get it in Scala 2, I think we can look to drop this warning even at risk of diverging from 3.0-migration for a bit.

[som-snytt]

Maybe this is an opportunity to leverage -Xmigration as a way to opt-in to annoying messages. I understand the previous comment to mean, just drop the warning entirely. (Edit: there is no message under -Xsource:3; this is a deprecation, which probably should just be a migration warning, which sounds obvious to me now.)

To recap, the limitation is that the scanner message should be suppressed as spurious by parser, but it's a bit much to add the mechanism just for this niche case. I did add a check so that "postfix error" does not suppress informative warning; that was ad-hoc but cheap, since infix good, postfix bad.

[dwijnand]

Yeah, if fixing this is tricky, putting it behind -Xmigration sounds like a good idea.

[som-snytt]

Currently, -Xsource:3 issues a migration warning; -Xmigration:2.13.2 also warns but it's likely no one does that, and anyone migrating from 2.12 doesn't want to hear about this.

The irony is that it is usually an error; the warning is for when it surreptitiously compiles without error.

Worth adding that "Improve leading infix message to be less misleading" was an excellent pun. I just tried it over on a dotty discussion where it was not well received.