Skip to content

Optional braces not allowed for pattern match guard #11444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tgodzik opened this issue Feb 17, 2021 · 11 comments
Closed

Optional braces not allowed for pattern match guard #11444

tgodzik opened this issue Feb 17, 2021 · 11 comments

Comments

@tgodzik
Copy link
Contributor

tgodzik commented Feb 17, 2021

Compiler version

3.0.0-RC1

Minimized code

  1 match {
     case a if 
         val b = 11
         a < b =>
       a
  } 

Output

[info] compiling 1 Scala source to /home/tgodzik/Documents/workspaces/dotty-vinz/target/scala-3.0.0-M4/classes ...
[error] -- [E018] Syntax Error: /home/tgodzik/Documents/workspaces/dotty-vinz/src/main/scala/Main.scala:18:14 
[error] 18 |     case a if 
[error]    |              ^
[error]    |              expression expected but val found
[error] -- [E007] Type Mismatch Error: /home/tgodzik/Documents/workspaces/dotty-vinz/src/main/scala/Main.scala:18:11 
[error] 18 |     case a if 
[error]    |           ^
[error]    |           Found:    Null
[error]    |           Required: Boolean
[error] two errors found
[error] two errors found
[error] (Compile / compileIncremental) Compilation failed
[error] Total time: 1 s, completed Feb 17, 2021 5:29:10 PM

Expectation

Compiles succesfully.

It seems this works in both normal ifs and if in for comprehensions, so this seems a bit of an inconsistency.

@liufengyun
Copy link
Contributor

The syntax says:

Guard             ::=  ‘if’ PostfixExpr
PostfixExpr       ::=  InfixExpr [id]                                          -- only if language.postfixOperators is enabled
InfixExpr         ::=  PrefixExpr
                    |  InfixExpr id [nl] InfixExpr
                    |  InfixExpr MatchClause
PrefixExpr        ::=  [‘-’ | ‘+’ | ‘~’ | ‘!’] SimpleExpr
SimpleExpr        ::=  SimpleRef
                    |  ...
                    |  BlockExpr
                    |  ...
BlockExpr         ::=  <<< (CaseClauses | Block) >>>

From the spec, it seems to be allowed. But such code in practice is not readable, we may also align the spec to reject the code (if the fix to the syntax is simple).

The reason why the parser does not see the block is that in parsing guard, no indent is inserted.

@liufengyun
Copy link
Contributor

Here is the rule for inserting indentations:

 1. An `<indent>` is inserted at a line break, if

     - An indentation region can start at the current position in the source, and
     - the first token on the next line has an indentation width strictly greater
       than the current indentation width

    An indentation region can start

     - after the leading parameters of an `extension`, or
     - after a `with` in a given instance, or
     - after a ": at end of line" token (see below)
     - after one of the following tokens:

       ```
       =  =>  ?=>  <-  catch  do  else  finally  for
       if  match  return  then  throw  try  while  yield
       ```

The rule above says that <indent> is inserted after if at a line break.

@tgodzik
Copy link
Contributor Author

tgodzik commented Feb 18, 2021

The rule above says that is inserted after if at a line break.

Yeah, it seems to be allow in a guard.

I already reject it in the scalameta parser, but would love to have some clarification on this.

@liufengyun
Copy link
Contributor

The following code is valid (compiles without problem):

  1 match {
    case a
    if
      val b = 11
      a < b
    =>
      a
  }

The if in the new line will effectively set the current indentation width to 4, which is smaller than the indentation 6 in the next line, thus <indent> is inserted.

In contrast, in the following code:

  1 match {
     case a if 
         val b = 11
         a < b =>
       a
  } 

The if in same line in the case region will make the current indentation width to be the same as in the next line, thus NO <indent> is inserted.

Related code:

https://github.com/lampepfl/dotty/blob/9e34c72901fe305ffdf221b3117f70ce8cce196b/compiler/src/dotty/tools/dotc/parsing/Scanners.scala#L484-L485

https://github.com/lampepfl/dotty/blob/9e34c72901fe305ffdf221b3117f70ce8cce196b/compiler/src/dotty/tools/dotc/parsing/Scanners.scala#L1375-L1385

@liufengyun
Copy link
Contributor

@odersky is the two different <indent> insertion behavior in the two examples above intended?

@dwijnand
Copy link
Member

dwijnand commented Sep 8, 2021

Reading https://www.scala-lang.org/blog/2021/09/07/scala-3.0.2-released.html:

If your intention is to have a block of code evaluating into a single condition you should add a new line and indentation directly after if, e.g.

if
  val cond = foo(bar)
  cond
then //...

I was hoping that this variant to the original test case would now work:

  1 match {
     case a if 
              val b = 11
              a < b =>
       a
  } 

But sadly you still get:

-- [E018] Syntax Error: foo.scala:3:14 -----------------------------------------
3 |     case a if
  |              ^
  |              expression expected but val found

@odersky
Copy link
Contributor

odersky commented Apr 8, 2022

The following would work:

val _ = 1 match {
     case a
       if
         val b = 11
         a < b
        =>
       a
  }

And this works as well:

val _ = 1 match {
     case
       a if
         val b = 11
         a < b
        =>
       a
  }

The reason the first example does not work is that there is an implicit opening region where the pattern a starts. That region has an unknown indentation width since it does not start at the beginning of a line. So the indentation after if is not recognized, since in fact that indentation defines the indentation width of the enclosing region.

I don't think there's anything we can reasonably do here.

@odersky odersky closed this as completed Apr 8, 2022
@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 8, 2022

If we can't find the indentation of the enclosing region, shouldn't we just use the one enclosing it?

Are we saying that:

  1 match {
     case a if 
         val b = 11
         a < b =>
       a
  } 

will just nor work?

I don't think this is quite legible for the users and it feels like we have a bunch of hidden indentation rules or at least difficult ones that are not exactly user friendly.

@som-snytt
Copy link
Contributor

I recently became a fan of aligning case, if, => (as I saw in some gnarly matches in the codebase).

I'm glad to see that I can go to town left-adjusting the pattern.

For comprehensions offer less room for personal expression, but generator syntax is pretty generous.

class C:
  def f =
    42 match
    case
x
    if x > 27
    =>
      17

  def g(xs: List[Int]) =
    for
      x
      <- xs
          if
            x
        > 17
      y = x+1
    yield y

That's just exploiting leading infix. This outdents as expected:

  def g(xs: List[Int]) =
    for
      x
      <- xs if
        var b = false
        b
      = x > 17
        b
      y = x+1
    yield y

with an interesting message that sounds like the converse of the usual unindent unexpected

unindent expected, but '=' found

@som-snytt
Copy link
Contributor

My recent remotely related question at #14738 (comment) was whether the example on that ticket

    try op
    catch case _: NullPointerException =>
    handler

should already use "enclosing" indentation so this would be normal syntax

catch case K0 =>
case K1 =>

but I haven't thought about it again and in fact I've already forgotten everything I learned about indentation rules for optional braces, so it's nice that they mostly just work.

@odersky
Copy link
Contributor

odersky commented Apr 8, 2022

If we can't find the indentation of the enclosing region, shouldn't we just use the one enclosing it?

That's generally not how it works. One problem would be to decide when a region ends. If there are several reasons with the same indentation level that's an issue. So that's one reason why it is generally not done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants