scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.84k stars 1.05k forks source link

Scala 3 regression of new-line braced block argument #16760

Open soronpo opened 1 year ago

soronpo commented 1 year ago

The following code works in Scala 2.13. In Scala 3 the braces {} (when appearing in a new line) are considered to be separate from the method call.

Compiler version

v3.2.2-RC2

Minimized code

https://scastie.scala-lang.org/UUJ5id94STydaoPDCkAQhQ

@main def main: Unit =
  def process(arg: Int)(block: => Unit): Int = arg
  val x = process(1) 
  {
  }
  println(x)

Output

warning:   A pure expression does nothing in statement position; you may be omitting necessary parentheses
main$package$$$Lambda$26822/0x00000008049bcc10@4f7a0b91

Expectation

Should print 1.

soronpo commented 1 year ago

I'm actually very surprised we didn't stumble upon this beforehand. I checked and it's the same all the way back to 3.0.0.

odersky commented 1 year ago

This is actually fully intentional because it solves a nasty problem with Scala 2:

println(x)
{ val y = ...
  println(y)
}

does not do what you think it should do in Scala 2. In Scala 3, a block or parenthesized expression on the next line is only treated as a continuation of the previous line if it is indented.

soronpo commented 1 year ago

But what about this code (the actual original code the led me to post this issue). Doesn't work, and very common to see this style of braces in the wild:

process(arg)
{
//code
}

Works:

process(arg) {
//code
}

I claim that since both our arguments are valid, the fact that mine preserves the Scala 2 behavior, and is more likely to appear, we should switch the behavior back to prevent such regressions.

soronpo commented 1 year ago

Additionally, your long term goal for the language is to be braceless anyways, so there is no reason to break existing braced syntax.

odersky commented 1 year ago

Additionally, your long term goal for the language is to be braceless anyways, so there is no reason to break existing braced syntax.

That's a good point.

I am not sure how common the pattern with opening brace on the left is. The fact that nobody noticed the change so so far indicates that it might not be very common. Also, if we can keep ruling out this pattern then we avoid the common gotcha where a block after an application is not recognized.

I have opened in #16767 a draft PR to revert to the Scala 2 rules. Some tests broke as well one instance in the compiler, where a brace opening a follow-on block was used without a preceding newline. So adopting #16767 will technically be a regression on previous Scala 3 rules.

So, not sure what to do here. @tgodzik What is your take on this?

soronpo commented 1 year ago

I have opened in #16767 a draft PR to revert to the Scala 2 rules. Some tests broke as well one instance in the compiler, where a brace opening a follow-on block was used without a preceding newline. So adopting #16767 will technically be a regression on previous Scala 3 rules.

Maybe we should raise this point for a short vote for the next SIP meeting. Kind of choosing what we should break. I don't know if this particular issue was raised during the SIP talks confirming the Scala 3 syntax.

tgodzik commented 1 year ago
@main def main: Unit =
  def process(arg: Int)(block: => Unit): Int = arg
  val x = process(1) 
  {
  }
  println(x)

Scalameta currently parses the same way as the compiler, so only indented { will be counted as belonging to the previous expression. I think this is a much better behaviour IMHO, since it might unexpected that the next block is being taking into previous expression. On the plus side, if someone uses scalafmt they will most likely see what is going since this will be autoformatted differently whichever solution we decide on.

We can change that in Scalameta, but should we keep it as is for the version prior to the one this will be merged to?

Also, if no one raised that until then, is it a problem to keep the current behaviour? What about parenthesis? For example:

process(arg)
(code)

would that be also treated as the same expression and if not isn't it a bit inconsistent?

odersky commented 1 year ago

Parentheses will not be seen as a continuation. So yes, the Scala 3 behavior is more consistent. And if it did not cause problems yet, we might as well stick with it.

soronpo commented 1 year ago

Parentheses will not be seen as a continuation. So yes, the Scala 3 behavior is more consistent. And if it did not cause problems yet, we might as well stick with it.

Do we have some industry metric of how big a percent of existing projects migrated to Scala 3?

SethTisue commented 1 year ago

Do we have some industry metric of how big a percent of existing projects migrated to Scala 3?

For application code, the percentage is probably not that high. But most of what's in the Scala 3 community build (both the small one and the bigger open one) is Scala 2 code that cross-compiles on 2 and 3, and the open community build especially is quite large, so I think we can rest easy that we have good coverage in the area of handling old code well.

soronpo commented 1 year ago

For application code, the percentage is probably not that high. But most of what's in the Scala 3 community build (both the small one and the bigger open one) is Scala 2 code that cross-compiles on 2 and 3, and the open community build especially is quite large, so I think we can rest easy that we have good coverage in the area of handling old code well.

The problem is that the difference is set by user preference of how braces are used, which is NOT reflected by a community build. I used Scala for a lot of time and I never opened a brace block outside of a class/def. Is there even a bug opened about the Scala 2 syntax?

SethTisue commented 1 year ago

The problem is that the difference is set by user preference of how braces are used, which is NOT reflected by a community build.

Sorry, I'm not following what your argument is, here...? Can you spell it out more?

I used Scala for a lot of time and I never opened a brace block outside of a class/def

Me either, or only rarely. ...and that's part of why I'm not too concerned about the impact of the minor incompatibility here.

As a matter of style, I would personally use locally, but I'm not claiming I'm typical; probably lots of people don't know about locally. But lots of people probably don't know that it's legal to open a scope without locally, either.

soronpo commented 1 year ago

Sorry, I'm not following what your argument is, here...? Can you spell it out more?

Community build is driven by library authors. Not application audience which is more diverse. Some people prefer to open braces like:

process(arg)
{
//code
}

You are likely not to see that in a community build. In fact, the only reason that I noticed it is because students of mine preferred that style, and it took me a while to figure out the problem derived from the block to be considered separate from the method call, because it was completely unexpected and counter-intuitive for me, even though I don't prefer this style at all. So from my perspective, we solved a non-bug in Scala 2 that was reported by no-one and introduced an actual bug that could break existing code for users that just prefer their braces in a different way.

soronpo commented 1 year ago

So yes, the Scala 3 behavior is more consistent.

IMO, it's not consistent with the rest of the language though.

This is acceptable.

class Foo
{
}

This is also acceptable

def foo: Unit =
{
}

From my perspective the consistency is whether the braces block is a continuation of the previous statement.

Atry commented 1 year ago

Isn't locally designed to disambiguate it?

soronpo commented 1 year ago

Isn't locally designed to disambiguate it?

With the current parser implementation:

@main def main: Unit = 
  locally  //error:  missing arguments for value of type Any => Any
  {
  }
myazinn commented 1 year ago

This is acceptable.

class Foo
{
}

Looks very confusing tbh. Maybe it should've been unacceptable as well

soronpo commented 1 year ago

This is acceptable.

class Foo
{
}

Looks very confusing tbh. Maybe it should've been unacceptable as well

I disagree. It's a very common syntax pattern in many other languages. The ability to just open a local block in the middle of nowhere in the code (without locally) is the anti-pattern, IMO.

myazinn commented 1 year ago

I disagree. It's a very common syntax pattern in many other languages.

Which doesn't mean it's not confusing in Scala. In Scala, it is allowed to use code blocks with curly braces wherever you want (though I agree that it's an anti-pattern), which I believe is not a common thing in languages that use suggested syntax pattern. Scala style guide explicitly says

Opening curly braces ({) must be on the same line as the declaration they represent:

And then it also says that while GNU-style notation is allowed, it's behaviour unspecified and therefore unreliable.

odersky commented 1 year ago

I think the discussion shows that going back is also controversial. So I am going to close #16767 without merging. Right now I believe that if we want to change the 3.3 behavior we need a SIP.

soronpo commented 1 year ago

I doubt the original change ever passed through the SIP committee, and so to require one right now to fix this regression seems kind of a redundant blocker.

odersky commented 1 year ago

Normally I would agree with you, but it seems the majority opinion is that this was a good change and should be kept. And the change was made two years ago, so from the standpoint of Scala 3, that's the status quo. So it needs to be discussed again in breath and depth if we would want to revert it.