Skip to content

Document additional optional braces conditions #12090

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 Apr 14, 2021 · 8 comments · Fixed by #12136
Closed

Document additional optional braces conditions #12090

tgodzik opened this issue Apr 14, 2021 · 8 comments · Fixed by #12136

Comments

@tgodzik
Copy link
Contributor

tgodzik commented Apr 14, 2021

Compiler version

3.0.0-RC2

Minimized code

The below examples work with optional braces, but are undocumented, which caused us to skip it while implementing the parser:

  for{ a <- 0 to 11 }
      println(a)
      println(a)
  for( a <- 0 to 11 )
      println(a)
      println(a)
  while( a < 10)
      a += 1
      println(a)

Are we missing anything?

Expectation

We should have optional braces properly documented, otherwise it's hard to build tooling around it, but also it might cause suprises to the users.

@bishabosha
Copy link
Member

bishabosha commented Apr 14, 2021

I would say it is documented in the syntax http://dotty.epfl.ch/docs/reference/syntax.html:

Expr1             ::=  ...
                    |while’ ‘(’ Expr ‘)’ {nl} Expr
BlockExpr         ::=  <<< (CaseClauses | Block) >>> // BlockExpr <- SimpleExpr <- PrefixExpr <- InfixExpr <- PostfixExpr <- Expr1 <- Expr
<<< ts >>>        ::=  ‘{’ ts ‘}’
                    |  indent ts outdent
<<< ts >>>        ::=  [nl] ‘{’ ts ‘}’
                    |  `:` indent ts outdent

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 14, 2021

I haven't noticed it there 🤔 and also it's still kind of weird to have the additional do, then keyword, but still be able to just not use them :(

I would however add it here for the users: http://dotty.epfl.ch/docs/reference/other-new-features/indentation.html

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 14, 2021

Ok, thanks @bishabosha for pointing me to the syntax, I haven't noticed that it's actually included there. I thought that the optional braces page was better to look at. It seems we don't have anything else besides these 3 I mentioned.

@odersky
Copy link
Contributor

odersky commented Apr 14, 2021

Good point. The optional braces after for(..._) are probably an oversight. At some point we allowed optional braces after
if (...) as well but then we retracted since we said that was just too much choice at the boundary between new and old syntax. It seems we overlooked for (...). I think we should probably be uniform and either accept indentation after if (...) again or drop it after for (...). And among these two choices my slight tendency at the moment would be to drop it after for (...). But for 3.0 everything should stay as it is, and we should get more experience what users think before making the next moves.

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 14, 2021

Good point. The optional braces after for(..._) are probably an oversight. At some point we allowed optional braces after
if (...) as well but then we retracted since we said that was just too much choice at the boundary between new and old syntax.

I think that we decided that if (cond) <indentation> should stay at least until 3.1 ?

This works in 3.0.0-RC2:

  if (true)
    println(1)
    println(2)

We can add scalafix rules for all the syntax changes by then, so in most cases this should be straightforward to migrate.

@odersky
Copy link
Contributor

odersky commented Apr 14, 2021

Ah, yes, you are right of course. I got my versions crossed. So yes, everything behaves in the same way and we are good.

@odersky
Copy link
Contributor

odersky commented Apr 14, 2021

I actually don't see a discussion of indentation after if (...) in http://dotty.epfl.ch/docs/reference/other-new-features/indentation.html either. @tgodzik do you agree that we need to document that case as well?

@tgodzik
Copy link
Contributor Author

tgodzik commented Apr 14, 2021

Sure! That makes sense.

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

Successfully merging a pull request may close this issue.

3 participants