Major refactor of the transformer, added an `InitializePlugin` class which allows you to hook direc…

Major refactor of the transformer, added an `InitializePlugin` class which allows you to hook directly into the transformer and do custom actions at compile time.

R=sigmund@google.com

Committed: https://github.com/dart-lang/static_init/commit/4b775a0921a502b9ea18f8d774acc61cd20c6b47

[jakemac, 2015-02-12 21:23:22]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2015-02-13 00:50:37]

very nice!

minor comments, but otherwise looks great

https://codereview.chromium.org/923733002/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/923733002/diff/1/CHANGELOG.md#newcode1
CHANGELOG.md:1: ## 0.4.0
-dev?

https://codereview.chromium.org/923733002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/923733002/diff/1/README.md#newcode107
README.md:107: TODO: Add some examples here.
I think the pointer above is good, not sure if we need to add more here

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
File lib/build/initializer_plugin.dart (right):

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:129: String buildClassTarget(Element element,
InitializerPluginData pluginData) =>
let's change the type here Element -> ClassElement. Then you can probably
rephrase the comment a bit and simply refer to the parameter name (since the
type will be explicit)

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:132: /// Builds a [String] for the target of
an [InitEntry] given a [FunctionElement]
nit - 80 col (here and below)

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:135: Element element, InitializerPluginData
pluginData) =>
same here and with LibraryTarget below

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode26
lib/transformer.dart:26: bool appendDefaultPlugin: true}) {
I wonder if we need this, or we should just let them populate it into the
[plugins] list. 

Then we would just add it by default when plugins is null, but that's about it.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode152
lib/transformer.dart:152: transform.logger.error('Inline scripts are not
supported at this time.');
we could also point to the bug number here so people can follow it if they care.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode168
lib/transformer.dart:168: transform.logger, script.sourceSpan) ==
formatter bug? yikes!

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode222
lib/transformer.dart:222: plugins != null ? plugins : const [const
DefaultInitializerPlugin()];
not sure if the const list here will be much cheaper, I'd remove it for the sake
of fitting this in a single line :)

Same thing with const vs new on the DefaultInitia...

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode314
lib/transformer.dart:314: var pluginText;
delete this one? (it's declared again inside the loop)

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode315
lib/transformer.dart:315: while (!pluginRan && i < _plugins.length) {
maybe we could simplify it like this:

  var data = ...;
  var plugin = plugins.firstWhere((p) => p.shouldApply(data));
  var text = plugin.apply(data);
  if (text != null) initializersBuffer.writeln('$text,');

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode451
lib/transformer.dart:451: /// Element/ElementAnnotation pair.
reword - maybe just focus on the fact that this is a target and an annotation?

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode453
lib/transformer.dart:453: final Element element;
Since this class is now part of the plugin API, we should add dartdocs for each
member. 

Maybe rename:
 element -> targetElement
 node -> targetNode

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode457
lib/transformer.dart:457: // At least cache this for now, but provides a path to
doing something better.
nit: add a "TODO(jackemac): " prefix here

https://codereview.chromium.org/923733002/diff/1/test/plugin_transformer_test...
File test/plugin_transformer_test.dart (left):

https://codereview.chromium.org/923733002/diff/1/test/plugin_transformer_test...
test/plugin_transformer_test.dart:13: class DeleteInitMethodPlugin extends
InitializePluginTransformer {
should we add a test of this sort? Or it doesn't matter as much since the
default initialization is also implemented as a plugin?

[Siggi Cherem (dart-lang), 2015-02-13 16:45:37]

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode23
lib/transformer.dart:23: Asset bootstrapInitializers(Resolver resolver,
Transform transform,
looking at the other CL, I think we should rename this a bit.
"bootstrapInitializers" sounds to me like you are about to run the initializers,
some ideas:
 - generateBootstrapWithInitializers
 - generateInitializationMain
 - generateBootstrapFile

[jakemac, 2015-02-13 20:16:06]

https://codereview.chromium.org/923733002/diff/1/CHANGELOG.md
File CHANGELOG.md (right):

https://codereview.chromium.org/923733002/diff/1/CHANGELOG.md#newcode1
CHANGELOG.md:1: ## 0.4.0
On 2015/02/13 00:50:35, Siggi Cherem (dart-lang) wrote:
> -dev?

Oh whoops, I thought that 0.4.0 hadn't been published yet but it has, moved to a
0.5.0-dev section.

https://codereview.chromium.org/923733002/diff/1/README.md
File README.md (right):

https://codereview.chromium.org/923733002/diff/1/README.md#newcode107
README.md:107: TODO: Add some examples here.
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> I think the pointer above is good, not sure if we need to add more here

Done.

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
File lib/build/initializer_plugin.dart (right):

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:129: String buildClassTarget(Element element,
InitializerPluginData pluginData) =>
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> let's change the type here Element -> ClassElement. Then you can probably
> rephrase the comment a bit and simply refer to the parameter name (since the
> type will be explicit)

Done.

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:132: /// Builds a [String] for the target of
an [InitEntry] given a [FunctionElement]
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> nit - 80 col (here and below)

Done.

https://codereview.chromium.org/923733002/diff/1/lib/build/initializer_plugin...
lib/build/initializer_plugin.dart:135: Element element, InitializerPluginData
pluginData) =>
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> same here and with LibraryTarget below

Done.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode23
lib/transformer.dart:23: Asset bootstrapInitializers(Resolver resolver,
Transform transform,
On 2015/02/13 16:45:37, Siggi Cherem (dart-lang) wrote:
> looking at the other CL, I think we should rename this a bit.
> "bootstrapInitializers" sounds to me like you are about to run the
initializers,
> some ideas:
>  - generateBootstrapWithInitializers
>  - generateInitializationMain
>  - generateBootstrapFile

chose generateBootstrapFile

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode26
lib/transformer.dart:26: bool appendDefaultPlugin: true}) {
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> I wonder if we need this, or we should just let them populate it into the
> [plugins] list. 
> 
> Then we would just add it by default when plugins is null, but that's about
it.

I think it would be an extremely rare case where we wouldn't want this to be
added though, and it would be really easy for users to forget to add it.

The fact that it is a plugin is really just an implementation detail, that most
people should never care about imo.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode152
lib/transformer.dart:152: transform.logger.error('Inline scripts are not
supported at this time.');
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> we could also point to the bug number here so people can follow it if they
care.
> 

Done.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode168
lib/transformer.dart:168: transform.logger, script.sourceSpan) ==
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> formatter bug? yikes!

Ya this is pretty bad :(. I broke down and changed this into the {} syntax with
a local variable so its not so horrendous, although it does hit a different bug.
I also filed https://github.com/dart-lang/dart_style/issues/169

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode222
lib/transformer.dart:222: plugins != null ? plugins : const [const
DefaultInitializerPlugin()];
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> not sure if the const list here will be much cheaper, I'd remove it for the
sake
> of fitting this in a single line :)
> 
> Same thing with const vs new on the DefaultInitia...

left the const on the DefaultInitializerPlugin since that doesn't cause line
wrap, removed it from the list though.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode314
lib/transformer.dart:314: var pluginText;
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> delete this one? (it's declared again inside the loop)

Done.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode315
lib/transformer.dart:315: while (!pluginRan && i < _plugins.length) {
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> maybe we could simplify it like this:
> 
>   var data = ...;
>   var plugin = plugins.firstWhere((p) => p.shouldApply(data));
>   var text = plugin.apply(data);
>   if (text != null) initializersBuffer.writeln('$text,');

Done, this is much better for sure. I also added a try/catch with a logger error
if firstWhere throws.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode451
lib/transformer.dart:451: /// Element/ElementAnnotation pair.
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> reword - maybe just focus on the fact that this is a target and an annotation?

Done.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode453
lib/transformer.dart:453: final Element element;
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> Since this class is now part of the plugin API, we should add dartdocs for
each
> member. 
> 
> Maybe rename:
>  element -> targetElement
>  node -> targetNode

done, also renamed elementAnnotation -> annotationElement and annotation ->
annotationNode

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode457
lib/transformer.dart:457: // At least cache this for now, but provides a path to
doing something better.
On 2015/02/13 00:50:36, Siggi Cherem (dart-lang) wrote:
> nit: add a "TODO(jackemac): " prefix here

Done.

https://codereview.chromium.org/923733002/diff/1/lib/transformer.dart#newcode480
lib/transformer.dart:480: InitializerData(this.element, this.elementAnnotation);
I also renamed this to a private constructor, so we can change it to take a node
later without issue.

https://codereview.chromium.org/923733002/diff/1/test/plugin_transformer_test...
File test/plugin_transformer_test.dart (left):

https://codereview.chromium.org/923733002/diff/1/test/plugin_transformer_test...
test/plugin_transformer_test.dart:13: class DeleteInitMethodPlugin extends
InitializePluginTransformer {
On 2015/02/13 00:50:35, Siggi Cherem (dart-lang) wrote:
> should we add a test of this sort? Or it doesn't matter as much since the
> default initialization is also implemented as a plugin?

The plugin architecture is being lightly tested via the
DefaultInitializerPlugin, but I probably should add a test which combines
multiple plugins to make sure that all works.

done

[Siggi Cherem (dart-lang), 2015-02-13 20:25:42]

lgtm!

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart#new...
lib/transformer.dart:320: } on StateError catch (e) {
FYI - another option here would be to pass in the `orElse` to firstWhere:

  var plugin = plugins.firstWhere((p) => p.shouldApply(data), orElse: () =>
null);
  if (plugin == null) { 
    ... error;
  }


totally up to you though.

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart#new...
lib/transformer.dart:452: /// The [ElementAnnotation] representing the
annotation itself.
+ empty line above/run formatter once more :)

[jakemac, 2015-02-13 22:13:47]

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart
File lib/transformer.dart (right):

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart#new...
lib/transformer.dart:320: } on StateError catch (e) {
On 2015/02/13 20:25:42, Siggi Cherem (dart-lang) wrote:
> FYI - another option here would be to pass in the `orElse` to firstWhere:
> 
>   var plugin = plugins.firstWhere((p) => p.shouldApply(data), orElse: () =>
> null);
>   if (plugin == null) { 
>     ... error;
>   }
> 
> 
> totally up to you though.

Done.

https://codereview.chromium.org/923733002/diff/20001/lib/transformer.dart#new...
lib/transformer.dart:452: /// The [ElementAnnotation] representing the
annotation itself.
On 2015/02/13 20:25:42, Siggi Cherem (dart-lang) wrote:
> + empty line above/run formatter once more :)

Done.

Fyi the formatter does not do this, if its a general style rule then we should
probably file a bug.

[jakemac, 2015-02-13 22:13:54]

New patchsets have been uploaded after l-g-t-m from sigmund@google.com

[jakemac, 2015-02-13 22:19:38]

Committed patchset #3 (id:40001) manually as
4b775a0921a502b9ea18f8d774acc61cd20c6b47 (presubmit successful).