Enforce a blank line after local function declarations.

R=nweiz@google.com Review URL: https://codereview.chromium.org//2186343003 .
dart-lang · Jul 28, 2016 · 8b5aa7e · 8b5aa7e
1 parent 2b4d8df
commit 8b5aa7e
Show file tree

Hide file tree

Showing 4 changed files with 149 additions and 74 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,6 +3,7 @@
 * Make it strong mode clean.
 * Put labels on their own line (#43).
 * Gracefully handle IO errors when failing to overwrite a file (#473).
+* Add a blank line after local functions, to match top level ones (#488).
 * Improve indentation in non-block-bodied control flow statements (#494).
 * Better indentation on very long return types (#503).
 * When calling from JS, guess at which error to show when the code cannot be

diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart
@@ -253,18 +253,43 @@ class SourceVisitor implements AstVisitor {
       return;
     }
 
-    // For a block that is not a function body, just bump the indentation and
-    // keep it in the current block.
-    if (node.parent is! BlockFunctionBody) {
-      _writeBody(node.leftBracket, node.rightBracket, body: () {
-        visitNodes(node.statements, between: oneOrTwoNewlines, after: newline);
-      });
-      return;
+    // If the block is a function body, it may get expression-level indentation,
+    // so handle it specially. Otherwise, just bump the indentation and keep it
+    // in the current block.
+    if (node.parent is BlockFunctionBody) {
+      _startLiteralBody(node.leftBracket);
+    } else {
+      _beginBody(node.leftBracket);
+    }
+
+    var needsDouble = true;
+    for (var statement in node.statements) {
+      if (needsDouble) {
+        twoNewlines();
+      } else {
+        oneOrTwoNewlines();
+      }
+
+      visit(statement);
+
+      needsDouble = false;
+      if (statement is FunctionDeclarationStatement) {
+        // Add a blank line after non-empty block functions.
+        var body = statement.functionDeclaration.functionExpression.body;
+        if (body is BlockFunctionBody) {
+          needsDouble = body.block.statements.isNotEmpty;
+        }
+      }
     }
 
-    _startLiteralBody(node.leftBracket);
-    visitNodes(node.statements, between: oneOrTwoNewlines, after: newline);
-    _endLiteralBody(node.rightBracket, forceSplit: node.statements.isNotEmpty);
+    if (node.statements.isNotEmpty) newline();
+
+    if (node.parent is BlockFunctionBody) {
+      _endLiteralBody(node.rightBracket,
+          forceSplit: node.statements.isNotEmpty);
+    } else {
+      _endBody(node.rightBracket);
+    }
   }
 
   visitBlockFunctionBody(BlockFunctionBody node) {
@@ -406,38 +431,40 @@ class SourceVisitor implements AstVisitor {
     space();
 
     builder.unnest();
-    _writeBody(node.leftBracket, node.rightBracket, body: () {
-      if (node.members.isNotEmpty) {
-        for (var member in node.members) {
-          visit(member);
-
-          if (member == node.members.last) {
-            newline();
-            break;
-          }
+    _beginBody(node.leftBracket);
 
-          var needsDouble = false;
-          if (member is ClassDeclaration) {
-            // Add a blank line after classes.
-            twoNewlines();
-          } else if (member is MethodDeclaration) {
-            // Add a blank line after non-empty block methods.
-            if (member.body is BlockFunctionBody) {
-              var body = member.body as BlockFunctionBody;
-              needsDouble = body.block.statements.isNotEmpty;
-            }
-          }
+    if (node.members.isNotEmpty) {
+      for (var member in node.members) {
+        visit(member);
 
-          if (needsDouble) {
-            twoNewlines();
-          } else {
-            // Variables and arrow-bodied members can be more tightly packed if
-            // the user wants to group things together.
-            oneOrTwoNewlines();
+        if (member == node.members.last) {
+          newline();
+          break;
+        }
+
+        var needsDouble = false;
+        if (member is ClassDeclaration) {
+          // Add a blank line after classes.
+          twoNewlines();
+        } else if (member is MethodDeclaration) {
+          // Add a blank line after non-empty block methods.
+          if (member.body is BlockFunctionBody) {
+            var body = member.body as BlockFunctionBody;
+            needsDouble = body.block.statements.isNotEmpty;
           }
         }
+
+        if (needsDouble) {
+          twoNewlines();
+        } else {
+          // Variables and arrow-bodied members can be more tightly packed if
+          // the user wants to group things together.
+          oneOrTwoNewlines();
+        }
       }
-    });
+    }
+
+    _endBody(node.rightBracket);
   }
 
   visitClassTypeAlias(ClassTypeAlias node) {
@@ -480,33 +507,30 @@ class SourceVisitor implements AstVisitor {
 
     visitNodes(directives, between: oneOrTwoNewlines);
 
-    if (node.declarations.isNotEmpty) {
-      var needsDouble = true;
-
-      for (var declaration in node.declarations) {
-        // Add a blank line before classes.
-        if (declaration is ClassDeclaration) needsDouble = true;
-
-        if (needsDouble) {
-          twoNewlines();
-        } else {
-          // Variables and arrow-bodied members can be more tightly packed if
-          // the user wants to group things together.
-          oneOrTwoNewlines();
-        }
+    var needsDouble = true;
+    for (var declaration in node.declarations) {
+      // Add a blank line before classes.
+      if (declaration is ClassDeclaration) needsDouble = true;
 
-        visit(declaration);
+      if (needsDouble) {
+        twoNewlines();
+      } else {
+        // Variables and arrow-bodied members can be more tightly packed if
+        // the user wants to group things together.
+        oneOrTwoNewlines();
+      }
 
-        needsDouble = false;
-        if (declaration is ClassDeclaration) {
-          // Add a blank line after classes.
-          needsDouble = true;
-        } else if (declaration is FunctionDeclaration) {
-          // Add a blank line after non-empty block functions.
-          if (declaration.functionExpression.body is BlockFunctionBody) {
-            var body = declaration.functionExpression.body as BlockFunctionBody;
-            needsDouble = body.block.statements.isNotEmpty;
-          }
+      visit(declaration);
+
+      needsDouble = false;
+      if (declaration is ClassDeclaration) {
+        // Add a blank line after classes.
+        needsDouble = true;
+      } else if (declaration is FunctionDeclaration) {
+        // Add a blank line after non-empty block functions.
+        var body = declaration.functionExpression.body;
+        if (body is BlockFunctionBody) {
+          needsDouble = body.block.statements.isNotEmpty;
         }
       }
     }
@@ -734,9 +758,9 @@ class SourceVisitor implements AstVisitor {
     visit(node.name);
     space();
 
-    _writeBody(node.leftBracket, node.rightBracket, space: true, body: () {
-      visitCommaSeparatedNodes(node.constants, between: split);
-    });
+    _beginBody(node.leftBracket, space: true);
+    visitCommaSeparatedNodes(node.constants, between: split);
+    _endBody(node.rightBracket, space: true);
   }
 
   visitExportDirective(ExportDirective node) {
@@ -2156,13 +2180,9 @@ class SourceVisitor implements AstVisitor {
     _collectionArgumentLists[leftBracket] = argumentList;
   }
 
-  /// Writes an bracket-delimited body and handles indenting and starting the
-  /// rule used to split the contents.
-  ///
-  /// If [space] is `true`, then the contents and delimiters will have a space
-  /// between then when unsplit.
-  void _writeBody(Token leftBracket, Token rightBracket,
-      {bool space: false, body()}) {
+  /// Writes the beginning of a brace-delimited body and handles indenting and
+  /// starting the rule used to split the contents.
+  void _beginBody(Token leftBracket, {bool space: false}) {
     token(leftBracket);
 
     // Indent the body.
@@ -2171,9 +2191,10 @@ class SourceVisitor implements AstVisitor {
     // Split after the bracket.
     builder.startRule();
     builder.split(isDouble: false, nest: false, space: space);
+  }
 
-    body();
-
+  /// Finishes the body started by a call to [_beginBody].
+  void _endBody(Token rightBracket, {bool space: false}) {
     token(rightBracket, before: () {
       // Split before the closing bracket character.
       builder.unindent();

diff --git a/test/regression/0400/0488.stmt b/test/regression/0400/0488.stmt
@@ -0,0 +1,13 @@
+>>>
+{
+  fn(x) { ; }
+  items.forEach(fn);
+}
+<<<
+{
+  fn(x) {
+    ;
+  }
+
+  items.forEach(fn);
+}
diff --git a/test/whitespace/blocks.stmt b/test/whitespace/blocks.stmt
@@ -54,4 +54,44 @@
   var a;
   ;
   ;
+}
+>>> force blank line after non-empty local function
+{
+  a() {;}
+  b();
+
+
+  c() {;}d(){;}
+
+
+}
+<<<
+{
+  a() {
+    ;
+  }
+
+  b();
+
+  c() {
+    ;
+  }
+
+  d() {
+    ;
+  }
+}
+>>> do not force blank line after empty local function
+{ a() {} b() {} }
+<<<
+{
+  a() {}
+  b() {}
+}
+>>> do not force blank line after => body local function
+{ a() => null; b() => null; }
+<<<
+{
+  a() => null;
+  b() => null;
 }