Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

program code is not run in global execution context #6254

Closed
michaelficarra opened this issue Sep 23, 2013 · 5 comments
Closed

program code is not run in global execution context #6254

michaelficarra opened this issue Sep 23, 2013 · 5 comments

Comments

@michaelficarra
Copy link

It appears programs are wrapped in a function expression to introduce its parameters (exports, require, module, __filename, __dirname) into the program's environment before being executed. This has a few unexpected consequences.

  • arguments is introduced into scope
    • arguments.callee exposes FE wrapping
    • arguments.callee.caller exposes execution internals
  • top-level variable/function declarations do not use the global object as their scope object
  • return is allowed at the top level
  • inconsistency with REPL, which behaves correctly in all the above aspects

Tested on latest 0.10.x and 0.11.x releases.

My proposed solution: remove the wrapper function and just run the program in a context with the FE's parameters already defined. This will obviously break programs that depend on any of the above behaviours, but that seems okay since they are observed quirks, not documented or ECMA-262-spec'd features.

@othiym23
Copy link

I apologize if this seems like an obtuse question, but what's the actual problem here? Node modules are already running with the full privileges of the UID they're run under, so it's not like arguments or arguments.callee is exposing anything privileged. Node exposes the internal details of its implementation in many places and relies on developers not shooting themselves in the foot, which works more often than not.

As someone from core will undoubtedly be along to point out before long, the module interface is frozen and is therefore highly unlikely to change in such a major way. In additiion, vm / vm2 are fast but not anywhere as fast as the current approach, which is probably enough of a nail in the coffin for your suggested remedy.

@michaelficarra
Copy link
Author

I would consider pretty much all the things I mentioned to be problems. arguments being added to scope is the least of these, since you can't really assume nonexistence of any variable at the beginning of program execution. I would consider the others pretty significant violations of both ECMA-262 and users' expectations. According to ECMA-262, the top-level scope object should be the global object, and top-level return should cause a SyntaxError.

This program fails, but should not:

var a = true;
if(!global.a) throw new Error;

This program does not fail, but should:

return;

And the REPL behaviour should not differ from that of regular programs. I think we can all agree on those. I also don't think the frozen module interface will be a problem, since I don't think this would require changing the interface.

@othiym23
Copy link

ES-262 and ES5 are red herrings here; Node has never claimed to provide a pure or standard JavaScript environment. If you look into how src/node. js and src/node.cc are implemented, there are all kinds of impure things going on in the interests of simplicity or performance. @bnoordhuis or @isaacs may disagree, but I think the sense of core is that the REPL and debugger disagree with the runtime frequently because they're fulfilling different needs, and when there's a divergence between the two, the runtime behavior should be considered correct. In this case, the REPL is run inside a context to allow it to trap and recover from more errors, at the cost of significant additional overhead.

As for the module system's frozen status, in this case the API is the implementation. Somewhat unfortunately, it's unsafe to assume there aren't any modules depending on the behavior you consider erroneous. Look at some of the closed issues filed on the module system for examples. This is a central part of the runtime, which makes it very hard to change without breaking working code or introducing performance regressions.

@bnoordhuis
Copy link
Member

As someone from core will undoubtedly be along to point out before long, the module interface is frozen and is therefore highly unlikely to change in such a major way.

The module system is frozen and highly unlikely to change in such a major way. Actually, scratch that and replace with "won't ever change that way".

Besides, the current behavior is a feature, not a bug - it nicely encloses the module in a lexical scope. If everything ran in the global context, you'd have no end of symbol clashes. If you run everything in a new context, performance will tank: env NODE_MODULE_CONTEXTS=1 node script.js if you don't believe me.

Closing as WONTFIX. Thanks for the suggestion though.

@michaelficarra
Copy link
Author

I'd like to re-open this issue in the context of ES6 modules. Import and export declarations are required to exist at the top level. However, this conflicts with the current method used to retain backwards compatibility with top-level returns, }()); (function(){, and other non-programs that node accepts: the IIFE wrapper. What are the plans going forward? Is this finally convincing enough to drop support for these ridiculous invalid programs?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants