Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: parsingContext option for vm.compileFunction is a little unclear #23194

Closed
darahayes opened this issue Oct 1, 2018 · 8 comments
Closed

Comments

@darahayes
Copy link

  • Version: v10.10.0
  • Platform: All Platforms
  • Subsystem: vm

Hi there, perhaps this is not an issue. If so feel free to close. I've been playing around with the vm API and I found that the compileFunction docs were a little unclear.

There is an option called parsingContext and its description is <Object> The sandbox/context in which the said function should be compiled in.

I had two problems with this.

  • I initially thought the parsingContext might be a plain object, but it's actually supposed to be a Context object. The reason I got confused with this is because similar options for other functions in the vm API explicitly say the object is contextified. See the runInContext docs as an example. Perhaps the description could be updated to something like <Object> The contextified sandbox in which the said function should be compiled in.
  • I'm not fully sure of how the context changes the behaviour of the function (if at all). I initially thought It was similar to how runInContext works, i.e. whatever I put in the context, will be available globally inside the code. Take this example
const vm = require('vm')

const parsingContext = vm.createContext({name: 'world'})

const code = `return 'hello ' + name`

const fn = vm.compileFunction(code, [], { parsingContext })

console.log(fn())

I thought perhaps the name property would be available inside the function at runtime but I guess I'm wrong. Instead I see this error:

:1
return 'hello ' + name
                  ^

ReferenceError: name is not defined
    at <anonymous>:1:19
    at Object.<anonymous> (/Users/dara/mydev/vmtest/index.js:9:13)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:696:3)

Based off the description, I guess the context somehow comes into play at compile time as opposed to run time, but I'm not 100% sure how it affects the code.

I'd be super happy to open a docs PR to clarify this if I could get a little help. Cheers!

@targos
Copy link
Member

targos commented Oct 1, 2018

@nodejs/vm

@ryzokuken
Copy link
Contributor

Hi, @darahayes 👋

I added compileFunction in #21571, and while a lot of back-and-forth attention was given to the documentation, looks like I inadvertently missed a few clarifications.

@hashseed @addaleax do you think we should pass parsing_context instead of context as a parameter in v8::ScriptCompiler::CompileFunctionInContext at https://github.com/nodejs/node/pull/21571/files#diff-0cf206672499c2f86db4ffb0cc5b668bR1093? In case this counts as a bug, that'll probably be the fix. That said, we are using parsing_context to initialize to Context::Scope in use.

@darahayes
Copy link
Author

darahayes commented Oct 1, 2018

Hey @ryzokuken yeah it looks like we're creating context from the current context at the beginning and then checking if a parsing_context was supplied and if it wasn't then we're setting parsing_context = context; here

I'm not familiar with this code but it looks like perhaps we should be passing parsing_context as opposed to context, or doing something like context = parsing_context; in the correct conditions.

That might explain the behaviour I see in the example I gave above? I could be totally wrong but I'll try out the change locally and see what happens. It'll be a learning experience for me either way 😊

@darahayes
Copy link
Author

darahayes commented Oct 1, 2018

So just a follow up. I cloned Node and changed the following line:

context, &source, params.size(), params.data(),
to pass in parsing_context instead of context and tested against two code examples.

Code Example 1

const vm = require('vm')

const parsingContext = vm.createContext({name: 'world'})

const code = `return 'hello ' + name`

const fn = vm.compileFunction(code, [], { parsingContext })

console.log(fn())

Code Example 2

const vm = require('vm')

const name = 'world'
const parsingContext = vm.createContext({ foo: 'bar' })

const code = `return global`

const fn = vm.compileFunction(code, [], { parsingContext })

console.log(global === fn())

Results on Node v10.10.0

Example 1

I get the following result:

:1
return 'hello ' + name
                  ^

ReferenceError: name is not defined
    at <anonymous>:1:19
    at Object.<anonymous> (/Users/dara/mydev/vmtest/index.js:13:13)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:696:3)

Question: Is this expected behaviour?

Example 2

I get true because the compiled function has access to the same global object as my actual process. I would have expected global to be undefined inside the compiled function. Is this a bug or am I not understanding it properly?

Results on locally modified Node (using parsing_context instead of context)

Example 1

I get the following result.

hello world

I would have expected this.

Example 2

I get the following result:

:1
return global
^

ReferenceError: global is not defined
    at <anonymous>:1:1
    at Object.<anonymous> (/Users/dara/mydev/vmtest/index.js:10:24)
    at Module._compile (internal/modules/cjs/loader.js:703:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:714:10)
    at Module.load (internal/modules/cjs/loader.js:613:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:552:12)
    at Function.Module._load (internal/modules/cjs/loader.js:544:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:756:12)
    at startup (internal/bootstrap/node.js:303:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:865:3)

I would have also expected this because we're explicitly providing a new context which right?

It seems to me that all of the code leading up to the call to v8::ScriptCompiler::CompileFunctionInContext is gearing up to pass parsing_context in. For example like the Context::Scope(parsing_context) call @ryzokuken mentioned.

So we potentially have a bug and this might be the fix. If that's the case, I would be more than happy to go through the process of submitting a PR 😊

I am still unsure of one thing though. What should be in the scope of the function, when parsing_context is undefined?

For example, if we modify the code to look like this:

const vm = require('vm')

const name = 'world'

const code = `return 'hello ' + name`

const fn = vm.compileFunction(code)

console.log(fn())

In this case we aren't passing in any context so the current one should be used right?

Should the compiled function have access to variables defined in the current scope? That is, should it have access to the local name variable? Right now it doesn't in Node v10.10.0 or in my modified version. Thanks for your help!

@devsnek
Copy link
Member

devsnek commented Oct 1, 2018

compiled functions won't have access to any lexically declared variables because they don't exist in any lexical scope. the contextExtensions option might help

@darahayes
Copy link
Author

@devsnek thanks for that clarification. I suppose that clears up the one thing I was confused about. If I'm understanding things properly though, we still may have found a bug (parsing_context vs. context)

@ryzokuken
Copy link
Contributor

@darahayes

In this case we aren't passing in any context so the current one should be used right?

Yes, that is correct.

So we potentially have a bug and this might be the fix. If that's the case, I would be more than happy to go through the process of submitting a PR 😊

I do believe that this was an oversight on my part when building this behavior and since your results confirm that it's fixed with the changes, I think it's a good idea to make a PR for the same. Thank you for your work.

@darahayes
Copy link
Author

@ryzokuken thanks so much for your help. I've opened a PR which I hope is useful :)

darahayes pushed a commit to darahayes/node that referenced this issue Oct 9, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: nodejs#23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction
@danbev danbev closed this as completed in 8da674d Oct 11, 2018
targos pushed a commit that referenced this issue Oct 12, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: #23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: #23206
Fixes: #23194
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this issue Oct 17, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: #23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: #23206
Fixes: #23194
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 30, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: #23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: #23206
Fixes: #23194
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this issue Nov 28, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: #23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: #23206
Fixes: #23194
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
ContextifyContext::CompileFunction in src/node_contextify.cc
was incorrectly passing the context variable to
ScriptCompiler::CompileFunctionInContext

This meant that the parsingContext option in vm.compileFunction
was not being applied properly to the compiled function.

fixes: #23194

doc: clarify parsingContext option for vm.compileScript

test: usage of parsingContext in vm.compileFunction

PR-URL: #23206
Fixes: #23194
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants