estools / esvalid Goto Github PK

See #13 for reasoning.

Hi, first thank for your handy module!!
I get a error on the code: a: try { break a; } finally { } which is legal I believe.
Below is a minimal bug reproduction:

var Esprima = require('esprima');
var Esvalid = require('esvalid');

var code = 'a: try { break a; } finally { }';
var ast = Esprima.parse(code);
var errors = Esvalid.errors(ast);
if (errors.length > 0) { throw errors[0]; }

TL;DR: try { esvalid.isValid(esprima.parse(text)) } catch(e) { true } should, intuitively, always evaluate to true. This is about some cases where it doesn't. This is not necessarily a bug report.

I wrote a quick tool to slurp up all the javascript files on my harddrive, run it through esprima and esvalid, select the files that parsed fine but failed the esvalid checks, and automatically minify them. Oh yeah, and unique them by the particular error I get from esvalid. Sorry, I may have had a little too much fun with this. ;)

Here's what I found:

• 10 instances of "unrecognized node type", all of which are ES6-specific: ForOfStatement, ClassDeclaration, TemplateLiteral, TaggedTemplateExpression, Super, YieldExpression, ArrowFunctionExpression, ClassExpression, ObjectPattern, SpreadElement
• 2 instances of ES6 features that aren't explicitly new node types
• 17 instances of fallout from the unrecognized node types (i.e. "cascading failures")
• 4 instances of "identical keys" that the esprima isn't catching, but could
• an instance where esprima doesn't detect a break without a switch, where it could
• 10 instances where function(){} is placed where expressions/statements are expected.

That last case is the most interesting to me. For example: `for(;;)function f(){}", "switch(n){case 1:function f(){}}", etc. Here, I think either:

• Esprima is really generating invalid trees
• The examples are really syntax errors, and esprima is being lenient, not catching some syntax errors it could
• Esvalid is wrong, and that particular syntax tree is perfectly valid.

What are your thoughts?

You can see the full results here: https://gist.github.com/joshuawarner32/994d2756687551d30d1d

This is them as of right now:

• "BreakStatement must have an IterationStatement or SwitchStatement as an ancestor"
• "labelled BreakStatement must have a matching LabeledStatement ancestor"
• "ContinueStatement must have an IterationStatement as an ancestor"
• "labelled ContinueStatement must have a matching LabeledStatement ancestor"
• "Identifier name member must be a valid IdentifierName"
• "Identifier name member must not be a ReservedWord"
• "IfStatement with null alternate must not be the consequent of an IfStatement with a non-null alternate"
• "LabeledStatement must not be nested within a LabeledStatement with the same label"
• "numeric Literal nodes must not be NaN"
• "numeric Literal nodes must not be negative"
• "numeric Literal nodes must be finite"
• "static MemberExpression property member must have a valid IdentifierName name member"
• "ObjectExpression getter property value member must have zero parameters"
• "ObjectExpression setter property value member must have exactly one parameter"
• "duplicate data property in object literal not allowed in strict mode"
• "ReturnStatement must be nested within a FunctionExpression or FunctionDeclaration node"
• "SequenceExpression expressions member length must be >= 2"
• "SwitchStatement cases member length must be >= 1"
• "SwitchStatement cases member must contain no more than one SwitchCase with a null test member"
• "TryStatement must have a non-null handler member or a non-null finalizer member"
• "delete with bare identifier not allowed in strict mode"
• "VariableDeclaration declarations member must be non-empty"

We'll have to add the errors for #23, #24, and #25 when they are done.

These six (copied from above) can be fixed by changing the AST structure:

• "ObjectExpression getter property value member must have zero parameters"
• "ObjectExpression setter property value member must have exactly one parameter"
• "SequenceExpression expressions member length must be >= 2"
• "SwitchStatement cases member length must be >= 1"
• "SwitchStatement cases member must contain no more than one SwitchCase with a null test member"
• "VariableDeclaration declarations member must be non-empty"

I'm writing a program that only needs to check for validity of an AST subtree, so I'm using esvalid.errors(partOfAst) and stripping all errors with the message "given AST node should be of type Program". I've pinned esvalid as a specific version, because I'm unsure whether that's part of the public API.

Is it? Might that human-readable message change in the future?

If I've correctly understood, #33 plans to introduce machine-readable identifiers for the specific error types, which might be a better way to solve this.

Just ES5 support for now, no destructuring.

• function parameters
• setter parameters
• AssignmentExpression.left
• function names
• catch variable
• variable declarations
• prefix/postfix increment/decrement

This is not actually a restriction of SpiderMonkey Literal nodes.

For example:

• Structural Context (SC)
• Duplication Not Allowed (DNA)
• Restricted Names (RN)
• Incorrect Type (IT)
• Nullability (N)

Working on this.

We should be sure to normalise numbers/strings/identifiers in order to compare the keys properly.

In 3 different types for loops in listed in http://es5.github.io/#A.4. This may need to introduce a new state.

https://github.com/michaelficarra/esvalid/blob/8c54e7b3668e1c7734231e21251f51ac4b38c01b/index.js#L363

Should be "Identifier name member must be a valid IdentifierName"

Labels need to exist as ancestors of continue/break.

Hopefully this should be 1 for most of them or at least we can reduce them to 1.

https://github.com/Constellation/esutils

• isReservedWord (estools/esutils#3)
• isExpression, isStatement, isIterationStatement, isSourceElement (estools/esutils#8)
• isAssignmentOperator, isBinaryOperator, isLogicalOperator, isUnaryOperator, isUpdateOperator

Bug introduced in a19be8d.

https://github.com/michaelficarra/esvalid/blob/a57c8c48a335d10eb7e1554c15c887c04103174e/index.js#L269

https://github.com/michaelficarra/esvalid/blob/8c54e7b3668e1c7734231e21251f51ac4b38c01b/index.js#L596

errors.push(new E(node, "SwitchStatement cases member length must be >= 2"));

It shall be ">= 1".

Apparently esvalidate is already taken (https://www.npmjs.org/package/esvalidate) grumble...

For "get", expect zero parameters. For "set", expect one.

The identifiers "implements", "interface", "let", "package", "private", "protected", "public", "static", and "yield" are classified as FutureReservedWord tokens within strict mode code. (7.6.1.2).

Apparently esprima is still generating ASTs like this.

Test case: https://github.com/michaelficarra/esvalid/blob/master/test/unit.js#L147
Spec: http://es5.github.io/#x12.1

An empty SwitchStatement act as if it is a expression statement with only the discriminant.

LeftHandSideExpressions are:
Literal
Identifier
ArrayExpression
ObjectExpression
FunctionExpression
MemberExpression
CallExpression

... and unfortunately all the "GroupExpression".

Note that only MemberExpression and Identifier can be parsed correctly by esprima.

In ES5, they were restricted in strict mode, but ES6 allows them again. Rationale had to do with dynamic property names.

We could also add an isValidExpression, but isValidStatement is a non-starter because it relies on the current label set and whether we're in a function or IterationStatement, which is context we don't have.

Only for nodes types that also exist in JavaScript.