Patch indentation when removing braces (and other bug fixes in -indent -rewrite)
#17522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
adpi2
force-pushed
the
insert-indent
branch
10 times, most recently
from
e355923
to
017c518
Compare
adpi2
force-pushed
the
insert-indent
branch
12 times, most recently
from
a415bdc
to
c7a3eb5
Compare
This was referenced May 30, 2023
adpi2
changed the title
adpi2
added a commit
to adpi2/scala3
that referenced
this pull request
May 31, 2023
If the first indentation of the region is greater than the indentation
of the enclosing region, we use it to indent the whole region.
Otherwise we use the incremented indentation of the enclosing region.
```scala
def foo = {
x // we replicate indentation of x downward in region
y
}
```
```scala
def foo = {
x // indentation of x is incorrect, we increment enclosing indentation
y
}
```
A bigger indentation than the required one is permitted except
just after a closing brace.
```scala
def bar = {
x
.toString // permitted indentation
def foo = {
}
bar // must be unindented, to not fall into the body of foo
}
```
And other bug fixes (see scala#17522)
adpi2
commented
Jun 19, 2023
| @@ -1545,7 +1553,7 @@ object Scanners { | |||
|
|
|||
| /* Initialization: read first char, then first token */ | |||
| nextChar() | |||
| nextToken() | |||
| (this: @unchecked).nextToken() | |||
There was a problem hiding this comment.
The initialization of ToIndentParser is unsafe because the ctor of Parser calls in.nextToken() where in is a ToIndentScanner and can access uninitialized fields in ToIndentParser. In practice the if token != EMPTY in ToIndentScanner.nextToken prevents this from happening.
|
|
||
| import ast.untpd._ | ||
|
|
||
| class ScriptParser(source: SourceFile)(using Context) extends Parser(source) { |
There was a problem hiding this comment.
It seems that ScriptParser is no so much useful anymore and I would like to avoid adding a ToIndentScriptParser.
|
@adpi2 Some tests are still failing. |
Remove ScriptParsers
We cannot remove braces after a new line, to prevent from removing braces in stat seq.
|
The CI is green again. |
|
ping @odersky? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Primary goal
The goal of this PR is to ensure the
-identrewrite does not break the compilation, by patching the indentation where needed. It fixes #17456.For instance, if we consider this piece of code:
It used to be rewritten to:
After the PR it is rewritten to:
They are many other patterns of compilation breakages involving indentation: in control structure, in pattern matching, in argument passing.
The PR consists mostly in two parts:
Some important considerations are:
should stay unchanged. The rewrite is not a formatting tool, it tries to do as few changes as possible.
The associated tests are indent-3-spaces.scala and indent-mix-tab-space.
Secondary goals
While working on this I found many other bugs or unintended behaviors that I fixed along the way.
Disabling rewrite in Outline Parser
The outline parser, that parses the source path, should not emit any patch.
Preservation of comments
The indent rewrite should not remove any comments.
should be rewritten to
The associated test is indent-comments.scala.
Back-quoting operators in brace-less syntax
foo >=> { bar }is rewritten to
foo `>=>`: barDo not remove braces after
}foo { 5 } { 6 }should not be rewritten to:
foo { 5 }: // missing arguments for method foo 6but left unchanged.
Do not remove braces of blocks in sequence of statements
Fix #10986
{ given Foo = foo1 doSomethingWithFoo } { given Foo = foo2 doSomethingWithFoo }should not be rewritten to:
In general we cannot remove the braces of a standalone block in a sequence of statements.
Back-quoting
endident inend match {Otherwise it is interpreted as an end marker.
Tests and results
Added tests
Rewritting dotty to indent
The output of running the rewrite in the
scala3-libraryandscala3-compilercan be found in #17618.In summary, it changed 13.381 lines and removed 10.034 lines. It removed more than 10.000 pairs of
{}. In the other 3.350 lines there are some} else {rewritten toelse, some} // commentrewritten to// commentbut also some fixed indentations.Here are a few noticeable patches:
Running the rewrite in the community build
Here is the current status of running the rewrite in the entire community build (in #17617):
Community Build A
-no-identwas used)a test fails because of source lines
Community Build B
Community Build C
Why does the rewrite fails in some community build projects?
-no-indentit is not possible to rewrite it to-indent.-new-syntax -rewritebefore running-indent -rewrite. Some exotic patterns that are valid with the new control structure syntax are not with the combination of the old and brace-less syntax. For instance:This is valid:
This is valid:
But this is not valid: