src: remove unused env->vm_parsing_context_symbol

[maclover7]

Stopped being used via 77b52fd, was
originally added in d932e80.

For the one remaining usecase inside of lib/vm.js, define a Symbol at
the top of the file.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[nodejs-github-bot]

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/384/pipeline

[TimothyGu]

This is still very much used in https://github.com/nodejs/node/blob/master/lib/vm.js. The way the symbol was used in that file made the tests magically pass (as the string key 'undefined' is used in place).

In that particular file the symbol could just be changed to a regular local one declared with Symbol().

[addaleax]

@TimothyGu But is the property read anywhere? At a glance, it looks like this would be safe to remove from vm.js as well…?

[maclover7]

Ah, sorry, missed the lib/vm.js usecase (got when looking over 77b52fd and read that filename as lib/vm.js by accident)

I agree with @addaleax though that the use in lib/vm.js could be removed -- doing ggrep -r kParsingContext test/ comes up empty, and whatever ggrep -r "new vm.Script(" test/ and ggrep -r "new Script(" . returns doesn't seem to interact with the symbol at all.

[targos]

+1 for removal and using a Symbol() declared in vm.js

[maclover7]

@TimothyGu I updated to use a symbol declared inside vm.js -- does this seem okay to you?

[TimothyGu]

Looks good, thanks!

[maclover7]

CI: https://ci.nodejs.org/job/node-test-pull-request/16166/

[maclover7]

Landed in bd2ee60, thank you for the reviews!

[addaleax]

@maclover7 @TimothyGu Uh … this seems to have gotten lost in the discussion, but still, does anything speak against removing this key completely?

[TimothyGu]

@addaleax The symbol is used to transmit the parsing context information from vm.runInContext and vm.runInNewContext to the Script constructor, without creating a new public option as the Script constructor is public. I'm fine with exposing it as a public option but it met resistance in #14888 (#14888 (comment)).

[targos]

Depends on #21668 to land on v10.x-staging. More specifically this commit: 104953c