Make observable transform a barback transform.

pkg/observe/lib/transform.dart

Index: pkg/observe/lib/transform.dart
diff --git a/pkg/observe/lib/transform.dart b/pkg/observe/lib/transform.dart
new file mode 100644
index 0000000000000000000000000000000000000000..c7d3eb09654db6736c289d5cb2c27f437c1ef96a
--- /dev/null
+++ b/pkg/observe/lib/transform.dart
@@ -0,0 +1,335 @@
+// 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.
+
+/**
+ * Code transform for @observable. The core transformation is relatively
+ * straightforward, and essentially like an editor refactoring.
+ */
+library observe.transform;
+
+import 'dart:async';
+import 'package:path/path.dart' as path;
+import 'package:analyzer_experimental/src/generated/ast.dart';
+import 'package:analyzer_experimental/src/generated/error.dart';
+import 'package:analyzer_experimental/src/generated/parser.dart';
+import 'package:analyzer_experimental/src/generated/scanner.dart';
+import 'package:barback/barback.dart';
+import 'package:codegen/refactor.dart';
+import 'package:source_maps/span.dart' show SourceFile;
+
+/**
+ * A [Transformer] that replaces observables based on dirty-checking with an
+ * implementation based on change notifications. 
+ *
+ * The transformation adds hooks for field setters and notifies the observation
+ * system of the change.
+ */
+class ObservableTransformer extends Transformer {
+
+  Future<bool> isPrimary(Asset input) {
+    if (input.id.extension != '.dart') return new Future.value(false);
+    // Note: technically we should parse the file to find accurately the
+    // observable annotation, but that seems expensive. It would require almost
+    // as much work as applying the transform. We rather have some false
+    // positives here, and then generate no outputs when we apply this
+    // transform.

[Bob Nystrom, 2013/08/08 22:47:09]

You could always do this fast check first and then only do a real parse if it
passes to filter out false positives like "@observable" in a comment.

[Siggi Cherem (dart-lang), 2013/08/08 23:14:02]

true, but I feel is ok in that case to do the work during apply.

[nweiz, 2013/08/08 23:34:57]

There's no harm in running the transform on a file without any observable
annotations, though, is there? It should just spit out the original file.

[Siggi Cherem (dart-lang), 2013/08/09 00:29:54]

correct. Or in my case not spit anything =)

+    return input.readAsString().then((c) => c.contains("@observable"));
+  }
+
+  Future apply(Transform transform) {
+    return transform.primaryInput
+        .then((input) => input.readAsString()).then((content) {
+      var id = transform.primaryId;
+      var sourceFile = new SourceFile.text(id.toString(), content);

[nweiz, 2013/08/08 23:34:57]

Right now, [id.toString] will return a string of the form
"package_name|path/to/file.dart". This is a representation we've been using for
convenience internally, but it's not going to be understood well by code
expecting normal paths or URIs, nor is it guaranteed to remain in that format in
the future. It's probably safer to just pass [id.path] for now.

[Siggi Cherem (dart-lang), 2013/08/09 00:29:54]

I see. since the path doesn't have the package context, I would like this file
url to have some additional information. I changed this to reconstruct the
package: url if possible (if the path starts with lib/), otherwise to use
id.path.

Would it be possible for barback to expose a file url that works from the root
of the project?

[nweiz, 2013/08/09 00:53:48]

Not in general. Intermediate assets may have no on-disk representation, and thus
no valid file URL.

Right now, pub's implementation of PackageProvider ensures that all source
assets will come from a package's lib/ or asset/ directory. If we wanted to
encode that more directly into barback, it would be possible to always be able
to generate a "package:" or "asset:" URL for a given asset. That said, the
assets those URLs referred to could be an intermediate version of an asset that
gets transformed multiple times, and may thus not be outwardly accessible in the
form this transformer sees them.

+      var transaction = _transformCompilationUnit(
+          content, sourceFile, transform.log);
+      if (transaction.hasEdits) {

[Bob Nystrom, 2013/08/08 22:47:09]

I would do:

if (!transaction.hasEdits) return;

To get rid of a level of nesting.

[Siggi Cherem (dart-lang), 2013/08/08 23:10:41]

Done. Yeah, I like doing that too.

[nweiz, 2013/08/08 23:34:57]

If this returns without any outputs, that's not the same as passing the file
through unmodified. You'll need to do that explicitly if that's your desired
effect.

[Siggi Cherem (dart-lang), 2013/08/09 00:29:54]

I'm not sure I see what's the difference, so I'll drop shortly by to ask you
about it.

Basically what I want to indicate is that this asset shouldn't have been
considered a primary.

[Siggi Cherem (dart-lang), 2013/08/09 00:40:24]

Fixed. So it turns out that if we dont emit it, it gets 'consumed' and a future
phase of transformers wont see it. 
(Thanks for the clarification!)

+        var printer = transaction.commit();
+        var filename = path.basename(id.path);
+        printer.add('\n//# sourceMappingURL=$filename.map');
+        printer.build(id.path);
+        transform.addOutput(new Asset.fromString(id, printer.text));
+        transform.addOutput(
+            new Asset.fromString(id.addExtension('.map'), printer.map));

[nweiz, 2013/08/08 23:34:57]

This sourcemap is not going to consistently point to something real. Even if you
pass [id.path] to [new SourceFile.text], that path is only going to be valid at
the root level of whatever package this transform is being applied to. What's
more, it will be overwritten by this transform; once you transform an asset, the
previous version is no longer available.

The only way this would work is if you made the sourcemap point to a file URI
that identifies the original file on disk. However, barback doesn't expose that,
and there can be cases where this transform is pipelined and that file doesn't
exist anywhere at all on disk.

[Siggi Cherem (dart-lang), 2013/08/09 00:29:54]

Oh my! those are good points. I don't know yet what's the best solution. I
created this bug so we can brainstorm and track progress:
https://dartbug.com/12339

[nweiz, 2013/08/09 00:53:48]

I'd just get rid of the sourcemaps here for now until we get better barback
support for it.

[Siggi Cherem (dart-lang), 2013/08/09 01:17:30]

ok, no prob. I removed it for now. Our current use case only requires one
transform on .dart files, so it might be ok to add it back before barback has
full support for it.

+      }
+    });

[Bob Nystrom, 2013/08/08 22:47:09]

Nice!

[Siggi Cherem (dart-lang), 2013/08/08 23:10:41]

:)

+  }
+}
+
+void _transformCompilationUnit(

[Bob Nystrom, 2013/08/08 22:47:09]

Should this be void?

[Siggi Cherem (dart-lang), 2013/08/08 23:10:41]

oops, good catch, thanks.

+    String inputCode, SourceFile sourceFile, Log log) {
+  var unit = _parseCompilationUnit(inputCode);
+  return transformObservables(unit, sourceFile, inputCode, log);
+}
+
+// TODO(sigmund): make this private. This is currently public so it can be used
+// by the polymer.dart package which is not yet entirely migrated to use
+// pub-serve and pub-deploy.
+TextEditTransaction transformObservables(CompilationUnit unit,
+    SourceFile sourceFile, String content, Log log) {
+  var code = new TextEditTransaction(content, sourceFile);
+  for (var directive in unit.directives) {
+    if (directive is LibraryDirective && _hasObservable(directive)) {
+      log.warning('@observable on a library no longer has any effect. '
+          'It should be placed on individual fields.',
+          _getSpan(sourceFile, directive));
+      break;
+    }
+  }
+
+  for (var declaration in unit.declarations) {
+    if (declaration is ClassDeclaration) {
+      _transformClass(declaration, code, sourceFile, log);
+    } else if (declaration is TopLevelVariableDeclaration) {
+      if (_hasObservable(declaration)) {
+        log.warning('Top-level fields can no longer be observable. '
+            'Observable fields should be put in an observable objects.',
+            _getSpan(sourceFile, declaration));
+      }
+    }
+  }
+  return code;
+}
+
+/** Parse [code] using analyzer_experimental. */
+CompilationUnit _parseCompilationUnit(String code) {
+  var errorListener = new _ErrorCollector();
+  var scanner = new StringScanner(null, code, errorListener);
+  var token = scanner.tokenize();
+  var parser = new Parser(null, errorListener);
+  return parser.parseCompilationUnit(token);
+}
+
+class _ErrorCollector extends AnalysisErrorListener {
+  final errors = new List<AnalysisError>();

[Bob Nystrom, 2013/08/08 22:47:09]

final errors = <AnalysisError>[];

[Siggi Cherem (dart-lang), 2013/08/08 23:10:41]

Done.

+  onError(error) => errors.add(error);
+}
+
+_getSpan(SourceFile file, ASTNode node) => file.span(node.offset, node.end);
+
+/** True if the node has the `@observable` annotation. */
+bool _hasObservable(AnnotatedNode node) => _hasAnnotation(node, 'observable');
+
+bool _hasAnnotation(AnnotatedNode node, String name) {
+  // TODO(jmesserly): this isn't correct if the annotation has been imported
+  // with a prefix, or cases like that. We should technically be resolving, but
+  // that is expensive.
+  return node.metadata.any((m) => m.name.name == name &&
+      m.constructorName == null && m.arguments == null);
+}
+
+void _transformClass(ClassDeclaration cls, TextEditTransaction code,
+    SourceFile file, Log log) {
+
+  if (_hasObservable(cls)) {
+    log.warning('@observable on a class no longer has any effect. '
+        'It should be placed on individual fields.',
+        _getSpan(file, cls));
+  }
+
+  // We'd like to track whether observable was declared explicitly, otherwise
+  // report a warning later below. Because we don't have type analysis (only
+  // syntactic understanding of the code), we only report warnings that are
+  // known to be true.
+  var declaresObservable = false;
+  if (cls.extendsClause != null) {
+    var id = _getSimpleIdentifier(cls.extendsClause.superclass.name);
+    if (id.name == 'ObservableBase') {
+      code.edit(id.offset, id.end, 'ChangeNotifierBase');
+      declaresObservable = true;
+    } else if (id.name == 'ChangeNotifierBase') {
+      declaresObservable = true;
+    } else if (id.name != 'PolymerElement' && id.name != 'CustomElement'
+        && id.name != 'Object') {
+      // TODO(sigmund): this is conservative, consider using type-resolution to
+      // improve this check.
+      declaresObservable = true;
+    }
+  }
+
+  if (cls.withClause != null) {
+    for (var type in cls.withClause.mixinTypes) {
+      var id = _getSimpleIdentifier(type.name);
+      if (id.name == 'ObservableMixin') {
+        code.edit(id.offset, id.end, 'ChangeNotifierMixin');
+        declaresObservable = true;
+        break;
+      } else if (id.name == 'ChangeNotifierMixin') {
+        declaresObservable = true;
+        break;
+      } else {
+        // TODO(sigmund): this is conservative, consider using type-resolution
+        // to improve this check.
+        declaresObservable = true;
+      }
+    }
+  }
+
+  if (!declaresObservable && cls.implementsClause != null) {
+    // TODO(sigmund): consider adding type-resolution to give a more precise
+    // answer.
+    declaresObservable = true;
+  }
+
+  // Track fields that were transformed.
+  var instanceFields = new Set<String>();
+  var getters = new List<String>();
+  var setters = new List<String>();
+
+  for (var member in cls.members) {
+    if (member is FieldDeclaration) {
+      bool isStatic = _hasKeyword(member.keyword, Keyword.STATIC);
+      if (isStatic) {
+        if (_hasObservable(member)){
+          log.warning('Static fields can no longer be observable. '
+              'Observable fields should be put in an observable objects.',
+              _getSpan(file, member));
+        }
+        continue;
+      }
+      if (_hasObservable(member)) {
+        if (!declaresObservable) {
+          log.warning('Observable fields should be put in an observable'
+              ' objects. Please declare that this class extends from '
+              'ObservableBase, includes ObservableMixin, or implements '
+              'Observable.',
+              _getSpan(file, member));
+
+        }
+        _transformFields(member.fields, code, member.offset, member.end);
+
+        var names = member.fields.variables.map((v) => v.name.name);
+
+        getters.addAll(names);
+        if (!_isReadOnly(member.fields)) {
+          setters.addAll(names);
+          instanceFields.addAll(names);
+        }
+      }
+    }
+    // TODO(jmesserly): this is a temporary workaround until we can remove
+    // getValueWorkaround and setValueWorkaround.
+    if (member is MethodDeclaration) {
+      if (_hasKeyword(member.propertyKeyword, Keyword.GET)) {
+        getters.add(member.name.name);
+      } else if (_hasKeyword(member.propertyKeyword, Keyword.SET)) {
+        setters.add(member.name.name);
+      }
+    }
+  }
+
+  // If nothing was @observable, bail.
+  if (instanceFields.length == 0) return;
+
+  // Fix initializers, because they aren't allowed to call the setter.
+  for (var member in cls.members) {
+    if (member is ConstructorDeclaration) {
+      _fixConstructor(member, code, instanceFields);
+    }
+  }
+}
+
+SimpleIdentifier _getSimpleIdentifier(Identifier id) =>
+    id is PrefixedIdentifier ? (id as PrefixedIdentifier).identifier : id;
+
+
+bool _hasKeyword(Token token, Keyword keyword) =>
+    token is KeywordToken && (token as KeywordToken).keyword == keyword;
+
+String _getOriginalCode(TextEditTransaction code, ASTNode node) =>
+    code.original.substring(node.offset, node.end);
+
+void _fixConstructor(ConstructorDeclaration ctor, TextEditTransaction code,
+    Set<String> changedFields) {
+
+  // Fix normal initializers
+  for (var initializer in ctor.initializers) {
+    if (initializer is ConstructorFieldInitializer) {
+      var field = initializer.fieldName;
+      if (changedFields.contains(field.name)) {
+        code.edit(field.offset, field.end, '__\$${field.name}');
+      }
+    }
+  }
+
+  // Fix "this." initializer in parameter list. These are tricky:
+  // we need to preserve the name and add an initializer.
+  // Preserving the name is important for named args, and for dartdoc.
+  // BEFORE: Foo(this.bar, this.baz) { ... }
+  // AFTER:  Foo(bar, baz) : __$bar = bar, __$baz = baz { ... }
+
+  var thisInit = [];
+  for (var param in ctor.parameters.parameters) {
+    if (param is DefaultFormalParameter) {
+      param = param.parameter;
+    }
+    if (param is FieldFormalParameter) {
+      var name = param.identifier.name;
+      if (changedFields.contains(name)) {
+        thisInit.add(name);
+        // Remove "this." but keep everything else.
+        code.edit(param.thisToken.offset, param.period.end, '');
+      }
+    }
+  }
+
+  if (thisInit.length == 0) return;
+
+  // TODO(jmesserly): smarter formatting with indent, etc.
+  var inserted = thisInit.map((i) => '__\$$i = $i').join(', ');
+
+  int offset;
+  if (ctor.separator != null) {
+    offset = ctor.separator.end;
+    inserted = ' $inserted,';
+  } else {
+    offset = ctor.parameters.end;
+    inserted = ' : $inserted';
+  }
+
+  code.edit(offset, offset, inserted);
+}
+
+bool _isReadOnly(VariableDeclarationList fields) {
+  return _hasKeyword(fields.keyword, Keyword.CONST) ||
+      _hasKeyword(fields.keyword, Keyword.FINAL);
+}
+
+void _transformFields(VariableDeclarationList fields, TextEditTransaction code,
+    int begin, int end) {
+
+  if (_isReadOnly(fields)) return;
+
+  var indent = guessIndent(code.original, begin);
+  var replace = new StringBuffer();
+
+  // Unfortunately "var" doesn't work in all positions where type annotations
+  // are allowed, such as "var get name". So we use "dynamic" instead.
+  var type = 'dynamic';
+  if (fields.type != null) {
+    type = _getOriginalCode(code, fields.type);
+  }
+
+  for (var field in fields.variables) {
+    var initializer = '';
+    if (field.initializer != null) {
+      initializer = ' = ${_getOriginalCode(code, field.initializer)}';
+    }
+
+    var name = field.name.name;
+
+    // TODO(jmesserly): should we generate this one one line, so source maps
+    // don't break?
+    if (replace.length > 0) replace.write('\n\n$indent');
+    replace.write('''
+$type __\$$name$initializer;
+$type get $name => __\$$name;
+set $name($type value) {
+  __\$$name = notifyPropertyChange(const Symbol('$name'), __\$$name, value);
+}
+'''.replaceAll('\n', '\n$indent'));
+  }
+
+  code.edit(begin, end, '$replace');
+}