search

browserify / detective

Find all calls to require() no matter how deeply nested using a proper walk of the AST
Other
414 stars 60 forks source link

Error behaviour difference between esprima.parse() and detective.find() #18

Closed bengourley closed 11 years ago

bengourley commented 11 years ago

While tracking down an inconsistency from upgrading to browserify v2, I noticed a discrepancy between what is thrown by esprima.parse() and detective.find():

> esprima.parse('return a')
Error: Line 1: Illegal return statement
    at throwError (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:1161:21)
    at throwErrorTolerant (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:1172:24)
    at parseReturnStatement (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:2476:13)
    at parseStatement (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:2752:24)
    at parseSourceElement (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:3044:24)
    at parseSourceElements (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:3082:29)
    at parseProgram (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:3096:19)
    at Object.parse (/Users/bengourley/node_modules/module-deps/node_modules/detective/node_modules/esprima/esprima.js:3843:23)
    at repl:1:10
    at REPLServer.self.eval (repl.js:110:21)
> detective.find('return a')
{ strings: [], expressions: [] }

I can't see the error being caught anywhere in detective/index.js even though esprima.parse(str) is called directly here.

Now to the thing I was looking in to that brought me to this module. I hope this is the correct module on which to discuss the issue.

In browserify v1 (and also in node) it is ok to return from the main module body, eg:

if (condition) {
  // Don't do what you are about to do
  return
}

// Code here

browserify v2 says that this is an illegal return statement, which is presumably in accordance with ES semantics (in that you can't return if you're not in a function), but this introduces a behavioural difference between the browserify environment and the node environment. What do you think?

Cheers

bengourley commented 11 years ago

After a bit more poking around, I've tracked down the source of the inconsistency to https://github.com/substack/node-detective/blob/master/index.js#L49:

if (src.indexOf(word) == -1) return modules;

This short circuit (presumably for performance, to prevent an unnecessary esprima parse) means that sources with the string 'require' anywhere (including in a comment or within a string) will be parsed, but those that don't won't.

Calling detective on this source throws an error:

// I dont require any modules
return

This doesn't:

return
ghost commented 11 years ago

Fixed in 2.1.1.

bengourley commented 11 years ago

Thanks!