From 3f7f643816ebe2afa52aa79ab01b40a3ba8c30f5 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 12 Apr 2017 11:23:08 -0400 Subject: [PATCH] enforce helper function purity --- src/validate/index.js | 44 +++++++++++-------- src/validate/js/propValidators/helpers.js | 36 +++++++++++++++ .../input.html | 11 +++++ .../warnings.json | 8 ++++ .../helper-purity-check-no-this/errors.json | 8 ++++ .../helper-purity-check-no-this/input.html | 11 +++++ .../input.html | 13 ++++++ .../warnings.json | 1 + 8 files changed, 114 insertions(+), 18 deletions(-) create mode 100644 test/validator/samples/helper-purity-check-needs-arguments/input.html create mode 100644 test/validator/samples/helper-purity-check-needs-arguments/warnings.json create mode 100644 test/validator/samples/helper-purity-check-no-this/errors.json create mode 100644 test/validator/samples/helper-purity-check-no-this/input.html create mode 100644 test/validator/samples/helper-purity-check-uses-arguments/input.html create mode 100644 test/validator/samples/helper-purity-check-uses-arguments/warnings.json diff --git a/src/validate/index.js b/src/validate/index.js index f0a274c9eb..d83161168e 100644 --- a/src/validate/index.js +++ b/src/validate/index.js @@ -18,7 +18,7 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file error.toString = () => `${error.message} (${error.loc.line}:${error.loc.column})\n${error.frame}`; - onerror( error ); + throw error; }, warn: ( message, pos ) => { @@ -45,25 +45,33 @@ export default function validate ( parsed, source, { onerror, onwarn, name, file helpers: {} }; - if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) { - const error = new Error( `options.name must be a valid identifier` ); - onerror( error ); - } + try { + if ( name && !/^[a-zA-Z_$][a-zA-Z_$0-9]*$/.test( name ) ) { + const error = new Error( `options.name must be a valid identifier` ); + throw error; + } - if ( name && !/^[A-Z]/.test( name ) ) { - const message = `options.name should be capitalised`; - onwarn({ - message, - filename, - toString: () => message - }); - } + if ( name && !/^[A-Z]/.test( name ) ) { + const message = `options.name should be capitalised`; + onwarn({ + message, + filename, + toString: () => message + }); + } - if ( parsed.js ) { - validateJs( validator, parsed.js ); - } + if ( parsed.js ) { + validateJs( validator, parsed.js ); + } - if ( parsed.html ) { - validateHtml( validator, parsed.html ); + if ( parsed.html ) { + validateHtml( validator, parsed.html ); + } + } catch ( err ) { + if ( onerror ) { + onerror( err ); + } else { + throw err; + } } } diff --git a/src/validate/js/propValidators/helpers.js b/src/validate/js/propValidators/helpers.js index 6553f07972..8248445d44 100644 --- a/src/validate/js/propValidators/helpers.js +++ b/src/validate/js/propValidators/helpers.js @@ -1,5 +1,6 @@ import checkForDupes from '../utils/checkForDupes.js'; import checkForComputedKeys from '../utils/checkForComputedKeys.js'; +import { walk } from 'estree-walker'; export default function helpers ( validator, prop ) { if ( prop.value.type !== 'ObjectExpression' ) { @@ -9,4 +10,39 @@ export default function helpers ( validator, prop ) { checkForDupes( validator, prop.value.properties ); checkForComputedKeys( validator, prop.value.properties ); + + prop.value.properties.forEach( prop => { + if ( !/FunctionExpression/.test( prop.value.type ) ) return; + + let lexicalDepth = 0; + let usesArguments = false; + + walk( prop.value.body, { + enter ( node ) { + if ( /^Function/.test( node.type ) ) { + lexicalDepth += 1; + } + + else if ( lexicalDepth === 0 ) { + if ( node.type === 'ThisExpression' ) { + validator.error( `Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?`, node.start ); + } + + else if ( node.type === 'Identifier' && node.name === 'arguments' ) { + usesArguments = true; + } + } + }, + + leave ( node ) { + if ( /^Function/.test( node.type ) ) { + lexicalDepth -= 1; + } + } + }); + + if ( prop.value.params.length === 0 && !usesArguments ) { + validator.warn( `Helpers should be pure functions, with at least one argument`, prop.start ); + } + }); } diff --git a/test/validator/samples/helper-purity-check-needs-arguments/input.html b/test/validator/samples/helper-purity-check-needs-arguments/input.html new file mode 100644 index 0000000000..244c67e697 --- /dev/null +++ b/test/validator/samples/helper-purity-check-needs-arguments/input.html @@ -0,0 +1,11 @@ +{{foo()}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-needs-arguments/warnings.json b/test/validator/samples/helper-purity-check-needs-arguments/warnings.json new file mode 100644 index 0000000000..85abc4df92 --- /dev/null +++ b/test/validator/samples/helper-purity-check-needs-arguments/warnings.json @@ -0,0 +1,8 @@ +[{ + "message": "Helpers should be pure functions, with at least one argument", + "pos": 54, + "loc": { + "line": 6, + "column": 3 + } +}] \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-no-this/errors.json b/test/validator/samples/helper-purity-check-no-this/errors.json new file mode 100644 index 0000000000..9de3d7465d --- /dev/null +++ b/test/validator/samples/helper-purity-check-no-this/errors.json @@ -0,0 +1,8 @@ +[{ + "message": "Helpers should be pure functions — they do not have access to the component instance and cannot use 'this'. Did you mean to put this in 'methods'?", + "pos": 74, + "loc": { + "line": 7, + "column": 11 + } +}] \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-no-this/input.html b/test/validator/samples/helper-purity-check-no-this/input.html new file mode 100644 index 0000000000..e950152de8 --- /dev/null +++ b/test/validator/samples/helper-purity-check-no-this/input.html @@ -0,0 +1,11 @@ +{{foo()}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-uses-arguments/input.html b/test/validator/samples/helper-purity-check-uses-arguments/input.html new file mode 100644 index 0000000000..559f0ba0c0 --- /dev/null +++ b/test/validator/samples/helper-purity-check-uses-arguments/input.html @@ -0,0 +1,13 @@ +{{sum(1, 2, 3)}} + + \ No newline at end of file diff --git a/test/validator/samples/helper-purity-check-uses-arguments/warnings.json b/test/validator/samples/helper-purity-check-uses-arguments/warnings.json new file mode 100644 index 0000000000..0637a088a0 --- /dev/null +++ b/test/validator/samples/helper-purity-check-uses-arguments/warnings.json @@ -0,0 +1 @@ +[] \ No newline at end of file