Enables codegen support in polymer

pkg/polymer/lib/src/build/script_compactor.dart

 // 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.
 
 /// Transfomer that combines multiple dart script tags into a single one.
 library polymer.src.build.script_compactor;
 
 import 'dart:async';
 import 'dart:convert';
 
-import 'package:html5lib/dom.dart' show Document, Element;
+import 'package:html5lib/dom.dart' show Document, Element, Text;
+import 'package:html5lib/dom_parsing.dart';
 import 'package:analyzer/src/generated/ast.dart';
+import 'package:analyzer/src/generated/element.dart' hide Element;
+import 'package:analyzer/src/generated/element.dart' as analyzer show Element;
 import 'package:barback/barback.dart';
 import 'package:code_transformers/assets.dart';
 import 'package:path/path.dart' as path;
 import 'package:source_maps/span.dart' show SourceFile;
+import 'package:smoke/codegen/generator.dart';
+import 'package:smoke/codegen/recorder.dart';
+import 'package:code_transformers/resolver.dart';
+import 'package:code_transformers/src/dart_sdk.dart';
+
+import 'package:polymer_expressions/expression.dart' as pe;
+import 'package:polymer_expressions/parser.dart' as pe;
+import 'package:polymer_expressions/visitor.dart' as pe;
 
 import 'import_inliner.dart' show ImportInliner; // just for docs.
 import 'common.dart';
 
 /// Combines Dart script tags into a single script tag, and creates a new Dart
 /// file that calls the main function of each of the original script tags.
 ///
 /// This transformer assumes that all script tags point to external files. To
 /// support script tags with inlined code, use this transformer after running
 /// [ImportInliner] on an earlier phase.
 ///
 /// Internally, this transformer will convert each script tag into an import
 /// statement to a library, and then uses `initPolymer` (see polymer.dart)  to
 /// process `@initMethod` and `@CustomTag` annotations in those libraries.
 class ScriptCompactor extends Transformer {
+  final Resolvers resolvers;

[Jennifer Messerly, 2014/03/27 02:20:32]

this is an issue with the code_transformer pkg, but ... as a general rule it's
confusing to have a type named "Resolvers" and "Resolver" in the same system.
What do you call a list of Resolvers? And which type was I just talking about?
Exactly. ;-)

(not sure if I first saw this from the .NET design guidelines or something much
older, but it's a good rule to follow...)
 

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

Yeah, that's a good point.

Just chatted with Pete and we had some ideas. One idea is to rename it to
ResolverProvider, ResolverFactory, or ResolverCache.

Meanwhile, I filed: https://code.google.com/p/dart/issues/detail?id=17864

   final TransformOptions options;
 
-  ScriptCompactor(this.options);
+  ScriptCompactor(this.options, {String sdkDir})
+      : resolvers = new Resolvers(sdkDir != null ? sdkDir : dartSdkDirectory);
 
   /// Only run on entry point .html files.
   Future<bool> isPrimary(Asset input) =>
       new Future.value(options.isHtmlEntryPoint(input.id));
 
   Future apply(Transform transform) =>
-      new _ScriptCompactor(transform, options).apply();
+      new _ScriptCompactor(transform, options, resolvers).apply();
 }
 
 /// Helper class mainly use to flatten the async code.
 class _ScriptCompactor extends PolymerTransformer {
   final TransformOptions options;
   final Transform transform;
   final TransformLogger logger;
   final AssetId docId;
   final AssetId bootstrapId;
 
+  /// HTML document parsed from [docId].
   Document document;
+
+  /// List of ids for each Dart entry script tag (the main tag and any tag
+  /// included on each custom element definition).
   List<AssetId> entryLibraries;
+
+  /// The id of the main Dart program.
   AssetId mainLibraryId;
+
+  /// Script tag that loads the Dart entry point.
   Element mainScriptTag;
-  final Map<AssetId, List<_Initializer>> initializers = {};
 
-  _ScriptCompactor(Transform transform, this.options)
+  /// Initializers that will register custom tags or invoke `initMethod`s.
+  final List<_Initializer> initializers = [];
+
+  /// Attributes published on a custom-tag. We make these available via
+  /// reflection even if @published was not used.
+  final Map<String, List<String>> publishedAttributes = {};
+
+  /// Those custom-tags for which we have found a corresponding class in code.
+  final Set<String> tagsWithClasses = new Set();
+
+  /// Hook needed to access the analyzer within barback transformers.
+  final Resolvers resolvers;
+
+  /// The resolver instance associated with a single run of this transformer.
+  Resolver resolver;
+
+  /// Element representing `HtmlElement`.
+  ClassElement _htmlElementElement;

[Jennifer Messerly, 2014/03/27 02:20:32]

A thought here: there's a lot of state in this class. it went from ~10 fields to
~22 ...  and much of it is mutable. I wonder if some of it could be collected
into other objects?

an idea: maybe all of these constant types could be pulled out into a class

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

good idea. I moved these out in to a separate class, and inlined a couple that
didn't need to be a field. I thought at some point we might want to split out
the analysis of Dart code into a separate class or library, but we can do that
as a follow up later.

+
+  /// Element representing the constructor of `@CustomTag`.
+  ConstructorElement _customTagConstructor;
+
+  /// Element representing the type of `@published`.
+  ClassElement _publishedElement;
+
+  /// Element representing the type of `@observable`.
+  ClassElement _observableElement;
+
+  /// Element representing the type of `@ObserveProperty`.
+  ClassElement _observePropertyElement;
+
+  /// Element representing the `@initMethod` annotation.
+  TopLevelVariableElement _initMethodElement;
+
+  /// Code generator used to create the static initialization for smoke.
+  final SmokeCodeGenerator generator = new SmokeCodeGenerator();

[Jennifer Messerly, 2014/03/27 02:20:32]

IMO, it's nice to not repeat the type annotation for final fields, if
possible...

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

Done.

It seems fine in this particular example, but I'm not convinced about making
this a general rule. Here it seems fine because we only use the field within
this file, but for a public API I think keeping the annotation can improve
documentation and type checking (if someone were to use the class as an
interface, for example.)

+
+  /// Recorder that uses analyzer data to feed data to [generator].
+  Recorder recorder;
+
+  _ScriptCompactor(Transform transform, this.options, this.resolvers)
       : transform = transform,
         logger = transform.logger,
         docId = transform.primaryInput.id,
         bootstrapId = transform.primaryInput.id.addExtension('_bootstrap.dart');
 
   Future apply() =>
       _loadDocument()
       .then(_loadEntryLibraries)
       .then(_processHtml)
       .then(_emitNewEntrypoint);
@@ skipping 44 matching lines @@
   Future _emitNewEntrypoint(_) {
     if (mainScriptTag == null) {
       // We didn't find any main library, nothing to do.
       transform.addOutput(transform.primaryInput);
       return null;
     }
 
     // Emit the bootstrap .dart file
     mainScriptTag.attributes['src'] = path.url.basename(bootstrapId.path);
     entryLibraries.add(mainLibraryId);
-    return _computeInitializers().then(_createBootstrapCode).then((code) {
-      transform.addOutput(new Asset.fromString(bootstrapId, code));
-      transform.addOutput(new Asset.fromString(docId, document.outerHtml));
+
+    return _initResolver()
+        .then(_extractUsesOfMirrors)
+        .then(_emitFiles)
+        .then((_) => resolver.release());
+  }
+
+  /// Load a resolver that computes information for every library in
+  /// [entryLibraries], then use it to initialize the [recorder] (for import
+  /// resolution) and to resolve specific elements (for analyzing the user's
+  /// code).
+  Future _initResolver() => resolvers.get(transform, entryLibraries).then((r) {
+    resolver = r;
+    recorder = new Recorder(generator,
+        (lib) => resolver.getImportUri(lib, from: bootstrapId).toString());
+
+    // Load class elements that are used in queries for codegen.
+    var polymerLib = r.getLibrary(new AssetId('polymer', 'lib/polymer.dart'));

[Jennifer Messerly, 2014/03/27 02:20:32]

just curious, any reason to prefer AssetIds over package: URIs or vice versa?

(seems like there's two very similar ways to query for libraries)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

code_transformers indexes the libraries that are reachable from the program
using the asset-ids, so lookup by asset id is more efficient. Lookup-by-uri has
to search all of the assets to find which one matches the import uri. We only
use it for core libraries that can't be indexed with asset-ids.

I filed a bug to consider adding a secondary index by uri, though
(https://code.google.com/p/dart/issues/detail?id=17865)

Funny enough, there is not just 2, but 3 ways to query for libraries! (you can
query them by name too :-))

+    if (polymerLib == null) _definitionError('the polymer library');
+    var htmlLib = r.getLibraryByUri(Uri.parse('dart:html'));
+    if (htmlLib == null) _definitionError('the "dart:html" library');
+    var observeLib = r.getLibrary(
+        new AssetId('observe', 'lib/src/metadata.dart'));
+    if (observeLib == null) _definitionError('the observe library');
+
+    for (var unit in polymerLib.parts) {
+      if (unit.uri == 'src/loader.dart') {
+        _initMethodElement = unit.topLevelVariables.firstWhere(
+            (t) => t.displayName == 'initMethod');
+        break;
+      }
+    }
+    _customTagConstructor =
+        _lookupType(polymerLib, 'CustomTag').constructors.first;
+    _publishedElement = _lookupType(polymerLib, 'PublishedProperty');
+    _observableElement = _lookupType(observeLib, 'ObservableProperty');
+    _observePropertyElement = _lookupType(polymerLib, 'ObserveProperty');
+    _htmlElementElement = _lookupType(htmlLib, 'HtmlElement');
+    if (_initMethodElement == null) _definitionError('@initMethod');
+  });
+
+  _lookupType(LibraryElement lib, String typeName) {
+    var result = lib.getType(typeName);
+    if (result == null) _definitionError(typeName);
+    return result;
+  }
+
+  _definitionError(name) {
+    throw new StateError("Internal error in polymer-builder: couldn't find "

[Jennifer Messerly, 2014/03/27 02:20:32]

could this error be cased by the user forgetting to import polymer anywhere? Or
is it firmly in the "should never happen" category?

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

It was happening in our tests before I added the mock libraries, but other than
that it shouldn't happen in user's code. If this happens, it means they have not
imported polymer, not even to call the initialization. 

+        "definition of $name.");
+  }
+
+  /// Inspects the entire program to find out anything that polymer accesses
+  /// using mirrors and produces static information that can be used to replace
+  /// the mirror-based loader and the uses of mirrors through the `smoke`
+  /// package. This includes:
+  ///
+  ///   * visiting entry-libraries to extract initializers,
+  ///   * visiting polymer-expressions to extract getters and setters,
+  ///   * looking for published fields of custom elements, and
+  ///   * looking for event handlers and callbacks of change notifications.
+  ///
+  void _extractUsesOfMirrors(_) {

[Jennifer Messerly, 2014/03/27 02:20:32]

Just wanted to say this method is awesome. It reads like psuedocode. :)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

:)

+    // Generate getters and setters needed to evaluate polymer expressions, and
+    // extract information about published attributes.
+    new _HtmlExtractor(generator, publishedAttributes).visit(document);
+
+    // Process all classes and top-level functions to include initializers,
+    // register custom elements, and include special fields and methods in
+    // custom element classes.
+    for (var id in entryLibraries) {
+      var lib = resolver.getLibrary(id);
+      for (var cls in _visibleClassesOf(lib)) {
+        _processClass(cls, id);
+      }
+
+      for (var fun in _visibleTopLevelMethodsOf(lib)) {
+        _processFunction(fun, id);
+      }
+    }
+
+    // Warn about tagNames with no corresponding Dart class
+    // TODO(sigmund): is there a way to exclude polymer.js elements? should we
+    // remove the warning below?

[Jennifer Messerly, 2014/03/27 02:20:32]

yes, please, let's remove :)

(even for Dart, things can be document.register'd ... more comment below)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

Done.

+    publishedAttributes.forEach((tagName, attrs) {
+      if (tagsWithClasses.contains(tagName)) return;
+      logger.warning('Class for custom-element "$tagName" not found. '
+        'Code-generation might be incomplete.');
+      // We include accessors for the missing attributes because they help get
+      // better warning messages at runtime.
+      for (var attr in attrs) {
+        generator.addGetter(attr);
+        generator.addSetter(attr);
+        generator.addSymbol(attr);
+      }
     });
   }
 
-  /// Emits the actual bootstrap code.
-  String _createBootstrapCode(_) {
+  /// Retrieves all classses that are visible if you were to import [lib]. This
+  /// includes exported classes from other libraries.
+  List<ClassElement> _visibleClassesOf(LibraryElement lib) {
+    var result = [];
+    result.addAll(lib.units.expand((u) => u.types));
+    for (var e in lib.exports) {
+      var exported = e.exportedLibrary.units.expand((u) => u.types).toList();
+      _filter(exported, e.combinators);
+      result.addAll(exported);
+    }
+    return result;
+  }
+
+  /// Retrieves all top-level methods that are visible if you were to import
+  /// [lib]. This includes exported methods from other libraries too.
+  List<ClassElement> _visibleTopLevelMethodsOf(LibraryElement lib) {
+    var result = [];
+    result.addAll(lib.units.expand((u) => u.functions));
+    for (var e in lib.exports) {
+      var exported = e.exportedLibrary.units
+          .expand((u) => u.functions).toList();
+      _filter(exported, e.combinators);
+      result.addAll(exported);
+    }
+    return result;
+  }
+
+  /// Filters [elements] that come from an export, according to its show/hide
+  /// combinators. This modifies [elements] in place.
+  void _filter(List<analyzer.Element> elements,
+      List<NamespaceCombinator> combinators) {
+    for (var c in combinators) {
+      if (c is ShowElementCombinator) {
+        var show = c.shownNames.toSet();
+        elements.retainWhere((e) => show.contains(e.displayName));
+      } else if (c is HideElementCombinator) {
+        var hide = c.hiddenNames.toSet();
+        elements.removeWhere((e) => hide.contains(e.displayName));
+      }
+    }
+  }
+
+  /// Process a class ([cls]). If it contains an appropriate [CustomTag]

[Jennifer Messerly, 2014/03/27 02:20:32]

so, today I can have a Dart class, mark it @reflectable (or @MirrorsUsed) and
then document.register it sometime later. This is great for things like deferred
libraries or just generally the app wants to lazy load things.

It seems like this new world requires @CustomTag.

Since the features are all Polymer features, would it be possible instead to
check for the Polymer mixin? That should be a 100% reliable signal that it is a
Polymer element & has the framework features. 

(Another issue: @CustomTag is currently implemented by Polymer, but I think we
want it to be a more general feature someday if possible...)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

This is a great idea! I made the switch.

The only thing missing is what to do for codegen when people have the following:
 - they use 'attributes' in the html
 - they manually call Polymer.register
 - they do not include @published

Here is where I think @UnregisteredCustomTag could be useful, or we can tell
them just to put the @published. I'm ok to start with the latter but we might
want to still have the former too. Thoughts?

I updated the bug with these details too.

[Jennifer Messerly, 2014/03/28 21:48:35]

ah, yeah that makes sense. seems like several problems all tangled up, including
the "attributes= vs @published", the fact that @CustomTag is current polymer
specific (but probably shouldn't be), @CustomTag not being able to register the
same type for multiple tag names, reflectability vs @published ... etc :). I
like where this ended up tho but agree we should have another round of design
here

+  /// annotation, we include an initializer to register this class, and make
+  /// sure to include everything that might be accessed or queried from them
+  /// using the smoke package. In particular, polymer uses smoke for the
+  /// following:
+  ///    * invoke #registerCallback on custom elements classes, if present.
+  ///    * query for methods ending in `*Changed`.
+  ///    * query for methods with the `@ObserveProperty` annotation.
+  ///    * query for non-final properties labeled with `@published`.
+  ///    * read declarations of properties named in the `attributes` attribute.
+  ///    * read/write the value of published properties .
+  ///    * invoke methods in event handlers.
+  _processClass(ClassElement cls, AssetId id) {
+    // Check whether the class has a @CustomTag annotation. Typically we expect
+    // a single @CustomTag, but it's possible to have several.
+    var tagNames = [];
+    for (var meta in cls.node.metadata) {
+      var tagName = _extractTagName(meta, cls);
+      if (tagName != null) tagNames.add(tagName);
+    }
+    if (tagNames.isEmpty) return;
+
+    if (cls.isPrivate) {
+      logger.error('@CustomTag is no longer supported on private classes:'

[Jennifer Messerly, 2014/03/27 02:20:32]

just curious, is this something we could lift in the future?

I guess we can always recommend putting types in a private ("src") library
instead. Maybe we could suggest that in the error message?

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

(Note that this warning was there before this change too)

Yes, we could lift this in the future, but we would have to change how we
generate initialization code.

The reason we have the restriction today is that we generate the initializers in
the main entrypoint file, so we need to be able to import this class. The fix
would be to change things so instead of doing:

   bootstrap.dart:
     configureForDeployment([
       () => Polymer.register('tag-name', prefix.ClassName),
     ]);


we would do:

   bootstrap.dart:
     configureForDeployment([
       prefix.polymer_init,
     ]);


   this_file.dart:
     ...
     polymer_init() => Polymer.register('tag-name', _ClassName);
I agree we should give hints on what to do, so I rewrote the warning.

I'm not sure if using a private library will work because the type needs to be
visible from importing this library (we only register things that are in the
immediate libraries from <script> tags, not in transitive imports.)

+          ' ${tagNames.first}', span: _spanForNode(cls, cls.node.name));
+      return;
+    }
+
+    // Include #registerCallback if it exists. Note that by default lookupMember
+    // and query will also add the corresponding getters and setters.
+    recorder.lookupMember(cls, 'registerCallback');

[Jennifer Messerly, 2014/03/27 02:20:32]

this reminds me, #registerCallback needs to go away :-)

(but hey, it's a nice place to issue a deprecation warning in a future CL)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

it's actually funny because smoke initially didn't have everything to support
#registerCallback and *Changed below, I had to add more features to support
them... oh well :)

+
+    // Include methods that end with *Changed.
+    recorder.runQuery(cls, new QueryOptions(
+          includeFields: false, includeProperties: false,
+          includeInherited: true, includeMethods: true,
+          includeUpTo: _htmlElementElement,
+          matches: (n) => n.endsWith('Changed') && n != 'attributeChanged'));
+
+    // Include methods marked with @ObserveProperty.
+    recorder.runQuery(cls, new QueryOptions(
+          includeFields: false, includeProperties: false,
+          includeInherited: true, includeMethods: true,
+          includeUpTo: _htmlElementElement,
+          withAnnotations: [_observePropertyElement]));
+
+    // Include @published and @observable properties.
+    // Symbols in @published are used when resolving bindings on published
+    // attributes, symbols for @observable are used via path observers when
+    // implementing *Changed an @ObserveProperty.
+    // TODO(sigmund): consider including only those symbols mentioned in
+    // *Changed and @ObserveProperty instead.
+    recorder.runQuery(cls, new QueryOptions(includeUpTo: _htmlElementElement,
+          withAnnotations: [_publishedElement, _observableElement]));
+
+    for (var tagName in tagNames) {
+      tagsWithClasses.add(tagName);
+      // Include an initializer that will call Polymer.register
+      initializers.add(new _CustomTagInitializer(id, tagName, cls.displayName));
+
+      // Include also properties published via the `attributes` attribute.
+      var attrs = publishedAttributes[tagName];
+      if (attrs == null) continue;
+      for (var attr in attrs) {
+        recorder.lookupMember(cls, attr, recursive: true);
+      }
+    }
+  }
+
+  /// If [meta] is [CustomTag], extract the name associated with the tag.
+  String _extractTagName(Annotation meta, ClassElement cls) {
+    if (meta.element != _customTagConstructor) return null;
+
+    // Read argument from the AST
+    var args = meta.arguments.arguments;
+    if (args == null || args.length == 0) {
+      logger.error('Missing argument in @CustomTag annotation',
+          span: _spanForNode(cls, meta));
+      return null;
+    }
+
+    if (args[0] is! StringLiteral) {
+      logger.error('Only string literals are currently supported by the polymer'

[Jennifer Messerly, 2014/03/27 02:20:32]

totally fine to fix in a follow up, but it looks like ConstantEvaluator in
lib/src/generated/constants.dart might do what you want pretty easily:

var result = new ConstantEvaluator(source, typeProvider).evaluate(args[0]);
if (result.isValid && result.value is String) { 
 ... use result.value
} else {
 ... print "not a constant" or "not a string" error
}

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

nice! Done

[Jennifer Messerly, 2014/03/28 21:48:35]

it worked? awesome! :D

[Siggi Cherem (dart-lang), 2014/03/28 22:08:47]

yeah! I was so surprised that I had to explicitly add tests before I believed it
:)

+          ' transformers. See dartbug.com/17739.',
+          span: _spanForNode(cls, meta));
+      return null;
+    }
+    return args[0].stringValue;
+  }
+
+  /// Adds the top-level [function] as an initalizer if it's marked with
+  /// `@initMethod`.
+  _processFunction(FunctionElement function, AssetId id) {
+    bool initMethodFound = false;
+    for (var meta in function.metadata) {
+      var e = meta.element;
+      if (e is PropertyAccessorElement && e.variable == _initMethodElement) {
+        initMethodFound = true;
+        break;
+      }
+    }
+    if (!initMethodFound) return;
+    if (function.isPrivate) {
+      logger.error('@initMethod is no longer supported on private '
+          'functions: ${function.displayName}',
+          span: _spanForNode(function, function.node.name));
+      return;
+    }
+    initializers.add(new _InitMethodInitializer(id, function.displayName));
+  }
+
+  /// Writes the final output for the bootstrap Dart file and entrypoint HTML
+  /// file.
+  void _emitFiles(_) {
     StringBuffer code = new StringBuffer()..writeln(MAIN_HEADER);
-    for (int i = 0; i < entryLibraries.length; i++) {
-      var url = assetUrlFor(entryLibraries[i], bootstrapId, logger);
-      if (url != null) code.writeln("import '$url' as i$i;");
-    }
-
-    code..write('\n')
-        ..writeln('void main() {')
-        ..writeln('  configureForDeployment([');
-
-    // Inject @CustomTag and @initMethod initializations for each library
-    // that is sourced in a script tag.
-    for (int i = 0; i < entryLibraries.length; i++) {
-      for (var init in initializers[entryLibraries[i]]) {
-        var initCode = init.asCode('i$i');
-        code.write("      $initCode,\n");
-      }
+    Map<AssetId, String> prefixes = {};
+    int i = 0;
+    for (var id in entryLibraries) {
+      var url = assetUrlFor(id, bootstrapId, logger);
+      if (url == null) continue;
+      code.writeln("import '$url' as i$i;");
+      prefixes[id] = 'i$i';
+      i++;
+    }
+
+    // Include smoke initialization.
+    generator.writeImports(code);
+    generator.writeTopLevelDeclarations(code);
+    code.writeln('\nvoid main() {');
+    generator.writeInitCall(code);
+    code.writeln('  configureForDeployment([');
+
+    // Include initializers to switch from mirrors_loader to static_loader.
+    // TODO(sigmund): do we need to sort out initializers to ensure that parent
+    // classes are initialized first?

[Jennifer Messerly, 2014/03/27 02:20:32]

currently no -- Polymer.js will handle it using the "extends" chain.

Is the order otherwise well defined? One nice property of html imports is it's
pretty easy to infer the order (* assuming the import graph isn't super deep)

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

yeah, we should be preserving the same order here, the only thing that changed
with this CL is that it used to be interleaved Polymer.register and initMethods
depending on the order they appeared in the source code. Now we cluster all
initMethods (for a given library) together first, and do class registrations
together afterwards.

I had this in the reverse order in the previous patch (classes before
initMethods), but I just swapped them to match the order we have in
mirrors_loader.dart. Yay consistency!

+    for (var init in initializers) {
+      var initCode = init.asCode(prefixes[init.assetId]);
+      code.write("      $initCode,\n");
     }
     code..writeln('    ]);')
         ..writeln('  i${entryLibraries.length - 1}.main();')
         ..writeln('}');
-    return code.toString();
-  }
-
-  /// Computes initializers needed for each library in [entryLibraries]. Results
-  /// are available afterwards in [initializers].
-  Future _computeInitializers() => Future.forEach(entryLibraries, (lib) {
-      return _initializersOf(lib).then((res) {
-        initializers[lib] = res;
-      });
-    });
-
-  /// Computes the initializers of [dartLibrary]. That is, a closure that calls
-  /// Polymer.register for each @CustomTag, and any public top-level methods
-  /// labeled with @initMethod.
-  Future<List<_Initializer>> _initializersOf(AssetId dartLibrary) {
-    var result = [];
-    return transform.readInputAsString(dartLibrary).then((code) {
-      var file = new SourceFile.text(_simpleUriForSource(dartLibrary), code);
-      var unit = parseCompilationUnit(code);
-
-      return Future.forEach(unit.directives, (directive) {
-        // Include anything from parts.
-        if (directive is PartDirective) {
-          var targetId = uriToAssetId(dartLibrary, directive.uri.stringValue,
-              logger, _getSpan(file, directive));
-          return _initializersOf(targetId).then(result.addAll);
-        }
-
-        // Similarly, include anything from exports except what's filtered by
-        // the show/hide combinators.
-        if (directive is ExportDirective) {
-          var targetId = uriToAssetId(dartLibrary, directive.uri.stringValue,
-              logger, _getSpan(file, directive));
-          return _initializersOf(targetId).then(
-            (r) => _processExportDirective(directive, r, result));
-        }
-      }).then((_) {
-        // Scan the code for classes and top-level functions.
-        for (var node in unit.declarations) {
-          if (node is ClassDeclaration) {
-            _processClassDeclaration(node, result, file, logger);
-          } else if (node is FunctionDeclaration &&
-              node.metadata.any(_isInitMethodAnnotation)) {
-            _processFunctionDeclaration(node, result, file, logger);
-          }
-        }
-        return result;
-      });
-    });
-  }
-
-  static String _simpleUriForSource(AssetId source) =>
-      source.path.startsWith('lib/')
-      ? 'package:${source.package}/${source.path.substring(4)}' : source.path;
-
-  /// Filter [exportedInitializers] according to [directive]'s show/hide
-  /// combinators and add the result to [result].
-  // TODO(sigmund): call the analyzer's resolver instead?
-  static _processExportDirective(ExportDirective directive,
-      List<_Initializer> exportedInitializers,
-      List<_Initializer> result) {
-    for (var combinator in directive.combinators) {
-      if (combinator is ShowCombinator) {
-        var show = combinator.shownNames.map((n) => n.name).toSet();
-        exportedInitializers.retainWhere((e) => show.contains(e.symbolName));
-      } else if (combinator is HideCombinator) {
-        var hide = combinator.hiddenNames.map((n) => n.name).toSet();
-        exportedInitializers.removeWhere((e) => hide.contains(e.symbolName));
-      }
-    }
-    result.addAll(exportedInitializers);
-  }
-
-  /// Add an initializer to register [node] as a polymer element if it contains
-  /// an appropriate [CustomTag] annotation.
-  static _processClassDeclaration(ClassDeclaration node,
-      List<_Initializer> result, SourceFile file,
-      TransformLogger logger) {
-    for (var meta in node.metadata) {
-      if (!_isCustomTagAnnotation(meta)) continue;
-      var args = meta.arguments.arguments;
-      if (args == null || args.length == 0) {
-        logger.error('Missing argument in @CustomTag annotation',
-            span: _getSpan(file, meta));
-        continue;
-      }
-
-      var tagName = args[0].stringValue;
-      var typeName = node.name.name;
-      if (typeName.startsWith('_')) {
-        logger.error('@CustomTag is no longer supported on private '
-          'classes: $tagName', span: _getSpan(file, node.name));
-        continue;
-      }
-      result.add(new _CustomTagInitializer(tagName, typeName));
-    }
-  }
-
-  /// Add a method initializer for [function].
-  static _processFunctionDeclaration(FunctionDeclaration function,
-      List<_Initializer> result, SourceFile file,
-      TransformLogger logger) {
-    var name = function.name.name;
-    if (name.startsWith('_')) {
-      logger.error('@initMethod is no longer supported on private '
-        'functions: $name', span: _getSpan(file, function.name));
-      return;
-    }
-    result.add(new _InitMethodInitializer(name));
+    transform.addOutput(new Asset.fromString(bootstrapId, code.toString()));
+    transform.addOutput(new Asset.fromString(docId, document.outerHtml));
+  }
+
+  _spanForNode(analyzer.Element context, AstNode node) {
+    var file = resolver.getSourceFile(context);
+    return file.span(node.offset, node.end);
   }
 }
 
-// TODO(sigmund): consider support for importing annotations with prefixes.
-bool _isInitMethodAnnotation(Annotation node) =>
-    node.name.name == 'initMethod' && node.constructorName == null &&
-    node.arguments == null;
-bool _isCustomTagAnnotation(Annotation node) => node.name.name == 'CustomTag';
-
 abstract class _Initializer {
+  AssetId get assetId;
   String get symbolName;
   String asCode(String prefix);
 }
 
 class _InitMethodInitializer implements _Initializer {
-  String methodName;
+  final AssetId assetId;
+  final String methodName;
   String get symbolName => methodName;
-  _InitMethodInitializer(this.methodName);
+  _InitMethodInitializer(this.assetId, this.methodName);
 
   String asCode(String prefix) => "$prefix.$methodName";
 }
 
 class _CustomTagInitializer implements _Initializer {
-  String tagName;
-  String typeName;
+  final AssetId assetId;
+  final String tagName;
+  final String typeName;
   String get symbolName => typeName;
-  _CustomTagInitializer(this.tagName, this.typeName);
+  _CustomTagInitializer(this.assetId, this.tagName, this.typeName);
 
   String asCode(String prefix) =>
       "() => Polymer.register('$tagName', $prefix.$typeName)";
 }
 
 _getSpan(SourceFile file, AstNode node) => file.span(node.offset, node.end);
 
 const MAIN_HEADER = """
 library app_bootstrap;
 
 import 'package:polymer/polymer.dart';
-import 'package:smoke/static.dart' as smoke;
 """;
+
+/// An html visitor that:
+///   * finds all polymer expressions and records the getters and setters that
+///     will be needed to evaluate them at runtime.
+///   * extracts all attributes declared in the `attribute` attributes of
+///     polymer elements.
+class _HtmlExtractor extends TreeVisitor {
+  final Map<String, List<String>> publishedAttributes;
+  final SmokeCodeGenerator generator;
+  final _SubExpressionVisitor visitor;
+  bool _inTemplate = false;
+
+  _HtmlExtractor(SmokeCodeGenerator generator, this.publishedAttributes)
+      : generator = generator,
+        visitor = new _SubExpressionVisitor(generator);
+
+  void visitElement(Element node) {
+    if (_inTemplate) _processNormalElement(node);
+    if (node.localName == 'polymer-element') {
+      _processPolymerElement(node);
+      _processNormalElement(node);
+    }
+
+    if (node.localName == 'template') {
+      var last = _inTemplate;
+      _inTemplate = true;
+      super.visitElement(node);
+      _inTemplate = last;
+    } else {
+      super.visitElement(node);
+    }
+  }
+
+  void visitText(Text node) {
+    if (!_inTemplate) return;
+    var bindings = _parseMustaches(node.data);
+    if (bindings == null) return;
+    for (var e in bindings.expressions) {
+      _addExpression(e, false, false);
+    }
+  }
+
+  /// Regex to split names in the attributes attribute, which supports 'a b c',
+  /// 'a,b,c', or even 'a b,c'.
+  static final _ATTRIBUTES_REGEX = new RegExp(r'\s|,');
+
+  /// Registers getters and setters for all published attributes.
+  void _processPolymerElement(Element node) {
+    var tagName = node.attributes['name'];
+    var value = node.attributes['attributes'];
+    if (value != null) {
+      publishedAttributes[tagName] =
+          value.split(_ATTRIBUTES_REGEX).map((a) => a.trim()).toList();
+    }
+  }
+
+  /// Produces warnings for misuses of on-foo event handlers, and for instanting
+  /// custom tags incorrectly.
+  void _processNormalElement(Element node) {
+    var tag = node.localName;
+    var isCustomTag = isCustomTagName(tag) || node.attributes['is'] != null;
+
+    // Event handlers only allowed inside polymer-elements
+    node.attributes.forEach((name, value) {
+      var bindings = _parseMustaches(value);
+      if (bindings == null) return;
+      var isEvent = false;
+      var isTwoWay = false;
+      if (name is String) {
+        name = name.toLowerCase();
+        isEvent = name.startsWith('on-');
+        isTwoWay = !isEvent && bindings.isWhole && (isCustomTag ||

[Jennifer Messerly, 2014/03/27 02:20:32]

how important is this?

it worries me on the maintenance side, new ones pop up occasionally in NodeBind
and/or TemplateBinding. e.g. it used to have some bindings for "template". I
think "ref" might still be bindable.

But also, with custom elements we can't really know; the element might chose
one-way or two-way.

Would it be a bad idea to defensively assume two-way bindings?

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

We could, not sure. We'd have to measure how much more code gets generated.

We are already defensively assuming two-way on any custom-tag attribute, though.
So I'm inclined to try this out white listing the possible two-way bindings we
have in TemplateBinding/NodeBind.

One idea: could we make the white list part of template_binding? Then it might
be easier to keep it up to date when updating that package.

[Jennifer Messerly, 2014/03/28 21:48:35]

yeah, it would be nice to be in template_binding, ideally even a way we can
assert that the binding is in the list at creation time. that way we can prevent
bugs. I don't think our Polymer tests adequately cover template_binding (whereas
template_binding tests cover all of these).

Doing it in a follow up change is good for me tho.

[Siggi Cherem (dart-lang), 2014/03/28 22:08:47]

Sounds great. I'll file a bug to track as well.

+            tag == 'input' && (name == 'value' || name =='checked') ||
+            tag == 'select' && (name == 'selectedindex' || name == 'value') ||
+            tag == 'textarea' && name == 'value');
+      }
+      for (var exp in bindings.expressions) {
+        _addExpression(exp, isEvent, isTwoWay);
+      }
+    });
+  }
+
+  void _addExpression(String stringExpression, bool inEvent, bool isTwoWay) {
+    if (inEvent) {
+      if (!stringExpression.startsWith("@")) {
+        generator.addGetter(stringExpression);
+        generator.addSymbol(stringExpression);
+        return;
+      }
+      stringExpression = stringExpression.substring(1);
+    }
+    visitor.run(pe.parse(stringExpression), isTwoWay);
+  }
+}
+
+/// A polymer-expression visitor that records every getter and setter that will
+/// be needed to evaluate a single expression at runtime.
+class _SubExpressionVisitor extends pe.RecursiveVisitor {
+  final SmokeCodeGenerator generator;
+  bool _includeSetter;
+
+  _SubExpressionVisitor(this.generator);
+
+  /// Visit [exp], and record getters and setters that are needed in order to
+  /// evaluate it at runtime. [includeSetter] is only true if this expression
+  /// occured in a context where it could be updated, for example in two-way
+  /// bindings such as `<input value={{exp}}>`.
+  void run(pe.Expression exp, bool includeSetter) {
+    _includeSetter = includeSetter;
+    visit(exp);
+  }
+
+  /// Adds a getter and symbol for [name], and optionally a setter.
+  _add(String name) {
+    generator.addGetter(name);
+    generator.addSymbol(name);
+    if (_includeSetter) generator.addSetter(name);
+  }
+
+  void preVisitExpression(e) {
+    // For two-way bindings the outermost expression may be updated, so we need
+    // both the getter and the setter, but subexpressions only need the getter.
+    // So we exclude setters as soon as we go deeper in the tree.
+    _includeSetter = false;
+  }
+
+  visitIdentifier(pe.Identifier e) {
+    if (e.value != 'this') _add(e.value);
+    super.visitIdentifier(e);
+  }
+
+  visitGetter(pe.Getter e) {
+    _add(e.name);
+    super.visitGetter(e);
+  }
+
+  visitInvoke(pe.Invoke e) {
+    _includeSetter = false; // Invoke is only valid as an r-value.
+    _add(e.method);
+    super.visitInvoke(e);
+  }
+}
+
+class _Mustaches {
+  /// Each expression that appears within `{{...}}` and `[[...]]`.
+  List<String> expressions = [];
+
+  /// Whether the whole text returned by [_parseMustaches] was a single mustache
+  /// expression.
+  bool isWhole;
+
+  _Mustaches(this.isWhole);
+}
+
+// TODO(sigmund): this is a simplification of what template-binding does. Could
+// we share some code here and in template_binding/src/mustache_tokens.dart?

[Jennifer Messerly, 2014/03/27 02:20:32]

could you just import that file? it doesn't look like it depends on dart:html or
anything crazy. I'm happy to help restructure it if needed...

this code has changed a few times already so that makes me leery about having a
copy

[Siggi Cherem (dart-lang), 2014/03/28 01:04:26]

Done. I managed to refactor it slightly and now I'm using it here. It
transitively imported dart:html via template_binding because of the dependency
on BindingDelegate, so I was hesitant at first to do it, but the changes seemed
pretty small. Let me know what you think.

+_Mustaches _parseMustaches(String text) {
+  if (text == null || text.isEmpty) return null;
+  var bindings = null;
+  var length = text.length;
+  var lastIndex = 0;
+  while (lastIndex < length) {
+    var startIndex = text.indexOf('{{', lastIndex);
+    var oneTimeStart = text.indexOf('[[', lastIndex);
+    var oneTime = false;
+    var terminator = '}}';
+
+    if (oneTimeStart >= 0 &&
+        (startIndex < 0 || oneTimeStart < startIndex)) {
+      startIndex = oneTimeStart;
+      oneTime = true;
+      terminator = ']]';
+    }
+
+    var endIndex = -1;
+    if (startIndex >= 0) {
+      endIndex = text.indexOf(terminator, startIndex + 2);
+    }
+
+    if (endIndex < 0) return bindings;
+
+    if (bindings == null) {
+      bindings = new _Mustaches(startIndex == 0 && endIndex == length - 2);
+    }
+    var pathString = text.substring(startIndex + 2, endIndex).trim();
+    bindings.expressions.add(pathString);
+    lastIndex = endIndex + 2;
+  }
+  return bindings;
+}