Dart formatter checkpoint.

pkg/analyzer_experimental/lib/src/services/formatter_impl.dart

Index: pkg/analyzer_experimental/lib/src/services/formatter_impl.dart
===================================================================
--- pkg/analyzer_experimental/lib/src/services/formatter_impl.dart	(revision 24194)
+++ pkg/analyzer_experimental/lib/src/services/formatter_impl.dart	(working copy)
@@ -84,21 +84,22 @@
 
   final FormatterOptions options;
   final List<AnalysisError> errors = <AnalysisError>[];

[scheglov, 2013/06/19 22:12:05]

Remove type annotation for the final field.

[pquitslund, 2013/06/20 22:34:52]

Done.

+  final EditRecorder recorder;
 
-  CodeFormatterImpl(this.options);
+  CodeFormatterImpl(FormatterOptions options) : this.options = options,

[scheglov, 2013/06/19 22:12:05]

May be "this.options" and remove initializer?

[pquitslund, 2013/06/20 22:34:52]

The trouble is that I'd like to use options in my creation of the EditRecorder
AND I'd like recorder to be final.  Unfortunately, I can't access options in the
initializer block and that's the only place I can assign to final fields...

(Sorry if this is confusing.  Feel free to walk over if you want me to show you
what's up.)

+      recorder = new EditRecorder(options);
 
   String format(CodeKind kind, String source, {int offset, int end,
       int indentationLevel:0}) {
 
     var start = tokenize(source);
-    _checkForErrors();
+    checkForErrors();
 
     var node = parse(kind, start);
-    _checkForErrors();
+    checkForErrors();
 
-    // To be continued...
-
-    return source;
+    var formatter = new FormattingEngine(options);
+    return formatter.format(source, node, start, kind, recorder);
   }
 
   ASTNode parse(CodeKind kind, Token start) {
@@ -115,7 +116,7 @@
     throw new FormatterException('Unsupported format kind: $kind');
   }
 
-  _checkForErrors() {
+  checkForErrors() {
     if (errors.length > 0) {
       throw new FormatterException.forError(errors);
     }
@@ -132,18 +133,13 @@
 
 }
 
-/// Placeholder class to hold a reference to the Class object representing
-/// the Dart keyword void.
-class Void extends Object {
 
-}
-
-
 /// Records a sequence of edits to a source string that will cause the string
 /// to be formatted when applied.
 class EditRecorder {
 
   final FormatterOptions options;
+  final EditStore editStore;
 
   int column = 0;
 
@@ -152,13 +148,108 @@
 
   Token currentToken;
 
-  int indentationLevel = 0;
   int numberOfIndentations = 0;
 
-  bool isIndentNeeded = false;
+  bool needsIndent = false;
 
-  EditRecorder(this.options);
+  EditRecorder(this.options): editStore = new EditStore();

[Brian Wilkerson, 2013/06/19 21:50:52]

Just curiosity, but why did you move away from using a field formal parameter in
CodeFormatterImpl, but not here? And how did you decide which of these
possibilities to use:

EditRecorder(this.options): editStore = new EditStore();
EditRecorder(FormatterOptions options): this.options = options, editStore = new
EditStore();
EditRecorder(FormatterOptions options): this(options, new EditStore());

or any variation that uses a block body,
or combining this with the 'forStore' constructor into a single

EditRecorder(this.options, [this.editStore = new EditStore()]);

[pquitslund, 2013/06/20 22:34:52]

Discussed (exhaustively) in person!

 
+  EditRecorder.forStore(this.options, this.editStore);
+
+  /// Add an [Edit] that describes a textual [replacement] of a text
+  /// interval starting at the given [offset] spanning the given [length].
+  void addEdit(int offset, int length, String replacement) {
+    editStore.addEdit(offset, length, replacement);
+  }
+
+  /// Advance past the given expected [token] (or fail if not matched).
+  void advance(Token token) {
+    if (currentToken.lexeme == token.lexeme) {
+
+      // TODO(pquitslund) emit comments
+//      if (needsIndent) {
+//        advanceIndent();
+//        needsIndent = false;
+//      }
+      // Record writing a token at the current edit location
+      advanceChars(token.length);
+      currentToken = currentToken.next;
+    } else {
+      wrongToken(token.lexeme);
+    }
+  }
+
+  /// Move indices past indent, adding an edit if needed to adjust indentation
+  void advanceIndent() {
+//    var indentWidth = options.indentPerLevel * indentationLevel;
+//    var indentString = getIndentString(indentWidth);
+//    var sourceIndentWidth = 0;
+//    for (var i = 0; i < source.length; i++) {
+//      if (isIndentChar(source[sourceIndex + i])) {
+//        sourceIndentWidth += 1;
+//      } else {
+//        break;
+//      }
+//    }
+//    var hasSameIndent = sourceIndentWidth == indentWidth;
+//    if (hasSameIndent) {
+//      for (var i = 0; i < indentWidth; i++) {
+//        if (source[sourceIndex + i] != indentString[i]) {
+//          hasSameIndent = false;
+//          break;
+//        }
+//      }
+//      if (hasSameIndent) {
+//        advanceChars(indentWidth);
+//        return;
+//      }
+//    }
+//    addEdit(sourceIndex, sourceIndentWidth, indentString);
+//    column += indentWidth;
+//    sourceIndex += sourceIndentWidth;
+
+    var indent = options.indentPerLevel * numberOfIndentations;
+
+    spaces(indent);
+  }
+
+  String getIndentString(int indentWidth) {
+
+    // TODO(pquitslund) a temporary workaround
+    if (indentWidth < 0) {
+      return '';
+    }
+
+    // TODO(pquitslund) allow indent with tab chars
+
+    // Fetch a precomputed indent string
+    if (indentWidth < SPACES.length) {
+      return SPACES[indentWidth];
+    }
+
+    // Build un-precomputed strings dynamically
+    var sb = new StringBuffer();
+    for (var i=0; i < indentWidth; ++i) {

[scheglov, 2013/06/19 22:12:05]

Whitespaces before/after '='.

[pquitslund, 2013/06/20 22:34:52]

Done.

+      sb.write(' ');
+    }
+    return sb.toString();
+  }
+
+  /// Advance past the given expected [token] (or fail if not matched).
+  void advanceToken(String token) {
+    if (currentToken.lexeme == token) {
+      advance(currentToken);
+    } else {
+      wrongToken(token);
+    }
+  }
+
+  /// Advance [column] and [sourceIndex] indices by [len] characters.
+  void advanceChars(int len) {
+    column += len;
+    sourceIndex += len;
+  }
+
   /// Count the number of whitespace chars beginning at the current
   /// [sourceIndex].
   int countWhitespace() {
@@ -173,9 +264,8 @@
     return count;
   }
 
-  /// Indent.
+  /// Update indent indices.
   void indent() {
-    indentationLevel += options.indentPerLevel;
     numberOfIndentations++;
   }
 
@@ -192,16 +282,98 @@
     return true;
   }
 
+  /// Newline.
+  void newline() {
+    // TODO(pquitslund) emit comments
+    needsIndent = true;
+    // If there is a newline before the edit location, do nothing.
+    if (isNewlineAt(sourceIndex - NEW_LINE.length)) {
+      return;
+    }
+    // If there is a newline after the edit location, advance over it.
+    if (isNewlineAt(sourceIndex)) {
+      advanceChars(NEW_LINE.length);
+      return;
+    }
+    // Otherwise, replace whitespace with a newline.
+    var charsToReplace = countWhitespace();
+    if (isNewlineAt(sourceIndex + charsToReplace)) {
+      ++charsToReplace;

[Brian Wilkerson, 2013/06/19 21:50:52]

Does this want to be incremented by NEW_LINE.length?

[pquitslund, 2013/06/20 22:34:52]

Yes!  Good catch. :)

+    }
+    addEdit(sourceIndex, charsToReplace, NEW_LINE);
+    advanceChars(charsToReplace);
+  }
+
+
+  /// Un-indent.
+  void unindent() {
+    numberOfIndentations--;
+  }
+
+  /// Space.
+  void space() {
+    // TODO(pquitslund) emit comments
+//    // If there is a space before the edit location, do nothing.
+//    if (isSpaceAt(sourceIndex - 1)) {
+//      return;
+//    }
+//    // If there is a space after the edit location, advance over it.
+//    if (isSpaceAt(sourceIndex)) {
+//      advance(1);
+//      return;
+//    }
+    // Otherwise, replace spaces with a single space.
+    spaces(1);
+  }
+
+  /// Spaces.
+  void spaces(int num) {
+    var charsToReplace = countWhitespace();
+    addEdit(sourceIndex, charsToReplace, SPACES[num]);
+    advanceChars(charsToReplace);
+  }
+
+  wrongToken(String token) {
+    throw new FormatterException('expected token: "${token}", '
+                                 'actual: "${currentToken}"');
+  }
+
+  String toString() =>
+      new EditOperation().apply(editStore.edits,
+                                source.substring(0, sourceIndex));
+
 }
 
 const SPACE = ' ';
+final SPACES = [
+          '',
+          ' ',
+          '  ',
+          '   ',
+          '    ',
+          '     ',
+          '      ',
+          '       ',
+          '        ',
+          '         ',
+          '          ',
+          '           ',
+          '            ',
+          '             ',
+          '              ',
+          '               ',
+          '                ',
+];
 
+
 bool isIndentChar(String ch) => ch == SPACE; // TODO(pquitslund) also check tab
 
 
 /// Manages stored [Edit]s.
 class EditStore {
 
+  const EditStore();

[Brian Wilkerson, 2013/06/19 21:50:52]

I wouldn't make this a 'const' constructor because edit stores are mutable, but
perhaps I'm thinking of 'const' the wrong way.

[pquitslund, 2013/06/20 22:34:52]

Actually, I think I AM!  Honestly, this is an artifact of an attempt to make
this constructor callable from with a constructor initializer block.  Fixed now.

+
   /// The underlying sequence of [Edit]s.
   final edits = <Edit>[];
 
@@ -239,7 +411,6 @@
 }
 
 
-
 /// Describes a text edit.
 class Edit {
 
@@ -264,11 +435,156 @@
 
 }
 
+/// Applies a sequence of [edits] to a [document].
+class EditOperation {
+
+  String apply(List<Edit> edits, String document) {
+
+    var edit;
+    for (var i = edits.length - 1; i >= 0; --i) {
+      edit = edits[i];
+      document = replace(document, edit.offset,
+                         edit.offset + edit.length, edit.replacement);
+    }
+
+    return document;
+  }
+
+}
+
+
+String replace(String str, int start, int end, String replacement) =>
+    str.substring(0, start) + replacement + str.substring(end);
+
+
 /// An AST visitor that drives formatting heuristics.
-class FormattingEngine extends RecursiveASTVisitor<Void> {
+class FormattingEngine extends RecursiveASTVisitor {
 
   final FormatterOptions options;
 
+  CodeKind kind;
+  EditRecorder recorder;
+
   FormattingEngine(this.options);
 
+  String format(String source, ASTNode node, Token start, CodeKind kind,
+      EditRecorder recorder) {
+
+    this.kind = kind;
+    this.recorder = recorder;
+
+    recorder..source = source
+            ..currentToken = start;

[Brian Wilkerson, 2013/06/19 21:50:52]

This looks weird. I would have expected the state of the recorder to have been
configured before passing it to this method and for the 'source' and 'start'
arguments to be removed. (The reference to 'source' below would presumably be
replaced by 'recorder.source'.)

[pquitslund, 2013/06/20 22:34:52]

Agreed.  Still working on how this entry point should look.  Thanks!

+
+    node.accept(this);
+
+    var editor = new EditOperation();
+    return editor.apply(recorder.editStore.edits, source);
+  }
+
+
+  visitClassDeclaration(ClassDeclaration node) {
+
+    recorder.advanceIndent();
+
+    if (node.documentationComment != null) {
+      node.documentationComment.accept(this);
+    }
+
+    recorder..advance(node.classKeyword)..space();
+
+    node.name.accept(this);
+
+    if (node.typeParameters != null) {
+      node.typeParameters.accept(this);
+    }
+    recorder.space();
+
+    if (node.extendsClause != null) {
+      node.extendsClause.accept(this);
+      recorder.space();
+    }
+

[Brian Wilkerson, 2013/06/19 21:50:52]

You missed the withClause.

[pquitslund, 2013/06/20 22:34:52]

Coming real soon.  (Driven by accompanying tests.)

+    if (node.implementsClause != null) {
+      node.implementsClause.accept(this);
+      recorder.space();
+    }
+
+    recorder..advance(node.leftBracket)
+            ..indent();
+
+    for (var member in node.members) {
+      recorder.newline();

[Brian Wilkerson, 2013/06/19 21:50:52]

Do you want to advanceIndent() after the newline, or are you expecting every
member to do that before starting to write themselves out?

[pquitslund, 2013/06/20 22:34:52]

Actually, yes.  That is cleaner.  Thanks!

+      member.accept(this);
+    }
+
+    recorder..unindent()
+            ..newline()
+            ..advanceIndent()
+            ..advance(node.rightBracket);
+  }
+
+
+  visitBlockFunctionBody(BlockFunctionBody node) {
+    recorder..advance(node.beginToken)

[Brian Wilkerson, 2013/06/19 21:50:52]

The begin token for a BlockFunctionBody is the '{' for the block. I don't think
you want to advance over it here, but leave it for the block to do. (I don't
think you ever want to use 'beginToken' or 'endToken'; either there are more
specific getters or they belong to a child.)

+            ..indent()
+            ..newline();
+    node.block.accept(this);
+    recorder..unindent()
+            ..advanceIndent()
+            ..advance(node.endToken);
+  }
+
+
+  visitBlock(Block block) {
+
+  }
+
+
+  visitExpressionFunctionBody(ExpressionFunctionBody node) {
+    recorder..advance(node.beginToken)

[Brian Wilkerson, 2013/06/19 21:50:52]

Here too: use 'functionDefinition' and 'semicolon'.

[pquitslund, 2013/06/20 22:34:52]

Thanks!  

+            ..indent()
+            ..newline();
+    node.expression.accept(this);
+    recorder..unindent()
+            ..advanceIndent()
+            ..advance(node.endToken);
+  }
+
+
+  visitMethodDeclaration(MethodDeclaration node) {
+
+    recorder.advanceIndent();

[Brian Wilkerson, 2013/06/19 21:50:52]

The rule for AST nodes is that whitespace before and after the node belongs to
the parent. I think you want to follow something similar here: the whitespace
before and after a node should be written while visiting the parent of the node.
I think it will lead to fewer mistakes, because you'll have more context when
deciding what whitespace to write.

[pquitslund, 2013/06/20 22:34:52]

Done.

+
+    if (node.modifierKeyword != null) {
+      recorder.advance(node.modifierKeyword);
+      recorder.space();
+    }
+
+    if (node.returnType != null) {
+      recorder.advance(node.returnType.beginToken);

[Brian Wilkerson, 2013/06/19 21:50:52]

I think you want to visit the returnType at this point.

[pquitslund, 2013/06/20 22:34:52]

Done.

+      recorder.space();
+    }
+
+    recorder.advance(node.name.beginToken);
+
+    node.parameters.accept(this);
+
+    recorder.space();
+
+    node.body.accept(this);
+  }
+
+
+  visitFormalParameterList(FormalParameterList node) {
+    recorder.advance(node.beginToken);
+    //...
+    recorder.advance(node.endToken);
+  }
+
+
+  visitSimpleIdentifier(SimpleIdentifier node) {
+    recorder.advance(node.token);
+  }
+
 }