src: make global non-enumerable

[ljharb]

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)

[Fishrock123]

@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]

@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]

@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]

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]

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.)

[ljharb]

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

[ljharb]

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

[ljharb]

k, figured this out, new commit incoming

[silverwind]

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

[not-an-aardvark]

@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]

@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]

Okay, sounds good.

[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.

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]

"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]

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]

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]

global global global

[Trott]

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]

@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]

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

[ljharb]

commits squashed.

[not-an-aardvark]

LGTM aside from a nitpick with the test

[not-an-aardvark]

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

[ljharb]

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')())

[not-an-aardvark]

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.)

[ljharb]

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]

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

[joaocgreis]

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

[ljharb]

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]

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]

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]

@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]

@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]

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]

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]

@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]

tiny nit: This could be const, right?

[ljharb]

fixed

[ljharb]

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

[Trott]

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

[jasnell]

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

[ljharb]

@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.