Add a polymer validator: a linter/analysis that will replace the old

Add a polymer validator: a linter/analysis that will replace the old
dwc.analyzer. This analysis covers what we used to have before, but hooks into
barback (so we'll be able to see assets that are generated too).

I plan to delete analyzer.dart and compiler.dart next, but decided to do so in a
separate CL.

R=jmesserly@google.com

Committed: https://code.google.com/p/dart/source/detail?r=27162

[Siggi Cherem (dart-lang), 2013-09-03 01:28:10]

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
File pkg/polymer/lib/src/validator.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:161: class _ElementSummary {
this is the only subset of Info that I still wanted to preserve. After this CL
and providing a way to invoke this validator (which should be similar to
deploy.dart), we should be ready to delete the rest of the compiler.

[Jennifer Messerly, 2013-09-04 03:13:26]

this is looking great, some relatively minor comments

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:102: String assetUrlFor(AssetId id,
AssetId sourceId, TransformLogger logger) {
TODO: this should be in barback, e.g. AssetId.uri or some function like that?
(how do we capture .uriForDart vs .uriForHtml distinction? since the latter does
not support "asset:")

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:109: return
'asset:${id.package}/${id.path.substring(6)}';
Hmm, this won't work if we're in a Dart file?

Does anything support "asset:" yet? Is the idea that html/css support this? It
all seems strange, if we set an image src to "asset:" at runtime that can't
possibly work... Or worse, it would work with dev server and not deployment... I
guess I'm confused about the intent of "asset" but I haven't had any experience
using it, so forgive the newbie questions :)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/script_compactor.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/script_compactor.dart:90: var urls =
libraries.map((id) => assetUrlFor(id, bootstrapId, logger))
as noted earlier, I don't think we want Dart files imported via "asset:"

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
File pkg/polymer/lib/src/validator.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:4: 
Apologies for any comments that are actually about the original code! I couldn't
tell from the diff tool :)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:6: * Barback transformer that doesn't
actually mutate the HTML, but validates that
"transformer" ... you keep using that word, I do not think it means what you
think it means ;)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:10: library validator;
polymer.src.linter?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:25: * A validator that checks for common
polymer errors and produces warnings to
Polymer?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:30: class Validator extends Transformer {
suggestion: "Validator" sounds to me like a Pass/Fail system. Perhaps "Linter"?

On the other hand, as I read further I like the use of "_validate" as a verb. So
I'm having second thoughts. Still it would be nice if we could call the tool
"Polymer linter", WDYT?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:34: final MessageFormatter _formatter;
newline after?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:97: if (elements.containsKey(name)) {
you could combine these two lines into one Map lookup:

var existing = elements[name];
if (existing != null) { ... }

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:117: class _TransformLoggerInterceptor
implements Transform, TransformLogger {
suggestion: since this is a private something shorter like "_LoggerInterceptor"
would work too. But this name is good too.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:127: noSuchMethod(Invocation m) =>
reflect(_original).delegate(m);
Wow. An actual non-boilerplate-y proxy. Win!

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:141: * by the Dart Editor when reporting
error messages.
maybe: "by tools such as the Dart Editor"?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:166: _ElementSummary extendsPolymerElement;
could this just be called "extends" (or "extendsType") ?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:174: String toString() => "($tagName <:
$extendsTag)";
nice use of subtype relation

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:187: if (extendsTag == null ||
_elements[extendsTag] == null) continue;
not a big deal, but cache _elements[extendsTag] in local?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:229: _logger.error('Nested polymer element
definitions are not allowed.',
hmmm, I'm not sure nested definitions are wise, but it would probably work :)

e.g. a meta-polymer-element that stamps out other elements, kind of like an
element macro.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:237: if (tagName == null) {
warn if (!_isCustomTag(tagName)) ?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:262: * they'll help polymer authors avoid
having their Dart code accidentally
Polymer?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:267: var src = node.attributes["src"];
single quote?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:271: _logger.warning('script tag in polymer
element with no type will '
add a TODO here? this warning is a bit dubious once we can interoperate with JS
components on the same page. I dunno what a good answer is for that case though,
since trying to determine automatically if it's Dart or JS is hard. It's not an
accurate test by any means, but we could sniff for `import
'polymer/polymer.dart'` and suggest type=dart for that case only

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:303: void _validateNormalElement(Element
node) {
something for the future:
validate that {{ bindings }} are valid polymer_expressions

[Siggi Cherem (dart-lang), 2013-09-04 17:57:27]

Thanks John. PTAL

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:102: String assetUrlFor(AssetId id,
AssetId sourceId, TransformLogger logger) {
On 2013/09/04 03:13:27, John Messerly wrote:
> TODO: this should be in barback, e.g. AssetId.uri or some function like that?
> (how do we capture .uriForDart vs .uriForHtml distinction? since the latter
does
> not support "asset:")

Done. Added TODO and more comments in bug/12610

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:109: return
'asset:${id.package}/${id.path.substring(6)}';
On 2013/09/04 03:13:27, John Messerly wrote:
> Hmm, this won't work if we're in a Dart file?

Correct. I added more details about this in the bug above. I also added a check
here to ensure we don't run into this problem where we intend to use the url for
dart imports.

> 
> Does anything support "asset:" yet?

Not yet, but soon. I plan to add support for it in our deploy script and in
boot.js.

> Is the idea that html/css support this?

In dartium/boot.js yes.

> It all seems strange, if we set an image src to "asset:" at runtime that can't
> possibly work... Or worse, it would work with dev server and not deployment...
I
> guess I'm confused about the intent of "asset" but I haven't had any
experience
> using it, so forgive the newbie questions :)

My understanding is that our goal is to eventually support it in all these use
cases.

For now, there are only 2 uses of this function in our codebase:
 - get import urls that we'll use in the bootstrap script. These only allow
package: urls, so we won't hit this case of asset: urls (just added some checks
for this)
 - find the url of the source file for reporting errors. The idea is that if you
import an html file from another package, the error message will use a URL that
resolves to that file.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/script_compactor.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/script_compactor.dart:90: var urls =
libraries.map((id) => assetUrlFor(id, bootstrapId, logger))
On 2013/09/04 03:13:27, John Messerly wrote:
> as noted earlier, I don't think we want Dart files imported via "asset:"

good point. added a check for that here.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
File pkg/polymer/lib/src/validator.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:4: 
On 2013/09/04 03:13:27, John Messerly wrote:
> Apologies for any comments that are actually about the original code! I
couldn't
> tell from the diff tool :)

No problem. Even if they are from the old code, it's good to make it better =)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:6: * Barback transformer that doesn't
actually mutate the HTML, but validates that
On 2013/09/04 03:13:27, John Messerly wrote:
> "transformer" ... you keep using that word, I do not think it means what you
> think it means ;)

I think I'm lost :-/?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:10: library validator;
On 2013/09/04 03:13:27, John Messerly wrote:
> polymer.src.linter?

Done. (polymer.src.validator for now until we decide what the name of the entire
thing should be)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:25: * A validator that checks for common
polymer errors and produces warnings to
On 2013/09/04 03:13:27, John Messerly wrote:
> Polymer?

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:30: class Validator extends Transformer {
On 2013/09/04 03:13:27, John Messerly wrote:
> suggestion: "Validator" sounds to me like a Pass/Fail system. Perhaps
"Linter"?
> 
> On the other hand, as I read further I like the use of "_validate" as a verb.
So
> I'm having second thoughts. Still it would be nice if we could call the tool
> "Polymer linter", WDYT?

Funny, I was chatting with Seth about this name yesterday too, I'm not sure
either. In a subsequent CL I plan to expose 2 top-level method that people can
use in their build.dart:
 - validate
 - deploy

I'd be ok with having the class and tool be called a Linter, but should we
change the name of the function to call it? 'validate' sounds nice, 'check' or
'checkClean' work too, but I have a slight preference for 'validate'.

Thoughts?

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:34: final MessageFormatter _formatter;
On 2013/09/04 03:13:27, John Messerly wrote:
> newline after?

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:97: if (elements.containsKey(name)) {
On 2013/09/04 03:13:27, John Messerly wrote:
> you could combine these two lines into one Map lookup:
> 
> var existing = elements[name];
> if (existing != null) { ... }

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:117: class _TransformLoggerInterceptor
implements Transform, TransformLogger {
On 2013/09/04 03:13:27, John Messerly wrote:
> suggestion: since this is a private something shorter like
"_LoggerInterceptor"
> would work too. But this name is good too.

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:127: noSuchMethod(Invocation m) =>
reflect(_original).delegate(m);
On 2013/09/04 03:13:27, John Messerly wrote:
> Wow. An actual non-boilerplate-y proxy. Win!

:-)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:166: _ElementSummary extendsPolymerElement;
On 2013/09/04 03:13:27, John Messerly wrote:
> could this just be called "extends" (or "extendsType") ?

Done. Went with extendsType since we extends is a keyword

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:174: String toString() => "($tagName <:
$extendsTag)";
On 2013/09/04 03:13:27, John Messerly wrote:
> nice use of subtype relation

=)

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:187: if (extendsTag == null ||
_elements[extendsTag] == null) continue;
On 2013/09/04 03:13:27, John Messerly wrote:
> not a big deal, but cache _elements[extendsTag] in local?

Done. Since in that case is null, removing the second check does basically what
we want.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:229: _logger.error('Nested polymer element
definitions are not allowed.',
On 2013/09/04 03:13:27, John Messerly wrote:
> hmmm, I'm not sure nested definitions are wise, but it would probably work :)
> 
> e.g. a meta-polymer-element that stamps out other elements, kind of like an
> element macro.

not sure how that works with document.register though.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:237: if (tagName == null) {
On 2013/09/04 03:13:27, John Messerly wrote:
> warn if (!_isCustomTag(tagName)) ?

Good idea. Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:262: * they'll help polymer authors avoid
having their Dart code accidentally
On 2013/09/04 03:13:27, John Messerly wrote:
> Polymer?

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:267: var src = node.attributes["src"];
On 2013/09/04 03:13:27, John Messerly wrote:
> single quote?

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:271: _logger.warning('script tag in polymer
element with no type will '
On 2013/09/04 03:13:27, John Messerly wrote:
> add a TODO here? this warning is a bit dubious once we can interoperate with
JS
> components on the same page. I dunno what a good answer is for that case
though,
> since trying to determine automatically if it's Dart or JS is hard. It's not
an
> accurate test by any means, but we could sniff for `import
> 'polymer/polymer.dart'` and suggest type=dart for that case only

Done.

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:303: void _validateNormalElement(Element
node) {
On 2013/09/04 03:13:27, John Messerly wrote:
> something for the future:
> validate that {{ bindings }} are valid polymer_expressions

Good idea. Created dartbug.com/13040 to track.

[Jennifer Messerly, 2013-09-04 19:21:33]

lgtm

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
File pkg/polymer/lib/src/validator.dart (right):

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:6: * Barback transformer that doesn't
actually mutate the HTML, but validates that
On 2013/09/04 17:57:27, Siggi Cherem (dart-lang) wrote:
> On 2013/09/04 03:13:27, John Messerly wrote:
> > "transformer" ... you keep using that word, I do not think it means what you
> > think it means ;)
> 
> I think I'm lost :-/?

Sorry that was too cute on my part :). What I meant is: it doesn't actually
transform anything, so it's strange to call it a "transformer".

https://codereview.chromium.org/23452010/diff/5001/pkg/polymer/lib/src/valida...
pkg/polymer/lib/src/validator.dart:30: class Validator extends Transformer {
On 2013/09/04 17:57:27, Siggi Cherem (dart-lang) wrote:
> On 2013/09/04 03:13:27, John Messerly wrote:
> > suggestion: "Validator" sounds to me like a Pass/Fail system. Perhaps
> "Linter"?
> > 
> > On the other hand, as I read further I like the use of "_validate" as a
verb.
> So
> > I'm having second thoughts. Still it would be nice if we could call the tool
> > "Polymer linter", WDYT?
> 
> Funny, I was chatting with Seth about this name yesterday too, I'm not sure
> either. In a subsequent CL I plan to expose 2 top-level method that people can
> use in their build.dart:
>  - validate
>  - deploy
> 
> I'd be ok with having the class and tool be called a Linter, but should we
> change the name of the function to call it? 'validate' sounds nice, 'check' or
> 'checkClean' work too, but I have a slight preference for 'validate'.
> 
> Thoughts?

yeah, it's a tossup. this works for me

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
pkg/polymer/lib/src/transform/common.dart:106: {bool allowAssetUrl: true}) {
might be safer to make "false" the default?

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
pkg/polymer/lib/src/transform/common.dart:118: return
'asset:${id.package}/${id.path.substring(6)}';
it's more of a Pub issue, not releated to this CL in particular, but introducing
another URI scheme makes me uneasy...

[Siggi Cherem (dart-lang), 2013-09-04 22:34:26]

thanks! renamed validator to linter, will submit shortly

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
pkg/polymer/lib/src/transform/common.dart:106: {bool allowAssetUrl: true}) {
On 2013/09/04 19:21:34, John Messerly wrote:
> might be safer to make "false" the default?

done.

https://codereview.chromium.org/23452010/diff/11001/pkg/polymer/lib/src/trans...
pkg/polymer/lib/src/transform/common.dart:118: return
'asset:${id.package}/${id.path.substring(6)}';
On 2013/09/04 19:21:34, John Messerly wrote:
> it's more of a Pub issue, not releated to this CL in particular, but
introducing
> another URI scheme makes me uneasy...

I hear you.

[Siggi Cherem (dart-lang), 2013-09-04 22:38:03]

Committed patchset #4 manually as r27162 (presubmit successful).