Make compiler sources invariant under syntax rewritings #7089

odersky · 2019-08-22T19:30:45Z

With this PR, the sequence

dotc -rewrite -no-syntax 
dotc -rewrite -indent 
dotc -rewrite -noindent
dotc -rewrite -old-syntax

leaves every source file under dotc unchanged (with the exception of files under dotc/sbt; I did not touch these).

Based on #7083.

smarter · 2019-08-22T23:01:18Z

compiler/src/dotty/tools/dotc/classpath/ClassPathFactory.scala

@@ -25,7 +25,8 @@ class ClassPathFactory {
    for {
      file <- expandPath(path, expandStar = false)
      dir <- Option(AbstractFile getDirectory file)
-    } yield createSourcePath(dir)
+    }
+    yield createSourcePath(dir)


This is an unusual style, can the rewrite favor putting the yield on the same line as the closing bracket if there is one ?

The yield on the next line is consistent with how we format else. I think it's preferable if section keywords don't hide after a }.

smarter · 2019-08-22T23:03:52Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

-        else if (
-          !(  isType // allow accesses to types from arbitrary subclasses fixes #4737
+        else if
+          (!(  isType // allow accesses to types from arbitrary subclasses fixes #4737


The formatting broke the alignment of isType with the conditions below, if there's no way to preserve it then the spaces in front of isType should be removed at least.

smarter · 2019-08-22T23:06:00Z

compiler/src/dotty/tools/dotc/core/TypeErasure.scala

@@ -118,7 +118,8 @@ object TypeErasure {
    semiEraseVCs <- List(false, true)
    isConstructor <- List(false, true)
    wildcardOK <- List(false, true)
-  } erasures(erasureIdx(isJava, semiEraseVCs, isConstructor, wildcardOK)) =
+  }
+  erasures(erasureIdx(isJava, semiEraseVCs, isConstructor, wildcardOK)) =


same issue than with yield here, I would even say that the newline make the code easy to misinterpret.

That just shows that for without do is evil. I would strongly suggest we get rid of it as fast as possible.

That is one interpretation of the reality, but I fear you are biased in interpreting it that way. An equally valid interpretation of the reality is that the coding style enforced by the rewriting is actively harmful in some situations. A coding style that does not exhibit this issue is to systematically surround the body of a for loop with {} if the generators are themselves surrounded with {}. (This is for example part of the Scala.js coding style.)

Even if we still need to discuss #7024 in general I believe we absolutely have to demand do after while and for. That part should be uncontroversial. Now that do-while is gone there is nothing holding us back and the alternative of having two {...} regions in sequence is just too ugly to keep.

smarter · 2019-08-22T23:08:37Z

compiler/src/dotty/tools/dotc/core/Types.scala

@@ -2363,8 +2361,9 @@ object Types {
  }

  /** Assert current phase does not have erasure semantics */
-  private def assertUnerased()(implicit ctx: Context) =
+  private def assertUnerased()(implicit ctx: Context) = {


Why did braces appear here ?

One armed ifs are always enclosed in braces, in order to avoid accidentally merging them with an enclosing else.

smarter · 2019-08-22T23:11:36Z

compiler/src/dotty/tools/dotc/core/unpickleScala2/Scala2Unpickler.scala

@@ -643,7 +644,7 @@ class Scala2Unpickler(bytes: Array[Byte], classRoot: ClassDenotation, moduleClas
    // Need to be careful not to run into cyclic references here (observed when
    // compiling t247.scala). That's why we avoid taking `symbol` of a TypeRef
    // unless names match up.
-    val isBound = (tp: Type) => {
+    val isBound = { (tp: Type) =>


This is also unusual.

Actually no. This is very common, and arguably objectively better, because it matches the style one gets when using a {}-based parameter list for lambdas. Like in

xs.map { (tp: Type) => ... } ```

sjrd · 2019-08-23T05:56:18Z

The fact that braces are systematically removed when they're not necessary from a syntactic point of view is problematic for the adoption of the rewrite-edit-rewrite workflow. Braces are sometimes used to help readability of nontrivial code blocks. It's quite possible that people will not accept to delete those braces in their codebase in order to support the workflow, and therefore will not try out the syntax.

Anecdotally, in my own coding style braces are required in a number of situations where the parser wouldn't require them.

odersky · 2019-08-23T07:16:18Z

The fact that braces are systematically removed when they're not necessary from a syntactic point of view is problematic for the adoption of the rewrite-edit-rewrite workflow. Braces are sometimes used to help readability of nontrivial code blocks. It's quite possible that people will not accept to delete those braces in their codebase in order to support the workflow, and therefore will not try out the syntax.

Yes, but how do we address this problem? Once braces are gone, how does the compiler know where to put them back?

sjrd · 2019-08-23T08:29:44Z

Yes, but how do we address this problem? Once braces are gone, how does the compiler know where to put them back?

It is probably not possible, but then we should be much more honest in the communication strategy of the rewrite-edit-rewrite workflow. There should be a big "IF your coding style agrees with what -unindent does" when we say that the round-trip is possible. We should not lie by omission.

odersky · 2019-08-23T10:40:01Z

@sjrd OK, I added a qualification "(depending on coding style)" to the other PR.

Additional rewritings to make the compiler source invariant if we also rewrite under braces

odersky · 2019-08-29T17:03:55Z

This is now superseded by #7114

odersky mentioned this pull request Aug 22, 2019

Allow significant indentation syntax #7083

Closed

smarter reviewed Aug 22, 2019

View reviewed changes

odersky force-pushed the change-indent-invariant branch 2 times, most recently from cb53940 to 789c6c6 Compare August 27, 2019 21:31

odersky added 4 commits August 28, 2019 17:53

Support significant indentation

5373727

Make compiler sources invariant under rewriting

f50e7d7

More invariant rewritings

d52edec

Additional rewritings to make the compiler source invariant if we also rewrite under braces

Fix indentation in Implicits

37c553d

odersky force-pushed the change-indent-invariant branch from 948c6b3 to 37c553d Compare August 28, 2019 15:56

odersky closed this Aug 29, 2019

Make compiler sources invariant under syntax rewritings #7089

Make compiler sources invariant under syntax rewritings #7089

Uh oh!

Conversation

odersky commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odersky Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjrd commented Aug 23, 2019

Uh oh!

odersky commented Aug 23, 2019

Uh oh!

sjrd commented Aug 23, 2019

Uh oh!

odersky commented Aug 23, 2019

Uh oh!

odersky commented Aug 29, 2019

Uh oh!

Uh oh!

odersky commented Aug 22, 2019 •

edited

Loading

smarter Aug 22, 2019 •

edited

Loading

odersky Aug 23, 2019 •

edited

Loading