Dart formatter babysteps.

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 0)
+++ pkg/analyzer_experimental/lib/src/services/formatter_impl.dart	(revision 0)
@@ -0,0 +1,291 @@
+// Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+library formatter_impl;
+
+
+import 'dart:io';

[Bob Nystrom, 2013/06/06 22:23:06]

Nit, but Nathan and I put organize our imports like:

1. "dart:" imports.
2. Blank line.
3. "package:" imports.
4. Blank line.
5. Relative imports.

[pquitslund, 2013/06/07 19:45:01]

Done.

+import 'package:analyzer_experimental/src/generated/scanner.dart';
+import 'package:analyzer_experimental/analyzer.dart';
+import 'package:analyzer_experimental/src/generated/parser.dart';
+import 'package:analyzer_experimental/src/generated/source.dart';

[Bob Nystrom, 2013/06/06 22:23:06]

These are fine, but you can also use relative imports for these if you prefer.

[pquitslund, 2013/06/07 19:45:01]

I thought this would lead to woes if I ever wanted to debug in dartium?  I
vaguely recall that this was the crux of some issues were seeing with stepping. 
Again: I'm probably mis-remembering.  Or it's been since fixed!

[Bob Nystrom, 2013/06/10 21:15:46]

It's very confusing, unfortunately. The basic rule is you can use relative
imports from stuff under lib/ to stuff under that same lib/ directory. Anything
that reaches out of or into a lib/ directory should use package:.

+
+
+/// OS line separator. (TODO: may not be necessary)

[messick, 2013/06/07 13:51:36]

TODO(ldap) everywhere? It probably seems redundant but I think many team members
expect to see the ldap.

[pquitslund, 2013/06/07 19:45:01]

Done.

+const String NEW_LINE = '\n' ; //Platform.pathSeparator;

[Bob Nystrom, 2013/06/06 22:23:06]

Ditch the type annotation.

[pquitslund, 2013/06/07 19:45:01]

Oh my.  This is taking some getting used to. :)  Done!

+
+/// Formatter options.
+class FormatterOptions {
+
+  /// Create formatter options with defaults derived (where defined) from
+  /// the style guide: <http://www.dartlang.org/articles/style-guide/>.
+  const FormatterOptions({this.initialIndentationLevel:0,

[Bob Nystrom, 2013/06/06 22:23:06]

Space after ":".

[pquitslund, 2013/06/07 19:45:01]

Done.  If only I had a formatter to do this for me!

+                 this.indentPerLevel:2,
+                 this.lineSeparator:NEW_LINE,
+                 this.pageWidth:80,
+                 this.tabSize:2});
+
+  final String lineSeparator;
+  final int initialIndentationLevel;
+  final int indentPerLevel;
+  final int tabSize;
+  final int pageWidth;
+}
+
+
+/// Thrown when an error occurs in formatting.
+class FormatterException implements Exception {
+
+  /// A message describing the error.
+  final String message;
+
+  /// Creates a new FormatterException with an optional error [message].
+  const FormatterException([this.message = '']);
+
+  FormatterException.forError(List<AnalysisError> errors) :
+    // TODO: add descriptive message based on errors
+    message = 'an analysis error occured during format';
+
+  String toString() => 'FormatterException: $message';
+
+}
+
+/// Specifies the kind of code snippet to format.
+class CodeKind {
+
+  final int index;
+
+  const CodeKind(this.index);
+
+  /// A compilation unit snippet.
+  static const COMPILATION_UNIT = const CodeKind(0);
+
+  /// A statement snippet.
+  static const STATEMENT = const CodeKind(1);
+
+}
+
+/// Dart source code formatter.
+abstract class CodeFormatter {

[messick, 2013/06/07 13:51:36]

I'm not clear what your plan is but I wasn't expecting to see an abstract
formatter class.

[pquitslund, 2013/06/07 19:45:01]

See response to Bob below.

+
+  factory CodeFormatter([FormatterOptions options = const FormatterOptions()])
+                        => new CodeFormatterImpl(options);
+
+  /// Format the specified portion (from [offset] with [length]) of the given
+  /// [source] string, optionally providing an [indentationLevel].
+  String format(CodeKind kind, String source, {int offset, int end,
+    int indentationLevel:0});

[Bob Nystrom, 2013/06/06 22:23:06]

Indent a line continuation +4, not +2.

[pquitslund, 2013/06/07 19:45:01]

Done.

+
+}
+
+class CodeFormatterImpl implements CodeFormatter, AnalysisErrorListener {

[Bob Nystrom, 2013/06/06 22:23:06]

Unify this with CodeFormatter. Simpler = better. :)

[pquitslund, 2013/06/07 19:45:01]

OK.  I *might*.  My thinking here is that I really want the API surface area of
the CodeFormatter consumed by the public to be concise.  As such I don't want
them seeing implementation details (e.g., implements AnalysisErrorListener and
so on).

This might be unnecessary but at least for now I want to lean this way.  But
feel free and try and unconvince me!

[Brian Wilkerson, 2013/06/07 19:55:11]

Another alternative would be to not implement AnalysisErrorListener, since it's
not really how you want a code formatter to be used, and to have a separate
object that gathers errors for you. There is a RecordingErrorListener in the
resolver and a GatheringErrorListener in the test framework (you couldn't use it
directly, but you could take it as a starting point for implementing your own).

+
+  final FormatterOptions options;
+  final List<AnalysisError> errors;
+
+  CodeFormatterImpl(this.options) : errors = new List<AnalysisError>();
+
+  String format(CodeKind kind, String source, {int offset, int end,
+    int indentationLevel:0}) {
+
+    var start = tokenize(source);
+    _checkForErrors();
+
+    var node = parse(kind, start);
+    _checkForErrors();
+
+    // To be continued...
+
+    return source;
+  }
+
+  ASTNode parse(CodeKind kind, Token start) {

[Bob Nystrom, 2013/06/06 22:23:06]

The style guide would name this "AstNode".

[messick, 2013/06/07 13:51:36]

We should talk to Konstantin to see if we can get ASTNode renamed. I wonder how
many people are depending on it already...

[pquitslund, 2013/06/07 19:45:01]

Aha.  Something for Konstantin (or Brian).

Thanks!

[pquitslund, 2013/06/07 19:45:01]

Good point!

+
+    var parser = new Parser(null, this);
+
+    switch (kind) {
+      case CodeKind.COMPILATION_UNIT:
+        return parser.parseCompilationUnit(start);
+      case CodeKind.STATEMENT:
+        return parser.parseStatement(start);

[messick, 2013/06/07 13:51:36]

At a guess, I'd expect refactoring to want a few more of these: directive list,
statement list (or block), expression.

[pquitslund, 2013/06/07 19:45:01]

Absolutely.  Need to crawl before I can walk though! :)

+    }
+
+    throw new FormatterException('Unsupported format kind: $kind');
+  }
+
+  _checkForErrors() {
+    if (errors.length > 0) {
+      throw new FormatterException.forError(errors);
+    }
+  }
+
+  void onError(AnalysisError error){

[Bob Nystrom, 2013/06/06 22:23:06]

Space before "{".

[pquitslund, 2013/06/07 19:45:01]

Eagle eye.  Fixed!

+    errors.add(error);
+  }
+
+  Token tokenize(String source) {
+    var scanner = new StringScanner(null, source, this);
+    return scanner.tokenize();

[Brian Wilkerson, 2013/06/06 22:21:58]

I noticed that you're testing for new-lines in the EditRecorder. You might want
to capture the line information from the scanner because this information has
probably been computed for you.

[pquitslund, 2013/06/07 19:45:01]

Cool.  Great tip.  I'll look into that.  Thanks!

+  }
+
+}
+
+/// Placeholder class to hold a reference to the Class object representing
+/// the Dart keyword void.
+class Void extends Object {

[messick, 2013/06/07 13:51:36]

I hope you'll be able to use Keyword.VOID instead of this.

[pquitslund, 2013/06/07 19:45:01]

The issue is that I really like to use VOID as a type parameter when
implementing visitors.  Maybe I'm missing something but I'm not sure how how
Keyword.VOID would work for that.  Or?

+
+}
+
+
+/// Records a sequence of edits to a source string that will cause the string
+/// to be formatted when applied.
+class EditRecorder {
+
+  final FormatterOptions options;
+
+  int column = 0;
+
+  int sourceIndex = 0;
+  String source = '';
+
+  Token currentToken;
+
+  int indentationLevel = 0;
+  int numberOfIndentations = 0;
+
+  bool isIndentNeeded = false;
+
+  EditRecorder(this.options);
+
+  /// Count the number of whitespace chars beginning at the current
+  /// [sourceIndex].
+  int countWhitespace() {
+    var count = 0;
+    for (var i = sourceIndex; i < source.length; ++i) {
+      if (isIndentChar(source[i])) {
+        ++count;
+      } else {
+        break;
+      }
+    }
+    return count;
+  }
+
+  /// Indent.
+  void indent() {
+    indentationLevel += options.indentPerLevel;

[messick, 2013/06/07 13:51:36]

These names are confusing. What's a "level"?
indentationLevel
indentPerLevel

[pquitslund, 2013/06/07 19:45:01]

Working on it.  Thanks!

+    numberOfIndentations++;
+  }
+
+  /// Test if there is a newline at the given source [index].
+  bool isNewlineAt(int index) {
+    if (index < 0 || index + NEW_LINE.length > source.length) {
+      return false;
+    }
+    for (var i = 0; i < NEW_LINE.length; i++) {

[Bob Nystrom, 2013/06/06 22:23:06]

Do you need to increment index here too?

[pquitslund, 2013/06/07 19:45:01]

Time for some more tests! :)

+      if (source[index] != NEW_LINE[i]) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+}
+
+final String SPACE = ' ';

[Bob Nystrom, 2013/06/06 22:23:06]

"final String" -> "const".

[pquitslund, 2013/06/07 19:45:01]

Done.

+
+bool isIndentChar(String ch) => ch == SPACE; // TODO(pquitslund) also check tab
+
+
+/// Manages stored [Edit]s.
+abstract class EditStore {

[Brian Wilkerson, 2013/06/06 22:21:58]

Edits and edit stores sound like a re-usable piece that should be in their own
library.

[messick, 2013/06/07 13:51:36]

I think so, too.

+
+  /// 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);
+
+  /// Get the index of the current edit (for use in caching location
+  /// information).
+  int getCurrentEditIndex();
+
+  /// Get the underlying sequence of [Edit]s.
+  List<Edit> get edits;
+
+  /// Get the last edit.
+  Edit getLastEdit();

[Bob Nystrom, 2013/06/06 22:23:06]

You'll already have edits.last, so I'd ditch this.

[pquitslund, 2013/06/07 19:45:01]

Right but for some reason I convinced myself that I'd like to return null if the
edit list is empty (rather than throwing an exception). I *think* this makes it
easier on callers.

[Bob Nystrom, 2013/06/10 21:15:46]

They could just do edits.isEmpty to check for that case.

+
+  /// Add an [Edit] that describes an insertion of text starting at the given
+  /// [offset].
+  void insert(int offset, String insertedString);
+
+  /// Reset cached state.
+  void reset();
+
+}
+
+ /// Basic un-optimized [EditStore] suitable for subclassing.
+ class BasicEditStore implements EditStore {

[Bob Nystrom, 2013/06/06 22:23:06]

You can unify this with EditStore. If users don't want to inherit its concrete
implementation, they can always just implement instead of extending it.

[pquitslund, 2013/06/07 19:45:01]

Ah yes.  Good point!

Done.

+
+  final List<Edit> edits = new List<Edit>();

[Bob Nystrom, 2013/06/06 22:23:06]

final edits = <Edit>[];

[pquitslund, 2013/06/07 19:45:01]

Done.

+
+  /// Add the given [Edit] to the end of the edit sequence.
+  void add(Edit edit) {
+    edits.add(edit);
+  }
+
+  void addEdit(int offset, int length, String replacement) {
+    add(new Edit(offset, length, replacement));
+  }
+
+  //TODO(pquitslund): verify that this should be "last" vs. "next"

[messick, 2013/06/07 13:51:36]

I wonder if this comment is in the right place?

[pquitslund, 2013/06/07 19:45:01]

Done.

+  int getCurrentEditIndex() =>  edits.length - 1;
+
+  Edit getLastEdit() => edits.isEmpty ? null : edits.last;
+
+  void insert(int offset, String insertedString) {
+    addEdit(offset, 0, insertedString);
+  }
+
+  void reset() {
+    edits.clear();
+  }
+
+  String toString() => 'EditStore( ${edits.toString()} )';
+
+ }

[Bob Nystrom, 2013/06/06 22:23:06]

Unindent.

[pquitslund, 2013/06/07 19:45:01]

Nothing escapes you!

+
+
+
+/// Describes a text edit.
+class Edit {
+
+  /// The offset at which to apply the edit.
+  final int offset;
+
+  /// The length of the text interval to replace.
+  final int length;
+
+  /// The replacement text.
+  final String replacement;
+
+  /// Create an edit.
+  const Edit(this.offset, this.length, this.replacement);
+
+  /// Create an edit for the given [range].
+  Edit.forRange(SourceRange range, String replacement):
+    this(range.offset, range.length, replacement);
+
+  String toString() => '${offset < 0 ? '(' : 'X('} offset: ${offset} , '
+                         'length ${length}, replacement :> ${replacement} <:)';
+
+}
+
+/// An AST visitor that drives formatting heuristics.
+class FormattingEngine extends RecursiveASTVisitor<Void> {
+
+  final FormatterOptions options;
+
+  FormattingEngine(this.options);
+
+}