src: make `global` non-enumerable #10405

ljharb · 2016-12-22T03:36:12Z

Per https://github.com/tc39/proposal-global

This is my first PR to node core, so I'm putting it up now without a completed checklist. Early feedback is welcome, and I'll be moving through the items as quickly I can.

Checklist

make -j4 test (UNIX), ~~or vcbuild test (Windows)~~ passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

the JS environment

Description of change

Per the stage 3 proposal, global should be non-enumerable (but writable and configurable). This PR alters the property descriptor to be so.

(If there's a better place this should be done, a pointer to it is most welcome)

Per https://github.com/tc39/proposal-global, the global value `global` should be configurable, writable, and non-enumerable. `node` has always provided it as configurable, writable, and enumerable. The plan is to make it non-enumerable, and barring strong evidence that node can not ship `global` as non-enumerable, this will be how it lands as stage 4 (in the spec).

Fishrock123 · 2016-12-22T04:14:45Z

@ljharb is there a summary on why it's not enumerable? I think it currently is here which mean this should probably be considered a breaking change...?

Trott · 2016-12-22T04:23:33Z

@Fishrock123 Object.keys(global) returns an array containing the string 'global' in Node.js 7.3.0, but with this change, it won't. So yeah, semver major. Labeling as such. Would be interesting to know if this actually breaks any code in the wild. CITGM run would be interesting.

Trott · 2016-12-22T04:33:02Z

@ljharb Here's a minimal test you can add to test/parallel/test-global-descriptors.js:

'use strict';
require('../common');

const assert = require('assert');

assert(!Object.keys(global).includes('global'),
       'global should be non-enumerable');

Adding additional tests to that for writable and configurable and whatever else needs to be set correctly to be spec-compliant would be a good thing, of course.

ljharb · 2016-12-22T04:59:24Z

Thanks to you both; I'll update the commit message as well as add a test.

@Fishrock123 it's non-enumerable because things shouldn't be enumerable unless there's a good reason for there to be so - the proposal was made stage 3 with the understanding that if node decided making it non-enumerable was untenable, that the spec would change to make it enumerable.

I think semver-major is appropriate, although I very much doubt the change will actually break anyone.

not-an-aardvark · 2016-12-22T06:16:33Z

lib/internal/bootstrap_node.js

@@ -190,6 +190,12 @@
  }

  function setupGlobalVariables() {
+    Object.defineProperty(global, 'global', {


By the time this code is run, the global property of the global object is already defined, and this line redefines it to be non-enumerable. This works, but I'm wondering if there's a less roundabout way to do it. (I assume it would be related to this, but I'm not sure about the specifics of how to make a property non-enumerable from the C++ side.)

Thanks, I can explore that file, thanks for the pointer.

(I'd definitely prefer to define it as non-enumerable in C++ instead, to be sure)

k, figured this out, new commit incoming

silverwind · 2016-12-22T06:22:17Z

Enumerability makes it easier for modules like https://github.com/sindresorhus/globals to discover the available globals, just saying.

not-an-aardvark · 2016-12-22T06:29:00Z

@silverwind This only applies to one property: the global property of the global object, which happens to be self-referential. for (foo in global) {} will still return all the global keys that it did before, with the exception of the string 'global'.

As far as libraries like https://github.com/sindresorhus/globals are concerned, I assume they use Object.getOwnPropertyNames(global) to get non-enumerable global variables. There are a lot of global variables (e.g. Number) that are already non-enumerable.

ljharb · 2016-12-22T06:46:11Z

@silverwind indeed, this doesn't affect the enumerability of all global variables, just of the global variable global - and since there's already lots of non-enumerable globals, you'd need Object.getOwnPropertyNames anyways to get reliable results.

silverwind · 2016-12-22T14:13:34Z

Okay, sounds good.

Fishrock123 · 2016-12-22T16:14:03Z

Object.keys(global) returns an array containing the string 'global' in Node.js 7.3.0, but with this change, it won't. So yeah, semver major.

Ummm. I'm not ok with that?? I've used this for debugging certain things and finding things before... and I can see myself needing it again in the future...

...

it's non-enumerable because things shouldn't be enumerable unless there's a good reason for there to be so - the proposal was made stage 3 with the understanding that if node decided making it non-enumerable was untenable, that the spec would change to make it enumerable.

It would be unchangeable.

Object.getOwnPropertyNames

This is what I don't get, it is only not "enumerable" by Object.keys()? I never can remember what that property actually refers to.

ljharb · 2016-12-22T17:00:45Z

"Enumerable" determines whether it shows up in for..in, Object.assign, Object.keys/values/entries, etc. Object.getOwnPropertyNames, Reflect.ownKeys, and Object.getOwnPropertyDescriptors are the only ones that give you non-enumerable values.

Note that I said the spec would be changeable - the assumption is that if this PR were merged, that would constitute evidence that node is willing to make this change. Separately, it is not a breaking change to make a non-enumerable thing enumerable, it's a semver-minor, so it's definitely changeable anyways.

Trott · 2016-12-22T17:13:45Z

Ummm. I'm not ok with that?? I've used this for debugging certain things and finding things before... and I can see myself needing it again in the future...

Can you explain how this change would cause problems when using Object.keys(global) in debugging? Currently, Object.keys(global) returns this:

[ 'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'console',
  'module',
  'require' ]

With the proposed change here, it will instead return this:

[ 'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'console',
  'module',
  'require' ]

ljharb · 2016-12-22T21:27:54Z

Updated to set the property as non-enumerable from C++, rather than having to change it in JS. (make etc still passes)

Happy to squash the commits upon review approval.

ljharb · 2016-12-22T21:28:38Z

src/node.cc

  // (Allows you to set stuff on `global` from anywhere in JavaScript.)
-  global->Set(FIXED_ONE_BYTE_STRING(env->isolate(), "global"), global);
+  NONENUMERABLE_PROPERTY(global, "global", global);


global global global

Trott · 2016-12-22T21:42:04Z

Very minor note: Now that the change is in a C++ file in src rather than a JS file in lib, I changed the lib: at the start of the PR name to src: . That's (probably) how the commit message should start too.

ljharb · 2016-12-22T21:44:06Z

@Trott makes sense; when I squash post-approval pre-merge i'll update the commit message to say src instead of lib

cjihrig

LGTM. This introduces a slight naming inconsistency with the existing READONLY_DONT_ENUM_PROPERTY, although I personally prefer NONENUMERABLE_PROPERTY.

ljharb · 2016-12-22T23:18:58Z

@cjihrig happy to rename READONLY_DONT_ENUM_PROPERTY to READONLY_NONENUMERABLE_PROPERTY if that would help :-)

ljharb · 2016-12-22T23:30:10Z

commits squashed.

not-an-aardvark

LGTM aside from a nitpick with the test

not-an-aardvark · 2016-12-22T23:54:44Z

test/parallel/test-global-descriptors.js

+  writable
+} = Object.getOwnPropertyDescriptor(global, 'global');
+
+assert.strictEqual(value, global, 'global should be global object');


This assertion isn't actually verifying that global is the global object. (If there was a bug where global was set to some other value with a self-referential global property, the assertion would still pass because it's just comparing (the global object).global to (the global object).global.global.)

I think it would be better to compare global to something that is guaranteed to be the global object. For example:

assert.strictEqual(value, (0, eval)('this'), 'global should be global object');

edit: reworded for clarity

Good point, I'll change to Function('return this')() (i assume we're not worried about Function being overwritten in tests, or else i could use function () {}.constructor('return this')())

I think Function('return this')() is fine. (Technically, function () {}.constructor('return this')() could also cause a problem if Function.prototype.constructor was overridden, but I don't think we need to worry about that here.)

In that case constructor would be an own property on the function, and thus would shadow anything on Function.prototype, but I agree we don't need to worry about it :-)

targos · 2016-12-23T17:33:39Z

FWIW I created a CL to implement this in V8: https://codereview.chromium.org/2599903003

joaocgreis · 2016-12-23T19:41:52Z

@ljharb The arm hook is a known problem, but unrelated to arm-fanned. arm-fanned was red and updated.

ljharb · 2016-12-24T06:14:51Z

So does semver-major here mean it won't land in 7.x, but might in 8.x? Or does it mean it won't land until 9.x?

Trott · 2016-12-24T06:21:05Z

So does semver-major here mean it won't land in 7.x, but might in 8.x? Or does it mean it won't land until 9.x?

It means it might land in 8.x.

not-an-aardvark · 2016-12-24T06:25:36Z

To clarify, assuming we reach consensus that this is a good idea, this PR will probably be merged and land on master fairly soon. But it won't make it into a release until Node 8 is released in April.

Trott · 2016-12-24T06:28:35Z

@ljharb Two more things of note:

You probably know this, but just in case: v8.x is scheduled for an April 2017 release. (I see @not-an-aardvark has just posted this same info. STAYIN' ON TOP OF THE BALL!!!!)
An item has been put on the CTC agenda that (IMO at least) calls for the CTC to more clearly define what semver-major means to us. See my attempt to summarize the issue at timers: cleanup extraneous property on Immediates #10205 (comment). At least in theory, depending on how that conversation goes, this may not be considered semver-major after all. I don't expect that to be the outcome, so ultimately that will probably have no effect on this, but you strike me as someone who might be a "keep track of all the moving parts" kind of person, so if so, there's one more thing to keep an eye on.

ljharb · 2016-12-24T06:28:56Z

@not-an-aardvark what consensus do you envision is required? Node representatives at TC39 in September agreed that unless there was concrete evidence of code breakage, that it would be acceptable - that helped allow the proposal to go to stage 3.

node 8.x in April seems fine, thanks for clarifying.

jasnell · 2016-12-24T06:32:55Z

Yep, I don't see any reason why this change cannot land but given the nature of the change, semver-major is appropriate. As @not-an-aardvark and @Trott point out, that means that it can land in master at any time really but won't go into a release until 8.0.0 in April.

Trott · 2016-12-24T06:35:41Z

what consensus do you envision is required?

The absolute rock-bottom minimum would be:

48 hour wait period for people to have a chance to object (longer on weekends and, at least unofficially, major international holidays)
Approval from at least two CTC members
No objections from any Collaborators
Passing CI

Covering these each in turn:

The 48 hours has passed, but given the time of year, it may be prudent to wait at least until next week and probably until early January before landing something like this.
You have the approval of @jasnell and @cjihrig, so you're covered on the "at least two CTC members" front.
The only objection so far is from @Fishrock123 but I think it was based on a misunderstanding. Worst case scenario if that can't be sorted out in this issue is a ctc-agenda label gets stuck on the issue and it gets sorted out at a meeting.
Ci is passing on this.

Again, the above is the absolute minimum. More consensus is always better, of course.

Trott · 2016-12-24T06:42:04Z

@Fishrock123 wrote:

Ummm. I'm not ok with that?? I've used this for debugging certain things and finding things before... and I can see myself needing it again in the future...

Re-reading what I wrote (that the above was a response to), I see that I phrased it badly. I made it sound like Object.keys(global) will no longer return an array. It will still return an array of strings. The only change will be that the string global will no longer be one of them. In other words, the global.global circular reference will no longer be listed among the keys returned by Object.keys(global).

addaleax

LGTM and +1 on considering this semver-major

addaleax · 2016-12-25T18:04:34Z

test/parallel/test-global-descriptors.js

+  writable
+} = Object.getOwnPropertyDescriptor(global, 'global');
+
+var actualGlobal = Function('return this')();


tiny nit: This could be const, right?

ljharb · 2017-01-06T00:05:50Z

Merging this should be withheld, pending the outcome of the issues reported in tc39/proposal-global#20

Trott · 2017-01-06T00:09:16Z

I added a blocked label based on @ljharb's comment.

jasnell · 2017-03-22T22:10:23Z

@ljharb ... where are we at on this?

ljharb · 2017-03-22T22:33:26Z

@jasnell It seems like global is a no-go, and we need to find a new top-level global. In which case, I don't think there's any point in making global non-enumerable.

I'll open a new PR in the future when we've settled on the new top-level variable name.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 22, 2016

ljharb mentioned this pull request Dec 22, 2016

Path to Stage 4! tc39/proposal-global#12

Closed

31 tasks

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 22, 2016

not-an-aardvark reviewed Dec 22, 2016

View reviewed changes

ljharb force-pushed the global branch from e30e7a1 to ca5174e Compare December 22, 2016 07:35

ljharb changed the title ~~update: ensure proper property descriptor on global~~ lib: make global non-enumerable Dec 22, 2016

ljharb force-pushed the global branch from ca5174e to c7d52cf Compare December 22, 2016 21:27

ljharb commented Dec 22, 2016

View reviewed changes

Trott changed the title ~~lib: make global non-enumerable~~ src: make global non-enumerable Dec 22, 2016

cjihrig approved these changes Dec 22, 2016

View reviewed changes

ljharb force-pushed the global branch from c7d52cf to 21538fd Compare December 22, 2016 23:28

not-an-aardvark suggested changes Dec 22, 2016

View reviewed changes

ljharb force-pushed the global branch from 21538fd to 11bc46d Compare December 23, 2016 00:15

jasnell approved these changes Dec 23, 2016

View reviewed changes

ljharb force-pushed the global branch from 11bc46d to db9b913 Compare December 25, 2016 05:56

addaleax approved these changes Dec 25, 2016

View reviewed changes

ljharb force-pushed the global branch from db9b913 to a86b691 Compare December 25, 2016 19:48

addaleax added this to the 8.0.0 milestone Dec 26, 2016

ljharb force-pushed the global branch from a86b691 to 5a2b744 Compare December 27, 2016 06:45

addaleax mentioned this pull request Dec 28, 2016

lib: be robust when process global is clobbered #10135

Closed

ljharb force-pushed the global branch 3 times, most recently from 6f761fd to 4744e3f Compare January 4, 2017 07:27

Trott added the blocked PRs that are blocked by other issues or PRs. label Jan 6, 2017

ljharb closed this Mar 22, 2017

ljharb deleted the global branch March 22, 2017 22:33

ljharb restored the global branch August 5, 2017 02:59

TimothyGu mentioned this pull request May 7, 2018

Inconsistent enumerability #20565

Closed

ljharb mentioned this pull request Sep 13, 2018

src: add globalThis #22835

Closed

4 tasks

src: make global non-enumerable #10405

src: make global non-enumerable #10405

Conversation

ljharb commented Dec 22, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Fishrock123 commented Dec 22, 2016

Trott commented Dec 22, 2016 • edited Loading

Trott commented Dec 22, 2016

ljharb commented Dec 22, 2016

Choose a reason for hiding this comment

ljharb Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silverwind commented Dec 22, 2016

not-an-aardvark commented Dec 22, 2016 • edited Loading

ljharb commented Dec 22, 2016 • edited Loading

silverwind commented Dec 22, 2016

Fishrock123 commented Dec 22, 2016 • edited Loading

ljharb commented Dec 22, 2016

Trott commented Dec 22, 2016 • edited Loading

ljharb commented Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Trott commented Dec 22, 2016

ljharb commented Dec 22, 2016 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

ljharb commented Dec 22, 2016

ljharb commented Dec 22, 2016

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Dec 23, 2016

joaocgreis commented Dec 23, 2016

ljharb commented Dec 24, 2016

Trott commented Dec 24, 2016

not-an-aardvark commented Dec 24, 2016

Trott commented Dec 24, 2016

ljharb commented Dec 24, 2016

jasnell commented Dec 24, 2016

Trott commented Dec 24, 2016 • edited Loading

Trott commented Dec 24, 2016 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jan 6, 2017

Trott commented Jan 6, 2017

jasnell commented Mar 22, 2017

ljharb commented Mar 22, 2017

src: make `global` non-enumerable #10405

src: make `global` non-enumerable #10405

ljharb commented Dec 22, 2016 •

edited

Loading

Trott commented Dec 22, 2016 •

edited

Loading

ljharb Dec 22, 2016 •

edited

Loading

not-an-aardvark commented Dec 22, 2016 •

edited

Loading

ljharb commented Dec 22, 2016 •

edited

Loading

Fishrock123 commented Dec 22, 2016 •

edited

Loading

Trott commented Dec 22, 2016 •

edited

Loading

ljharb commented Dec 22, 2016 •

edited

Loading

ljharb commented Dec 22, 2016 •

edited

Loading

not-an-aardvark Dec 22, 2016 •

edited

Loading

Trott commented Dec 24, 2016 •

edited

Loading

Trott commented Dec 24, 2016 •

edited

Loading