Skip to content
This repository was archived by the owner on Sep 24, 2021. It is now read-only.

Initial commit of Analyzer-based Linter. Ported over first linter 'unbalanced-delimiters' #1

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 168 additions & 0 deletions src/expressions.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/**
* @license
* Copyright (c) 2015 The Polymer Project Authors. All rights reserved.
* This code may only be used under the BSD style license found at
* http://polymer.github.io/LICENSE.txt
* The complete set of authors may be found at
* http://polymer.github.io/AUTHORS.txt
* The complete set of contributors may be found at
* http://polymer.github.io/CONTRIBUTORS.txt
* Code distributed by Google as part of the polymer project is also
* subject to an additional IP rights grant found at
* http://polymer.github.io/PATENTS.txt
*/

// TODO(usergenic): Migrate this to polymer-analyzer
// https://github.com/Polymer/polymer-analyzer/issues/351

/**
* A parsed polymer binding expression
* @param {Array.<string>} keys The keys referenced by this expression.
* @param {Array.<string>} methods The methods referenced by this expression.
* @param {string} type One of 'computed', 'literal', or 'reference'
* @param {string} raw The unparsed expression
*/
export class ParsedExpression {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend instead making this:

type ParsedExpression = Computed | Literal | Reference

And make each of those three types an interface with a string literal type field. That way we can separate out the fields and type safety will help us in a lot of ways.

This is how we're typing estree, and it's paid off hugely. It lets you switch over expression.type.. lots of stuff.

public keys: Array<string>;
public methods: Array<string>;
public type: string;
public raw: string;
}

export interface Signature {
method: string;
static: boolean;
args?: Argument[];
}

export interface Argument {
name: string;
value?: string|number;
literal?: boolean;
structured?: boolean;
wildcard?: boolean;
}

function primaryName(expression: string): string {
// TODO(usergenic): Remove this commented out section copied in from polylint
// if (expression.name) {
// expression = expression.name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was sometimes an estree.Identifier, I say aggressively delete.

// }
if (expression.match(/^('|").*('|")$/)) { // string literal
return '';
}
if (expression.match(/^-?\d*\.?\d+$/)) { // number literal
return '';
}
if (expression.indexOf('!') === 0) {
return primaryName(expression.slice(1));
}
if (expression.indexOf('.') === -1) {
return expression;
} else {
return expression.split('.')[0];
}
}

export class ExpressionParser {
public extractBindingExpression(text: string): string {
const match = text.match(/\{\{(.*)\}\}/) || text.match(/\[\[(.*)\]\]/);
if (match && match.length === 2) {
let expression: string = match[1];
if (expression.indexOf('::') > -1) {
expression = expression.slice(0, expression.indexOf('::'));
}
return expression.trim();
}
return '';
}

public parseExpression(expression: string): ParsedExpression {
const parsed = new ParsedExpression();
parsed.raw = expression;

const unwrapped = this.extractBindingExpression(expression);
const parsedMethod = this._parseMethod(unwrapped);
if (parsedMethod) {
parsed.type = 'computed';
parsed.keys = parsedMethod.args!.map((arg) => primaryName(arg.name));
parsed.methods = [parsedMethod.method];
} else {
parsed.type = 'reference';
parsed.methods = [];
parsed.keys = [primaryName(unwrapped)];
}
return parsed;
}

private _parseMethod(expression: string): Signature|undefined {
const m = expression.match(/(\w*)\((.*)\)/);
if (m) {
const sig: Signature = {method: m[1], static: true};
if (m[2].trim()) {
// replace escaped commas with comma entity, split on un-escaped commas
const args = m[2].replace(/\\,/g, '&comma;').split(',');
return this._parseArgs(args, sig);
} else {
sig.args = [];
return sig;
}
}
}

private _parseArgs(argList: string[], sig: Signature): Signature {
sig.args = argList.map((rawArg) => {
const arg = this._parseArg(rawArg);
if (!arg.literal) {
sig.static = false;
}
return arg;
});
return sig;
}

private _parseArg(rawArg: string): Argument {
// clean up whitespace
const arg =
rawArg
.trim()
// replace comma entity with comma
.replace(/&comma;/g, ',')
// repair extra escape sequences; note only commas strictly need
// escaping, but we allow any other char to be escaped since its
// likely users will do this
.replace(/\\(.)/g, '\$1');
// basic argument descriptor
const a: Argument = {name: arg};
// detect literal value (must be String or Number)
let fc = arg[0];
if (fc >= '0' && fc <= '9') {
fc = '#';
}
switch (fc) {
case '\'':
case '"':
a.value = arg.slice(1, -1);
a.literal = true;
break;
case '#':
a.value = Number(arg);
a.literal = true;
break;
default:
// no-op
}
// if not literal, look for structured path
if (!a.literal) {
// detect structured path (has dots)
a.structured = arg.indexOf('.') > 0;
if (a.structured) {
a.wildcard = (arg.slice(-2) === '.*');
if (a.wildcard) {
a.name = arg.slice(0, -2);
}
}
}
return a;
}
}
7 changes: 4 additions & 3 deletions src/matchers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@
* subject to an additional IP rights grant found at
* http://polymer.github.io/PATENTS.txt
*/

'use strict';

import * as dom5 from 'dom5';

const p = dom5.predicates;

export const isDomBindTemplate =
p.AND(p.hasTagName('template'), p.hasAttrValue('is', 'dom-bind'));
export const isDomModuleTemplate = p.AND(
p.parentMatches(p.hasTagName('dom-module')), p.hasTagName('template'));
export const isTemplate = p.hasTagName('template');
export const isTemplateDescendant = p.parentMatches(isTemplate);
140 changes: 140 additions & 0 deletions src/rules/native-attribute-binding.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/**
* @license
* Copyright (c) 2014 The Polymer Project Authors. All rights reserved.
* This code may only be used under the BSD style license found at
* http://polymer.github.io/LICENSE.txt
* The complete set of authors may be found at
* http://polymer.github.io/AUTHORS.txt
* The complete set of contributors may be found at
* http://polymer.github.io/CONTRIBUTORS.txt
* Code distributed by Google as part of the polymer project is also
* subject to an additional IP rights grant found at
* http://polymer.github.io/PATENTS.txt
*/
// import {PolymerElement} from 'polymer-analyzer/lib/polymer/polymer-element';
import * as dom5 from 'dom5';
import * as parse5 from 'parse5';
import {ParsedHtmlDocument} from 'polymer-analyzer/lib/html/html-document';
import {Document} from 'polymer-analyzer/lib/model/model';
import {Severity, Warning} from 'polymer-analyzer/lib/warning/warning';

import * as expressions from '../expressions';
import * as matchers from '../matchers';
import {Rule} from '../rule';

export const a11yAttributes: Set<string> = new Set([
'aria-activedescendant',
'aria-atomic',
'aria-autocomplete',
'aria-busy',
'aria-checked',
'aria-controls',
'aria-describedby',
'aria-disabled',
'aria-dropeffect',
'aria-expanded',
'aria-flowto',
'aria-grabbed',
'aria-haspopup',
'aria-hidden',
'aria-invalid',
'aria-label',
'aria-labelledby',
'aria-level',
'aria-live',
'aria-multiline',
'aria-multiselectable',
'aria-orientation',
'aria-owns',
'aria-posinset',
'aria-pressed',
'aria-readonly',
'aria-relevant',
'aria-required',
'aria-selected',
'aria-setsize',
'aria-sort',
'aria-valuemax',
'aria-valuemin',
'aria-valuenow',
'aria-valuetext',
'role'
]);

export const nativeAttributes: Set<string> = new Set([
'accesskey',
'class',
'contenteditable',
'contextmenu',
'dir',
'draggable',
'dropzone',
'hidden',
'href',
'id',
'itemprop',
'lang',
'spellcheck',
'style',
'style',
'tabindex',
'title'
]);

const expressionParser = new expressions.ExpressionParser();

export class NativeAttributeBinding implements Rule {
public async check(document: Document): Promise<Warning[]> {
const warnings: Warning[] = [];
const templates = dom5.queryAll(
document.parsedDocument.ast,
dom5.predicates.OR(
matchers.isDomBindTemplate, matchers.isDomModuleTemplate));

for (const template of templates) {
for (const element of dom5.queryAll(
parse5.treeAdapters.default.getTemplateContent(template),
(e) => true)) {
for (const attr of element.attrs) {
if (this._isBindingExpression(attr.value) &&
!this._canAttributeUseNativeBinding(element, attr)) {
const parsedHtml = document.parsedDocument;
if (!(parsedHtml instanceof ParsedHtmlDocument)) {
continue;
}
warnings.push({
code: 'native-attribute-binding',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere in this file, this isn't about native attribute binding so much as attributes which have no corresponding property.

Recommend updating the names, code, and message here.

Also, do we need to add the attr value and name here? With the new warning printer it will show you the line of code with full info.

message: `The expression ${attr.value} bound to attribute ` +
`'${attr.name}' should use $= instead of =`,
severity: Severity.ERROR,
sourceRange:
parsedHtml.sourceRangeForAttribute(element, attr.name)!
});
}
}
}
}
return warnings;
}

private _canAttributeUseNativeBinding(
element: parse5.ASTNode,
attr: parse5.ASTAttribute): boolean {
const name: string = attr.name.toLowerCase();
if (name === 'for' && dom5.predicates.hasTagName('label')(element)) {
return false;
}
if (nativeAttributes.has(name)) {
return false;
}
if (name.indexOf('data-') === 0 && attr.name[name.length - 1] !== '$') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we checking whether attr.name ends with $ here? Shouldn't it be checked elsewhere already?

return false;
}
return true;
}

private _isBindingExpression(expression: string): boolean {
const bindingMatch = expressionParser.extractBindingExpression(expression);
return !!bindingMatch || false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK !!foo || false is equal to !!foo for all values of foo.

}
}
64 changes: 64 additions & 0 deletions src/test/native-attribute-binding_test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* @license
* Copyright (c) 2014 The Polymer Project Authors. All rights reserved.
* This code may only be used under the BSD style license found at
* http://polymer.github.io/LICENSE.txt
* The complete set of authors may be found at
* http://polymer.github.io/AUTHORS.txt
* The complete set of contributors may be found at
* http://polymer.github.io/CONTRIBUTORS.txt
* Code distributed by Google as part of the polymer project is also
* subject to an additional IP rights grant found at
* http://polymer.github.io/PATENTS.txt
*/
import * as assert from 'assert';
import {Severity, Warning} from 'polymer-analyzer/lib/warning/warning';

import {Linter} from '../linter';
import {NativeAttributeBinding} from '../rules/native-attribute-binding';

suite('NativeAttributeBinding', () => {

test('bind to class', async() => {
const linter: Linter = new Linter([new NativeAttributeBinding()]);
const warnings: Warning[] =
(await linter.lint(['test/sample/imports/bind-to-class.html']))
.filter((warning) => warning.code === 'native-attribute-binding');

assert.equal(warnings.length, 1);

assert.deepEqual(warnings[0], {
code: 'native-attribute-binding',
message:
'The expression [[myVars]] bound to attribute \'class\' should use $= instead of =',
severity: Severity.ERROR,
sourceRange: {
end: {column: 28, line: 11},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend using the WarningPrinter#printUnderlinedText() trick I used in my analyzer PR recently to test these source ranges. Makes them more maintainable and more easily reviewed.

file: 'test/sample/imports/bind-to-class.html',
start: {column: 10, line: 11}
}
});
});

test('bind to data', async() => {
const linter: Linter = new Linter([new NativeAttributeBinding()]);
const warnings: Warning[] =
(await linter.lint(['test/sample/imports/bind-to-data.html']))
.filter((warning) => warning.code === 'native-attribute-binding');

assert.equal(warnings.length, 1);

assert.deepEqual(warnings[0], {
code: 'native-attribute-binding',
message:
'The expression [[myVars]] bound to attribute \'data-page\' should ' +
'use $= instead of =',
severity: Severity.ERROR,
sourceRange: {
end: {column: 32, line: 11},
file: 'test/sample/imports/bind-to-data.html',
start: {column: 10, line: 11}
}
});
});
});
Loading