From dcaf20b30459f355cf885b3189f07f0b2eaff253 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Wed, 22 Jun 2016 13:07:59 -0400 Subject: [PATCH 1/5] repl: Enable tab completion for global properties. When `useGlobal` is false, tab completion in the repl does not enumerate global properties. Instead of just setting these properties blindly on the global context, e.g. context[prop] = global[prop] Use `Object.defineProperty` and the property descriptor found on `global` for the new property in `context`. Also addresses a previously unnoticed issue where `console` is writable when `useGlobal` is false. Ref: https://github.com/nodejs/node/issues/7353 --- lib/repl.js | 18 ++++++++++++++---- test/parallel/test-repl-console.js | 3 +++ test/parallel/test-repl-tab-complete.js | 11 +++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index 18c81c438dee91..f636a5fd55db58 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -582,10 +582,20 @@ REPLServer.prototype.createContext = function() { context = global; } else { context = vm.createContext(); - for (var i in global) context[i] = global[i]; - context.console = new Console(this.outputStream); - context.global = context; - context.global.global = context; + const _console = new Console(this.outputStream); + Object.getOwnPropertyNames(global).forEach((name) => { + if (name === 'console') { + Object.defineProperty(context, name, { + configurable: true, + enumerable: true, + value: _console + }); + } + else { + Object.defineProperty(context, name, + Object.getOwnPropertyDescriptor(global, name)); + } + }); } const module = new Module(''); diff --git a/test/parallel/test-repl-console.js b/test/parallel/test-repl-console.js index 609822703fef1e..98cb958cac8e68 100644 --- a/test/parallel/test-repl-console.js +++ b/test/parallel/test-repl-console.js @@ -18,3 +18,6 @@ assert(r.context.console); // ensure that the repl console instance is not the global one assert.notStrictEqual(r.context.console, console); + +// ensure that the repl console instance does not have a setter +assert.throws(() => r.context.console = 'foo', TypeError); diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index d92377efebf791..97e5bacfc86b76 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -267,3 +267,14 @@ putIn.run(['.clear']); testMe.complete('.b', common.mustCall((error, data) => { assert.deepStrictEqual(data, [['break'], 'b']); })); + +var testNonGlobal = repl.start({ + input: putIn, + output: putIn, + useGlobal: false +}); + +testNonGlobal.complete('I', common.mustCall((error, data) => { + assert.deepStrictEqual(data, [['Infinity', '', 'Int16Array', 'Int32Array', + 'Int8Array', 'Intl'], 'I']); +})); From 12902ddd20ca2e2c70798baeff9d08d91d45db66 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Thu, 23 Jun 2016 16:40:51 -0400 Subject: [PATCH 2/5] repl: Do not copy builtin properties to context. When `useGlobal` is false, all of the properties on `global` are copied to the new context (excluding `console` and `global`). Since `Object.getOwnPropertyNames()` returns all properties, not just enumerable ones, builtin properties on `global` are returned. We need to filter those out. --- lib/repl.js | 43 ++++++++++++++----------- test/parallel/test-repl-context.js | 33 +++++++++++++++++++ test/parallel/test-repl-tab-complete.js | 2 +- 3 files changed, 58 insertions(+), 20 deletions(-) create mode 100644 test/parallel/test-repl-context.js diff --git a/lib/repl.js b/lib/repl.js index f636a5fd55db58..541e9f705aa448 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -39,6 +39,16 @@ const debug = util.debuglog('repl'); const parentModule = module; const replMap = new WeakMap(); +const GLOBAL_OBJECT_PROPERTIES = ['NaN', 'Infinity', 'undefined', + 'eval', 'parseInt', 'parseFloat', 'isNaN', 'isFinite', 'decodeURI', + 'decodeURIComponent', 'encodeURI', 'encodeURIComponent', + 'Object', 'Function', 'Array', 'String', 'Boolean', 'Number', + 'Date', 'RegExp', 'Error', 'EvalError', 'RangeError', + 'ReferenceError', 'SyntaxError', 'TypeError', 'URIError', + 'Math', 'JSON']; +const GLOBAL_OBJECT_PROPERTY_MAP = {}; +GLOBAL_OBJECT_PROPERTIES.forEach((p) => GLOBAL_OBJECT_PROPERTY_MAP[p] = p); + try { // hack for require.resolve("./relative") to work properly. module.filename = path.resolve('repl'); @@ -582,19 +592,20 @@ REPLServer.prototype.createContext = function() { context = global; } else { context = vm.createContext(); + context.global = context; + context.global.global = context; const _console = new Console(this.outputStream); - Object.getOwnPropertyNames(global).forEach((name) => { - if (name === 'console') { - Object.defineProperty(context, name, { - configurable: true, - enumerable: true, - value: _console - }); - } - else { - Object.defineProperty(context, name, - Object.getOwnPropertyDescriptor(global, name)); - } + Object.defineProperty(context, 'console', { + configurable: true, + enumerable: true, + get: () => _console + }); + Object.getOwnPropertyNames(global).filter((name) => { + if (name === 'console' || name === 'global') return false; + return GLOBAL_OBJECT_PROPERTY_MAP[name] === undefined; + }).forEach((name) => { + Object.defineProperty(context, name, + Object.getOwnPropertyDescriptor(global, name)); }); } @@ -1062,13 +1073,7 @@ REPLServer.prototype.memory = function memory(cmd) { function addStandardGlobals(completionGroups, filter) { // Global object properties // (http://www.ecma-international.org/publications/standards/Ecma-262.htm) - completionGroups.push(['NaN', 'Infinity', 'undefined', - 'eval', 'parseInt', 'parseFloat', 'isNaN', 'isFinite', 'decodeURI', - 'decodeURIComponent', 'encodeURI', 'encodeURIComponent', - 'Object', 'Function', 'Array', 'String', 'Boolean', 'Number', - 'Date', 'RegExp', 'Error', 'EvalError', 'RangeError', - 'ReferenceError', 'SyntaxError', 'TypeError', 'URIError', - 'Math', 'JSON']); + completionGroups.push(GLOBAL_OBJECT_PROPERTIES); // Common keywords. Exclude for completion on the empty string, b/c // they just get in the way. if (filter) { diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js new file mode 100644 index 00000000000000..862571e84198c9 --- /dev/null +++ b/test/parallel/test-repl-context.js @@ -0,0 +1,33 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const repl = require('repl'); + +// Create a dummy stream that does nothing +const stream = new common.ArrayStream(); + +// Test when useGlobal is false +testContext(repl.start({ + input: stream, + output: stream, + useGlobal: false +})); + +// Test when useGlobal is true +repl.start({ + input: stream, + output: stream +}); + +function testContext(repl) { + const context = repl.createContext(); + // ensure that the repl context gets its own "console" instance + assert(context.console instanceof require('console').Console); + + // ensure that the repl's global property is the context + assert(context.global === context); + assert(context.global.global === context); + + // ensure that the repl console instance does not have a setter + assert.throws(() => context.console = 'foo'); +} diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 97e5bacfc86b76..2df29c4dd835e5 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -268,7 +268,7 @@ testMe.complete('.b', common.mustCall((error, data) => { assert.deepStrictEqual(data, [['break'], 'b']); })); -var testNonGlobal = repl.start({ +const testNonGlobal = repl.start({ input: putIn, output: putIn, useGlobal: false From b4d8de75faba92b460d03f819eeb7c990599c449 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Thu, 23 Jun 2016 16:45:03 -0400 Subject: [PATCH 3/5] repl: remove unused test code. --- test/parallel/test-repl-context.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js index 862571e84198c9..a8e299bab6ff6b 100644 --- a/test/parallel/test-repl-context.js +++ b/test/parallel/test-repl-context.js @@ -13,12 +13,6 @@ testContext(repl.start({ useGlobal: false })); -// Test when useGlobal is true -repl.start({ - input: stream, - output: stream -}); - function testContext(repl) { const context = repl.createContext(); // ensure that the repl context gets its own "console" instance From 0991efec657db7a0ed913da9a6582805862d8e26 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Fri, 24 Jun 2016 12:22:07 -0400 Subject: [PATCH 4/5] repl: Remove `context.global.global = context`. This existed in the original `repl.js` code, but it's not clear what the purpose is. It appears to be redundant. --- lib/repl.js | 1 - test/parallel/test-repl-context.js | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index 541e9f705aa448..b1e56b0fb5b70c 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -593,7 +593,6 @@ REPLServer.prototype.createContext = function() { } else { context = vm.createContext(); context.global = context; - context.global.global = context; const _console = new Console(this.outputStream); Object.defineProperty(context, 'console', { configurable: true, diff --git a/test/parallel/test-repl-context.js b/test/parallel/test-repl-context.js index a8e299bab6ff6b..1b319a036fa256 100644 --- a/test/parallel/test-repl-context.js +++ b/test/parallel/test-repl-context.js @@ -20,7 +20,6 @@ function testContext(repl) { // ensure that the repl's global property is the context assert(context.global === context); - assert(context.global.global === context); // ensure that the repl console instance does not have a setter assert.throws(() => context.console = 'foo'); From b1981b13017fb6cf0b215423206a0a2059c7d0f2 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Mon, 27 Jun 2016 14:21:46 -0400 Subject: [PATCH 5/5] repl: Check `Intl` existence before test. If the binary has been built with `./configure --without-intl` then the `Intl` builtin type will not be available in a repl runtime. Check for this in the test. --- test/parallel/test-repl-tab-complete.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index 2df29c4dd835e5..3d32d8e0ce0019 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -274,7 +274,12 @@ const testNonGlobal = repl.start({ useGlobal: false }); +const builtins = [['Infinity', '', 'Int16Array', 'Int32Array', + 'Int8Array'], 'I']; + +if (typeof Intl === 'object') { + builtins[0].push('Intl'); +} testNonGlobal.complete('I', common.mustCall((error, data) => { - assert.deepStrictEqual(data, [['Infinity', '', 'Int16Array', 'Int32Array', - 'Int8Array', 'Intl'], 'I']); + assert.deepStrictEqual(data, builtins); }));