Skip to content

Allow indentation to work inside parens #10969

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 3 commits into from
Jan 6, 2021

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jan 1, 2021

Actually indentation was always enabled inside parens, but the rule which indentation
width to use was too simplistic and often required an extra line.

This is now fixed, so both of these work:

def test1 = g(1, x =>
    val y = x * x
    y * y
  )

def test2 = g(1,
  x =>
    val y = x * x
    y * y
  )

Actually indentation was always enabled inside parens, but the rule which indentation
width to use was too simplistic and often required an extra line.

This is now fixed, so both of these work:

```scala
def test1 = g(1, x =>
    val y = x * x
    y * y
  )

def test2 = g(1,
  x =>
    val y = x * x
    y * y
  )
```
@odersky
Copy link
Contributor Author

odersky commented Jan 1, 2021

This does not work currently because we find idioms like these

f[[X, Y] =>
      X
      |
      Y
 ]

Since we are in an indentation region after =>, a newline is inserted. Previously that was not the case, because the
old algorithm decided that the indentation width of the outer [ region was the indentation width of the X after =>, so
no indentation level was opened up. But now there is one.

I believe the best way out would be to go further, and also allow leading infix operators that appear on their own as only token on a line. We want that anyway since it seems a pity that

condition1
|| condition2

should work but

condition1
||
condition2

doesn't. Let's see how much breaks when we enable this.

@odersky
Copy link
Contributor Author

odersky commented Jan 1, 2021

Summary so far:

  • two generalizations of syntax rules:

    1. Allow indentation within parentheses and brackets without requiring an extra line
    2. Allow infix operators on their own line
  • Nothing broke, or rather, a single test broke, which tested specifically the pathological behavior we are now abolishing, i.e.

     1
     +
     2

    in the test meant a statement 1, followed by an expression +2.

@odersky odersky requested a review from smarter January 1, 2021 23:08
Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM, this matches what I had in mind!

@@ -345,14 +345,15 @@ object Scanners {
allowLeadingInfixOperators
&& ( token == BACKQUOTED_IDENT
|| token == IDENTIFIER && isOperatorPart(name(name.length - 1)))
&& ch == ' '
&& ch <= ' '
Copy link
Member

@smarter smarter Jan 5, 2021

Choose a reason for hiding this comment

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

If this is trying to match all whitespace characters, then it would be better to use an isSpace method for clarity (we can reuse the existing https://github.com/lampepfl/dotty/blob/ea65338e06142f41f0e68b23250c8e22b0ba8837/compiler/src/dotty/tools/dotc/parsing/xml/Utility.scala#L113-L119), this would also avoid matching non-space characters that precede ' ' in ASCII like \b.

The documentation of this method also needs to be updated: it currently states "it is followed on the same line by at least one ' ' and a token that can start an expression."

@odersky odersky force-pushed the indent-parens.scala branch from 476a572 to f767c00 Compare January 6, 2021 13:44
@odersky odersky merged commit 0dc824c into scala:master Jan 6, 2021
@odersky odersky deleted the indent-parens.scala branch January 6, 2021 15:00
@anatoliykmetyuk anatoliykmetyuk added the release-notes Should be mentioned in the release notes label Feb 10, 2021
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants