PrettyPrinter bug fix on Term.Block

[RCMartins]

Fixes #2046

I tried to create a complete solution that works in all cases, not just the ones reported in the issue.
I added tests for all Stat combinations that I could find in https://scalameta.org/docs/trees/quasiquotes.html. Let me know if there is any relevant one that I missed.

---

Code style

In guessNeedsLineSep function I "expanded" the pattern matching of the stats (e.g. Defn.Val(_, _, _, rhs) =>), but maybe you would prefer a more concise solution.
Maybe something like this instead?

...
case t: Decl.Type => false
case t: Defn.Val => guessNeedsLineSep(t.rhs)
case t: Defn.Var => guessNeedsLineSep(t.body)
case t: Defn.Def => guessNeedsLineSep(t.body)
...

---

Note:
I think it would be better to not merge until the bugs I found while working on this (#3136 and #3138) are resolved.

[kitbellew]

thank you for this contribution! below are some suggestions and comments.

[RCMartins]

Besides using the new test function runTestAssert and waiting on issue #3138, I think I did all your sugestions. Let me know if there are more improvements to add in the meanwhile

[kitbellew]

about the ApplyInfix change checking for Block: I think it was this one. at least, one of the tests failed for me. not sure how my testBlock is different from yours (they look similar) :)

[RCMartins]

Now after the rebase it's failing for me too, it's probably a regression from #3141, do you want me to create a new issue for this? In v4.7.7 it was parsing this correctly.

The case is something like this:

  test("#3138: infix with block with another following block") {
    val codeOnNextLine =
      """|{
         |  Foo bar { a }
         |  { b }
         |}
         |""".stripMargin
    val syntaxOnNextLine =
      """|{
         |  Foo bar {
         |    a
         |  }
         |  {
         |    b
         |  }
         |}
         |""".stripMargin
    val treeOnNextLine = Term.Block(
      Term.ApplyInfix(
        Term.Name("Foo"),
        Term.Name("bar"),
        Nil,
        List(Term.Block(List(Term.Name("a"))))
      ) :: Term.Block(List(Term.Name("b"))) :: Nil
    )
    runTestAssert[Stat](codeOnNextLine, Some(syntaxOnNextLine))(treeOnNextLine)
  }

Right now in the main branch is returning:

{
  Foo bar ({
    a
  }) {
    b
  }
}

[kitbellew]

Now after the rebase it's failing for me too, it's probably a regression from #3141, do you want me to create a new issue for this? In v4.7.7 it was parsing this correctly.

Why is it a regression? Why can you do

foo bar (1)
{ 2 }

but not

foo bar { 1 }
{ 2 }

or

foo bar { (1, 2) }
{ 3 }

in fact, looks like both

foo bar (1)
{ 2 }
foo bar { 1 }
{ 2 }

should be allowed in scala2 but not scala3.

see https://dotty.epfl.ch/docs/internals/syntax.html
and https://www.scala-lang.org/files/archive/spec/2.11/13-syntax-summary.html

[RCMartins]

I think I know the anwser:
If you go back to the initial issue I opened: #3138, you can see that:

foo bar (1)
{ 2 }

is not the same as foo bar (1, 2) but rather foo bar ((1){2}) (just one param: 1.apply(2))

---

This expression:

foo bar { 1 }
{ 2 }

is actually a block with two terms, foo bar { 1 } and { 2 }, no ambiguity.
And you can't place them in the same line (without a ;)

---

If you try something like { (1, 2) } { 3 } is actually invalid syntax (you would have to place a ; between), but

{ (1, 2) }
{ 3 }

Are just two blocks in a row (probably inside another block)

---

I think this is just an issue with the scalameta parser, what is your opinion?

[kitbellew]

@RCMartins thank you for that but you are not explaining WHY you think that way.

I ran a little experiment here:
https://scastie.scala-lang.org/HkPoBGmNSwGxSz81IeNBOw

def foo1(a: Int)(b: Int) = a + b

def foo2(a: Int) = a + 1

// no errors
0 + foo1 { 1 }
{ 2 }

// error: missing argument list for method foo1
0 + foo1 { 1 }

{ 2 }

// error: Int doesn't take parameters
0 + foo2 { 1 }
{ 2 }

which means that { 2 } on a separate line, after { 1 }, was not actually treated as a separate expression.

[RCMartins]

Part 0 - my useless previous "example"

If you try something like { (1, 2) } { 3 } is actually invalid syntax (you would have to place a ; between), but

{ (1, 2) }
{ 3 }

Are just two blocks in a row (probably inside another block)

I can see now that this previous example didn't really matter and just added more confusion, so just completely ignore it please :)

Part 1 - It's just Term.Apply

In my original issue the incorrect expression is a Term.ApplyInfix, and I think the parsing problem is only with that particular Term.

I think your experiment tested a different tree that has no issue now or before:

def foo1(a: Int)(b: Int) = a + b

In your example:

0 + foo1 { 1 }
{ 2 }

Is the same as

0 + ( ( foo1 { 1 } ) { 2 } )

So, the "potential problemetic code" is just two Term.Apply:
Term.Apply(Term.Apply(Term.Name("foo1"), List(Term.Block(List(Lit.Int(1))))), List(Term.Block(List(Lit.Int(2)))))

And with that I have no issue with.

Part 2 - My example and Infix parsing logic

My example:

foo bar { 1 }
{ 2 }

Seems similar in quick glance to to your example but they are not:

0 + foo1 { 1 }
{ 2 }

With an infix op the right-hand side will only accept one of two things:

1. One Term, that can have braces around (in that case the term is a Term.Block, and any code after } should not be part of the ApplyInfix term)
2. Multiple Terms, with a () around, and , between

In my example, bar is the infix op, it will only accept one of two things, in my case its case 1. { 1 } is the term for the right-hand side of the operation. After the right-hand side there is a new line a new Stat begins: { 2 }.

In your example, + is the infix op, it will only accept one of two things, in your case its case 1. foo1 { 1 }\n{ 2 } is the term for right-hand side of the operation. After the right-hand side there's no more code.
WHY it parses that way? it first parses Term.Name("foo1") then it checks that it has an { and creates an Term.Apply and then it checks that it has another { and creates another Term.Apply.

So there is no issue in your example above, because it's just a diferent tree.

Part 3 - Generating a new example that causes the issue

I can create generate the issue again from your example if I add some braces (and an aditional block { 3 } to trigger the issue):

0 + {
  foo1 { 1 }
  { 2 }
}
{ 3 }

This expression is completely fine in scala, and parses correctly in scalameta 4.7.7, but parses the wrong tree with the latest scalameta version. You can try this new expression in: https://scastie.scala-lang.org/yY7TuHrEQC6K23I5Hj2Btw

As you can see, it's just two Stats, the + operator picks up the term on the right side which is: { foo1 { 1 } { 2 } } and finishes parsing that term, it should NOT continue into the next line ({ 3 }).

Conclusion

For completeness sake I'll add this two examples in my tests:

testBlockAddNL("foo {bar}", "foo {\n  bar\n}")
testBlockAddNL("foo\n{bar}\n{baz}", "foo {\n  bar\n} {\n  baz\n}")

I hope this helps clarifing the issue, it's not trivial to explain, but I tried my best!
if you need any help generating more problematic examples, let me know, I have less time during the week, but I'll try :)

[kitbellew]

does #3148 address this?

[RCMartins]

@kitbellew yes, I tried merging with the branch locally and it fixes the issue! Thank you :)

[kitbellew]

@RCMartins #3143 is merged.

[RCMartins]

@kitbellew Thanks, I'll use the new test class, and update the PR.
In the meanwhile I think I found another bug :) #3142

[RCMartins]

I think the only thing remaining is the regression from #3141