Skip to content

Commit

Permalink
Initial AST based linter spike (#218)
Browse files Browse the repository at this point in the history
refs https://github.com/TryGhost/gscan/issues/265
refs #163

- Initial AST linter spike
- Add basic handling of parse errors to AST linter
- catch an error from the parse step and push it into the returned messages as a fatal error

Invalid file `test.hbs`:
```hbs
{{#primary_author}}
    {{name}}
{{/author}}
```

Resulting message:
```
[
  {
    "moduleId": "test.hbs",
    "message": "primary_author doesn't match author - 1:3",
    "fatal": true,
    "column": 3,
    "line": 1
  }
]
```
  • Loading branch information
kevinansfield authored and naz committed Oct 29, 2019
1 parent 203a661 commit 361214a
Show file tree
Hide file tree
Showing 8 changed files with 281 additions and 0 deletions.
28 changes: 28 additions & 0 deletions lib/ast-linter/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# ast-linter

ast-linter uses handlebars.js to generate an AST which is then walked, it
allows for more robust checks than our regex approach allows in certain cases.

Heavily inspired by, borrowed from, and generally ripped off of
https://github.com/ember-template-lint/ember-template-lint ❤️

## Usage

### Direct usage

```js
const ASTLinter = require('./lib/ast-linter'); // adapt path as needed

const linter = new ASTLinter();
const template = fs.readFileSync('some/path/to/template.hbs', {encoding: 'utf8'});
const results = linter.verify({source: template, moduleId: 'template.hbs'});
```

`results` will be an array of objects which have the following properties:
* `rule` - The name of the rule that triggered this warning/error.
* `message` - The message that should be output.
* `line` - The line on which the error occurred.
* `column` - The column on which the error occurred.
* `moduleId` - The module path for the file containing the error.
* `source` - The source that caused the error.
* `fix` - An object describing how to fix the error.
2 changes: 2 additions & 0 deletions lib/ast-linter/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
module.exports = require('./linter');
module.exports.Rule = require('./rules/base');
94 changes: 94 additions & 0 deletions lib/ast-linter/linter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
const Handlebars = require('handlebars');
const defaultRules = require('./rules');

class Linter {
constructor(_options) {
const options = _options || {};

this.options = options;
this.constructor = Linter;
}

buildScanner(config) {
const nodeHandlers = [];

for (const ruleName in config.rules) {
let Rule = config.rules[ruleName];
let rule = new Rule({
name: ruleName,
log: config.log,
source: config.source
});

nodeHandlers.push({
rule,
visitor: rule.getVisitor()
});
}

function Scanner() {}
Scanner.prototype = new Handlebars.Visitor();

const nodeTypes = [
'Program',
'MustacheStatement',
'BlockStatement',
'PartialStatement',
'PartialBlockStatement'
// the following types are not used in Ghost or we don't validate
// 'ContentStatement',
// 'CommentStatement,
// 'Decorator',
// 'DecoratorBlock'
];

nodeTypes.forEach((nodeType) => {
Scanner.prototype[nodeType] = function (node) {
nodeHandlers.forEach((handler) => {
if (handler.visitor[nodeType]) {
handler.visitor[nodeType].call(handler.rule, node);
}
});

Handlebars.Visitor.prototype[nodeType].call(this, node);
};
});

return new Scanner();
}

verify(options) {
const messages = [];

function addToMessages(_message) {
let message = Object.assign({}, {moduleId: options.moduleId}, _message);
messages.push(message);
}

const scannerConfig = {
rules: options.rules || defaultRules,
log: addToMessages,
source: options.source
};

const scanner = this.buildScanner(scannerConfig);
let ast;

try {
ast = Handlebars.parse(options.source);
} catch (err) {
addToMessages({
message: err.message,
fatal: true,
column: err.column,
line: err.lineNumber
});
}

scanner.accept(ast);

return messages;
}
}

module.exports = Linter;
67 changes: 67 additions & 0 deletions lib/ast-linter/rules/base.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
module.exports = class BaseRule {
constructor(options) {
this.ruleName = options.name;
this._log = options.log;
this.source = options.source;
}

getVisitor() {
let visitor = {};
let ruleVisitor = this.visitor();

for (const key in ruleVisitor) {
visitor[key] = ruleVisitor[key];
}

return visitor;
}

// rules will extend this function
visitor() {}

log(result) {
const defaults = {
rule: this.ruleName
};

const reportedResult = Object.assign({}, defaults, result);

this._log(reportedResult);
}

// mostly copy/pasta from tildeio/htmlbars with a few tweaks:
// https://github.com/tildeio/htmlbars/blob/v0.14.17/packages/htmlbars-syntax/lib/parser.js#L59-L90
sourceForNode(node) {
if (!node.loc) {
return;
}

let source = this.source.split('\n');
let firstLine = node.loc.start.line - 1;
let lastLine = node.loc.end.line - 1;
let currentLine = firstLine - 1;
let firstColumn = node.loc.start.column;
let lastColumn = node.loc.end.column;
let string = [];
let line;

while (currentLine < lastLine) {
currentLine += 1;
line = source[currentLine];

if (currentLine === firstLine) {
if (firstLine === lastLine) {
string.push(line.slice(firstColumn, lastColumn));
} else {
string.push(line.slice(firstColumn));
}
} else if (currentLine === lastLine) {
string.push(line.slice(0, lastColumn));
} else {
string.push(line);
}
}

return string.join('');
}
};
4 changes: 4 additions & 0 deletions lib/ast-linter/rules/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module.exports = {
'no-img-url-in-conditionals': require('./lint-no-img-url-in-conditionals'),
'no-multi-param-conditionals': require('./lint-no-multi-param-conditionals')
};
35 changes: 35 additions & 0 deletions lib/ast-linter/rules/lint-no-img-url-in-conditionals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// https://github.com/TryGhost/gscan/issues/85

const Rule = require('./base');
const message = 'The {{img_url}} helper should not be used as a parameter to {{#if}} or {{#unless}}';

// invalid:
// {{#if img_url feature_image}}

module.exports = class NoMultiParamConditionals extends Rule {
_checkForImgUrlParam(node) {
const isConditional = node.path.original === 'if' || node.path.original === 'unless';
const hasImgUrlParam = node.params[0].original === 'img_url';
let fix;

if (isConditional && hasImgUrlParam) {
if (node.params[1]) {
fix = `Remove the 'img_url' so your conditional looks like {{#${node.path.original} ${node.params[1].original}}}`;
}

this.log({
message,
line: node.loc && node.loc.start.line,
column: node.loc && node.loc.start.column,
source: this.sourceForNode(node),
fix
});
}
}

visitor() {
return {
BlockStatement: this._checkForImgUrlParam
};
}
};
34 changes: 34 additions & 0 deletions lib/ast-linter/rules/lint-no-multi-param-conditionals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// https://github.com/TryGhost/gscan/issues/85

const Rule = require('./base');
const message = 'Multiple params are not supported in an {{if}} or {{unless}} statement.';

// valid:
// {{#if foo}}
// {{#unless foo}}

// invalid:
// {{#if foo bar}}
// {{#unless foo bar}}

module.exports = class NoMultiParamConditionals extends Rule {
_checkForMultipleParams(node) {
const isConditional = node.path.original === 'if' || node.path.original === 'unless';
const hasTooManyParams = node.params.length > 1;

if (isConditional && hasTooManyParams) {
this.log({
message,
line: node.loc && node.loc.start.line,
column: node.loc && node.loc.start.column,
source: this.sourceForNode(node)
});
}
}

visitor() {
return {
BlockStatement: this._checkForMultipleParams
};
}
};
17 changes: 17 additions & 0 deletions lib/ast-linter/test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// TODO: delete this file, put proper tests in place
const Linter = require('./');

const linter = new Linter();
const template = `
{{test}}
{{#if img_url feature_image}}
{{feature_image}}
{{/if}}
`;

const messages = linter.verify({
source: template,
moduleId: 'test.hbs'
});

console.log(JSON.stringify(messages)); // eslint-disable-line no-console

0 comments on commit 361214a

Please sign in to comment.