lampepfl / dotty-feature-requests

Historical feature requests. Please create new feature requests at https://github.com/lampepfl/dotty/discussions/new?category=feature-requests
31 stars 2 forks source link

Optional brace syntax in Scala3 is subtly misleading for single case partial functions #235

Closed mdedetrich closed 1 year ago

mdedetrich commented 3 years ago

Context of this issue can be found at https://github.com/scalameta/scalafmt/issues/2687#issuecomment-901882754 but in summary with Scala3's new python code style where {,} can be omitted there are some scenarios where the difference between having braces and not having braces is visually subtle but has completely different meaning.

One such case is having a partial function with only a single case statement that you assign to a variable, in Scala2 you would write something like this

val myFunc = { case a => "this is case a" }

In Scala3 you can also write this as

val myFunc
  = case SomeCase => "this is case a"

but NOT

val myFunc = case SomeCase => "this is case a"

As you can see, a decent argument that having

val myFunc
  = case SomeCase => "this is case a"

as valid Scala3 code is incredibly confusing since most people will not read that as a partial function but myFunc is equal to a single case statement (which doesn't make too much sense). The difference is so subtle that it actually created a bug in Scalafmt since Scalafmt typically formats equality statements with = to be on the same line which in this case actually creates invalid code.

My personal suggestion would be that only for the case of partial functions, the {,} should not be optional in the Scala3 dialect but I am not sure if its too late for this.

som-snytt commented 3 years ago

The indent follows the =:

abstract class C:
  def f: String => String

class D extends C:
  def f =
    case s => s + s

@main def test() = println {
  D().f("X")
}

I think "scalafmt should not remove indent token" is a good rule. Some transforms are OK, { x } to x, but not { case x => } to case x =>. Isn't it true that scalafmt must know that?

I like the new "single line catch" syntax, but it doesn't extend to all case syntax, for some reason.

mdedetrich commented 3 years ago

I think "scalafmt should not remove indent token" is a good rule

Sure I mean currently this is a bug within scalafmt but irrespective of scalafmt I view the current syntax as highly confusing (specifically when dealing with anonymous functions that can be assigned).

smarter commented 3 years ago

In Scala3 you can also write this as

val myFunc
 = case SomeCase => "this is case a"

Are you sure? I just tried it and it doesn't work:

object Test:
  val foo: PartialFunction[Int, Int]
    = case x => x
-- [E018] Syntax Error: try/pf.scala:3:6 ---------------------------------------
3 |    = case x => x
  |      ^^^^
  |      expression expected but case found

On the other hand, this works:

object Test:
  val foo: PartialFunction[Int, Int] =
    case x => x

which makes sense since = can introduce an indentation region.

To me, we should go in the other direction than what you're proposing and consider case x => x to be a valid expression so that it can appear without braces or indentation.

mdedetrich commented 3 years ago

Are you sure? I just tried it and it doesn't work:

I guess I made an error in creating a reduced example, but you can look at https://github.com/scalameta/scalafmt/issues/2687#issuecomment-901882754 for the original reproduction

To me, we should go in the other direction than what you're proposing and consider case x => x to be a valid expression so that it can appear without braces or indentation.

Although I do like this option less I still prefer it over the current status quo, consistency should be important.

LPTK commented 3 years ago

To me, we should go in the other direction than what you're proposing and consider case x => x to be a valid expression so that it can appear without braces or indentation.

I think this should be allowed. I don't see a reason for it not to work.

It's quite cumbersome to go from xs.map(x => ...) to xs.map { case C(x, y) => ... }. I'd often rather write xs.map(case C(x, y) => ...).

Bonus point if this works:

xs collect case Left(x) => x
som-snytt commented 3 years ago

Single line catch syntax uses ExprCaseClause, where the expr after the arrow is required:

try x catch case C(_) => ()

It's hard to go back after quiet syntax.

Speaking again to the original confusion, I think it is confusing if the interview question is, "What are the rules for indentation regions?", but less confusing in usage. After quiet case syntax in catch, I wanted it everywhere and was confused that I couldn't have it.

dwijnand commented 3 years ago

It's quite cumbersome to go from xs.map(x => ...) to xs.map { case C(x, y) => ... }. I'd often rather write xs.map(case C(x, y) => ...).

Doing that means that xs.map(case (x, y) => ...) is possible, allowing auto-tupling (which is really auto-untupling) to be dropped... :shipit:

smarter commented 3 years ago

allowing auto-tupling (which is really auto-untupling) to be dropped...

Do you mean https://docs.scala-lang.org/scala3/reference/other-new-features/parameter-untupling.html ? Even if partial function literal syntax becomes valid as an expression, I wouldn't want to drop that: not having to worry about the difference between a Function2[...] and a Function1[Tuple2[...]] is really convenient, and the previous behavior was confusing for newcomers.

mdedetrich commented 3 years ago

@som-snytt

I think "scalafmt should not remove indent token" is a good rule. Some transforms are OK, { x } to x, but not { case x => } to case x =>. Isn't it true that scalafmt must know that?

According to https://github.com/scalameta/scalafmt/issues/2687#issuecomment-903112072, scalafmt is not aware of this and apparently its a pretty significant change (I suspect that its because its a new exception to how scalafmt treats indenting)

I think the core point is this (from the original reproduction case)

def lift =
 case PropertyAlias(a, b) => '{ PropertyAlias(${a.expr}, ${b.expr}) }

As a way to assign values to partial functions is completely new to scalafmt and because of this scalafmt is just treating it as a standard assignment and for the same reasons

 val x =
  "x"

is equivalent to when formatting code

 val x = "x"

scalafmt sees no issue in flattening the assignment to a single line by removing the newline causing the issue.

I am getting the impression that Scala3 was too liberal in copying Python/Ruby in removing optional {,} by doing it almost everywhere rather than only in cases where its truly unambiguous. In cases with top level body definitions inside of class/trait/object it makes sense, but with other cases like this one I can easily see this being terrible confusing for new people (its definitely a bit of a wtf for me) but thats just my opinion.

I mean I am happy with scala3 accepting

def lift = case PropertyAlias(a, b) => '{ PropertyAlias(${a.expr}, ${b.expr}) }

vs the status quo right now but this is yet another thing new Scala3 ussers have to remember because when you read it literally it is saying " I want to assign lift to a single case statement" (which doesn't make too much sense).

EDIT: A new PR is open in scalafmt for this issue so it seems I was incorrect about the "it being difficult" statement but everything else stands

som-snytt commented 3 years ago

We cope with a great deal of ambiguity that is mitigated by "coding style" and not only by correctness.

This is ambiguous:

val myFunc = { case a => "this is case a" }

because you don't know if it's a Function or PartialFunction. Now, that's confusing.

Optional braces has the virtue of being just syntax, and reversible.

mdedetrich commented 3 years ago

Of course there are different types of ambiguities (although I disagree with your example because you can easily tell if a function is partial depending on the value of a, i.e if its covering subset of possible inputs or not) but I am specifically talking about syntax ambiguities.

For example if you google "python whitespace ambiguities" you can find examples where due to how whitespace/indentation is parsed in python it can create puzzling results and my point is that I (personally) think this is an equivalent case but in Scala

LPTK commented 3 years ago

I disagree with your example because you can easily tell if a function is partial depending on the value of a, i.e if its covering subset of possible inputs or not

Is this a partial function?

class Foo extends Bar:
  def f = { case Some(x) => x }

The only way to know is to look arbitrarily far in the code base, to find either something like

abstract class Bar:
  def f: Some[Int] => Int

or like

abstract class Bar:
  def f: PartialFunction[Option[Int], Int]

which changes the answer. So much for "easy to tell".

I am specifically talking about syntax ambiguities

There are no syntactic ambiguities here. When they are not used to group expressions whose grouping would be ambiguous otherwise, curly braces are pure noise.

This is ambiguous:

x match case a: A => a match case B => b; case C => c

// better as
x match case a: A => a match { case B => b; case C => c }
// or
x match case a: A => a match { case B => b }; case C => c

// or as
x match
case a: A =>
  a match
  case B => b
  case C => c
// or
x match
case a: A =>
  a match
  case B => b
case C => c

These are not ambiguous:

x match case a: A => a

case SomeCase => "this is case a"
mdedetrich commented 2 years ago

@LPTK Maybe ambiguous was the wrong word to use here, I don't mean that the technically the parsing is ambiguous but rather visually it looks ambiguous (which has nothing to do with the parser but rather that syntactically/visually the variations look so similar that it can be confusing to notice the subtle difference).

LPTK commented 2 years ago

syntactically/visually the variations look so similar that it can be confusing to notice the subtle difference

To be clear, I feel like both of these should be accepted:

val myFunc = case SomeCase => "this is case a"
val myFunc =
  case SomeCase => "this is case a"

It only seems more consistent this way.

Accepting that both should be valid, I don't see the visual ambiguity problems you are hinting at.

mdedetrich commented 2 years ago

Sure I can accept that as long as they are also parsed to the same AST, for me its about consistency. The only caveat here is that this is something thats unique to Scala3 because having a case statement that is just "alone" (if thats the best way to put it) is a first. Typically a case has meaning in a context when combined with something else that also has syntax that identifies this context (i.e. a case within a map or a case within a partial function)

som-snytt commented 2 years ago

My previous comment about PartialFunction vs Function intended to say that { case P => } depends on context for semantics, because "pattern matching anonymous function" is Partial or not depending on the expected type. (I did not mean "how do you know if it's a total function.") So it's not just syntax, it's type-driven.

The "naked case" syntax looks a bit bare or spare, but I like it. It sounds like there is some consensus about the improved consistency (even if one doesn't prefer the syntax). I like the single-line catch, and it's extra to remember that it only works for catch.

IIUC, the reference to "visual ambiguity" had to do with "consistency": the two variants look similar and should parse the same. (Just to rephrase the previous comments for my benefit.)

mdedetrich commented 2 years ago

So I guess there is general consensus here that at least as a first step we should make the cases consistent? i.e. making

val myFunc = case SomeCase => "this is case a"

parsable and equivalent to the current

val myFunc =
  case SomeCase => "this is case a"