Skip to content

syntax.md: update syntax of for-expressions #11624

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

Merged
merged 1 commit into from
Jul 8, 2021

Conversation

unkarjedy
Copy link
Contributor

Comments to ForExpr ::=

  1. new line before yield or do is supported in braceless syntax as well
  2. do can also go in for-expressions with braces/parenthesis
  3. we could save some space and replace 3 lines with 2 by merging (‘(’ Enumerators ‘)’ | ‘{’ Enumerators ‘}’)
    but I find my version more readable

Comments to Enumerators0 ::=

  1. enumerators can be preceded by an arbitrary amount of newline character
  2. ; is possible after the last enumerator

All-in-one example with valid for-expressions which are accepted by Scala 3 compiler:

for x <- 0 to 1 do println(42)
for x <- 0 to 1 yield 42

for { x <- 0 to 1 } do println(42)
for { x <- 0 to 1 } yield 42

for (x <- 0 to 1) do println(42)
for (x <- 0 to 1) yield 42

////////////////////////////////////////

for x <- 0 to 1
do println(42)
for x <- 0 to 1
yield 42

for { x <- 0 to 1 }
do println(42)
for { x <- 0 to 1 }
yield 42

for (x <- 0 to 1)
do println(42)
for (x <- 0 to 1)
yield 42


for (x <- 0 to 1) println(42)
for (x <- 0 to 1)
  println(42)
// DO or YIELD is obligatory when no FOR is braceless
//for x <- 0 to 1 println(42)
//for x <- 0 to 1
// println(42)

for

x <- 0 to 1;

do println(42)

ForExpr ::= ‘for’ ‘(’ Enumerators0 ‘)’ {nl} [‘do‘ | ‘yield’] Expr
| ‘for’ ‘{’ Enumerators0 ‘}’ {nl} [‘do‘ | ‘yield’] Expr
| ‘for’ Enumerators0 {nl} (‘do‘ | ‘yield’) Expr
Enumerators0 ::= {nl} Enumerators [semi]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No {nl} needs to be inserted here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enumerators themselves do not accept new line before them.

No {nl} needs to be inserted here.

Is it because it's kinda "obvious" that here can go an arbitrary amount of new lines?
For me, it's as obvious as {nl} after ) in for / if / while, still they contain {nl} in rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nl means an newline token inserted by the lexer. But there is never a newline inserted in front of a do or yield.

| ‘for’ Enumerators (‘do’ Expr | ‘yield’ Expr) ForDo(enums, expr)
ForExpr ::= ‘for’ ‘(’ Enumerators0 ‘)’ {nl} [‘do‘ | ‘yield’] Expr ForYield(enums, expr) / ForDo(enums, expr)
| ‘for’ ‘{’ Enumerators0 ‘}’ {nl} [‘do‘ | ‘yield’] Expr
| ‘for’ Enumerators0 {nl} (‘do‘ | ‘yield’) Expr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about nl. Also, there's a divergence between parser and syntax.md regarding the trailing [semi] in Enumerators0. The syntax is also not what is supported by the parser. The parser supports trailing semicolons in braces and in braceless syntax but not in parens. That is a question we should clear up. When should enumerators suppport a trailing ;. I believe all other proposed changes to the syntax are unnecessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, any PR for syntax.md needs to change both versions: in internals and in reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser supports trailing semicolons in braces and in braceless syntax but not in parens. That is a question we should clear up

Yeah, that I have missed...

I believe all other proposed changes to the syntax are UNnecessary.

Do you mean that's ok there is a divergence between parser and syntax reference?
(syntax doesn't support do after for with brackets and new line before do/yield in braceless version

Finally, any PR for syntax.md needs to change both versions: in internals and in reference.

The PR contains the change in both reference/syntax.md and internals/syntax.md (you've commented in both files)
What did I miss?

@b-studios b-studios requested a review from odersky April 27, 2021 09:50
ForExpr ::= ‘for’ ‘(’ Enumerators0 ‘)’ {nl} [‘do‘ | ‘yield’] Expr
| ‘for’ ‘{’ Enumerators0 ‘}’ {nl} [‘do‘ | ‘yield’] Expr
| ‘for’ Enumerators0 {nl} (‘do‘ | ‘yield’) Expr
Enumerators0 ::= {nl} Enumerators [semi]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, nl means an newline token inserted by the lexer. But there is never a newline inserted in front of a do or yield.

@dwijnand
Copy link
Member

dwijnand commented Jul 7, 2021

@unkarjedy so to me this looks like Martin approved applying some changes, but then it failed due to failures we had in master. Do you want to rebase (and squash) this branch so we can merge the PR?

@unkarjedy unkarjedy force-pushed the syntax.md/for-expression branch from 1c71721 to bc604e3 Compare July 8, 2021 08:56
@unkarjedy
Copy link
Contributor Author

@dwijnand done

@dwijnand dwijnand enabled auto-merge July 8, 2021 09:17
@dwijnand dwijnand merged commit 0e472bf into scala:master Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants