From 3906e145cae4e69053d2821deac2ab9d27c205f2 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Wed, 11 Dec 2019 19:24:39 +0100 Subject: [PATCH] repl,readline: refactor for simplicity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This just refactors code without changing the behavior. Especially the REPL code is difficult to read and deeply indented. This reduces the indentation to improve that. PR-URL: https://github.com/nodejs/node/pull/30907 Reviewed-By: Michaël Zasso Reviewed-By: Rich Trott --- lib/readline.js | 64 ++++++----- lib/repl.js | 296 ++++++++++++++++++++++++------------------------ 2 files changed, 179 insertions(+), 181 deletions(-) diff --git a/lib/readline.js b/lib/readline.js index 9a306c654b50a5..0ba30cc04b1124 100644 --- a/lib/readline.js +++ b/lib/readline.js @@ -505,41 +505,43 @@ Interface.prototype._tabComplete = function(lastKeypressWasTab) { return; } - const completions = rv[0]; - const completeOn = rv[1]; // The text that was completed - if (completions && completions.length) { - // Apply/show completions. - if (lastKeypressWasTab) { - self._writeToOutput('\r\n'); - const width = completions.reduce(function completionReducer(a, b) { - return a.length > b.length ? a : b; - }).length + 2; // 2 space padding - let maxColumns = MathFloor(self.columns / width); - if (!maxColumns || maxColumns === Infinity) { - maxColumns = 1; - } - let group = []; - for (let i = 0; i < completions.length; i++) { - const c = completions[i]; - if (c === '') { - handleGroup(self, group, width, maxColumns); - group = []; - } else { - group.push(c); - } - } - handleGroup(self, group, width, maxColumns); - } + // Result and the text that was completed. + const [completions, completeOn] = rv; + + if (!completions || completions.length === 0) { + return; + } - // If there is a common prefix to all matches, then apply that portion. - const f = completions.filter((e) => e); - const prefix = commonPrefix(f); - if (prefix.length > completeOn.length) { - self._insertString(prefix.slice(completeOn.length)); + // Apply/show completions. + if (lastKeypressWasTab) { + self._writeToOutput('\r\n'); + const width = completions.reduce((a, b) => { + return a.length > b.length ? a : b; + }).length + 2; // 2 space padding + let maxColumns = MathFloor(self.columns / width); + if (!maxColumns || maxColumns === Infinity) { + maxColumns = 1; } + let group = []; + for (const c of completions) { + if (c === '') { + handleGroup(self, group, width, maxColumns); + group = []; + } else { + group.push(c); + } + } + handleGroup(self, group, width, maxColumns); + } - self._refreshLine(); + // If there is a common prefix to all matches, then apply that portion. + const f = completions.filter((e) => e); + const prefix = commonPrefix(f); + if (prefix.length > completeOn.length) { + self._insertString(prefix.slice(completeOn.length)); } + + self._refreshLine(); }); }; diff --git a/lib/repl.js b/lib/repl.js index 221b5f1f612d92..4f32c9c28ced3d 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -52,7 +52,6 @@ const { ObjectGetOwnPropertyNames, ObjectGetPrototypeOf, ObjectKeys, - ObjectPrototypeHasOwnProperty, ObjectSetPrototypeOf, Symbol, } = primordials; @@ -1164,7 +1163,7 @@ function complete(line, callback) { completeOn = match[1]; const subdir = match[2] || ''; filter = match[1]; - let dir, files, name, base, ext, abs, subfiles, isDirectory; + let dir, files, subfiles, isDirectory; group = []; let paths = []; @@ -1186,14 +1185,14 @@ function complete(line, callback) { continue; } for (let f = 0; f < files.length; f++) { - name = files[f]; - ext = path.extname(name); - base = name.slice(0, -ext.length); + const name = files[f]; + const ext = path.extname(name); + const base = name.slice(0, -ext.length); if (versionedFileNamesRe.test(base) || name === '.npm') { // Exclude versioned names that 'npm' installs. continue; } - abs = path.resolve(dir, name); + const abs = path.resolve(dir, name); try { isDirectory = fs.statSync(abs).isDirectory(); } catch { @@ -1261,86 +1260,87 @@ function complete(line, callback) { // foo.<|> # completions for 'foo' with filter '' } else if (line.length === 0 || /\w|\.|\$/.test(line[line.length - 1])) { match = simpleExpressionRE.exec(line); - if (line.length === 0 || match) { - let expr; - completeOn = (match ? match[0] : ''); - if (line.length === 0) { - filter = ''; - expr = ''; - } else if (line[line.length - 1] === '.') { - filter = ''; - expr = match[0].slice(0, match[0].length - 1); - } else { - const bits = match[0].split('.'); - filter = bits.pop(); - expr = bits.join('.'); + if (line.length !== 0 && !match) { + completionGroupsLoaded(); + return; + } + let expr; + completeOn = (match ? match[0] : ''); + if (line.length === 0) { + filter = ''; + expr = ''; + } else if (line[line.length - 1] === '.') { + filter = ''; + expr = match[0].slice(0, match[0].length - 1); + } else { + const bits = match[0].split('.'); + filter = bits.pop(); + expr = bits.join('.'); + } + + // Resolve expr and get its completions. + const memberGroups = []; + if (!expr) { + // Get global vars synchronously + completionGroups.push(getGlobalLexicalScopeNames(this[kContextId])); + let contextProto = this.context; + while (contextProto = ObjectGetPrototypeOf(contextProto)) { + completionGroups.push(filteredOwnPropertyNames(contextProto)); } + const contextOwnNames = filteredOwnPropertyNames(this.context); + if (!this.useGlobal) { + // When the context is not `global`, builtins are not own + // properties of it. + contextOwnNames.push(...globalBuiltins); + } + completionGroups.push(contextOwnNames); + if (filter !== '') addCommonWords(completionGroups); + completionGroupsLoaded(); + return; + } - // Resolve expr and get its completions. - const memberGroups = []; - if (!expr) { - // Get global vars synchronously - completionGroups.push(getGlobalLexicalScopeNames(this[kContextId])); - let contextProto = this.context; - while (contextProto = ObjectGetPrototypeOf(contextProto)) { - completionGroups.push(filteredOwnPropertyNames(contextProto)); - } - const contextOwnNames = filteredOwnPropertyNames(this.context); - if (!this.useGlobal) { - // When the context is not `global`, builtins are not own - // properties of it. - contextOwnNames.push(...globalBuiltins); + const evalExpr = `try { ${expr} } catch {}`; + this.eval(evalExpr, this.context, 'repl', (e, obj) => { + if (obj != null) { + if (typeof obj === 'object' || typeof obj === 'function') { + try { + memberGroups.push(filteredOwnPropertyNames(obj)); + } catch { + // Probably a Proxy object without `getOwnPropertyNames` trap. + // We simply ignore it here, as we don't want to break the + // autocompletion. Fixes the bug + // https://github.com/nodejs/node/issues/2119 + } } - completionGroups.push(contextOwnNames); - if (filter !== '') addCommonWords(completionGroups); - completionGroupsLoaded(); - } else { - const evalExpr = `try { ${expr} } catch {}`; - this.eval(evalExpr, this.context, 'repl', (e, obj) => { - if (obj != null) { - if (typeof obj === 'object' || typeof obj === 'function') { - try { - memberGroups.push(filteredOwnPropertyNames(obj)); - } catch { - // Probably a Proxy object without `getOwnPropertyNames` trap. - // We simply ignore it here, as we don't want to break the - // autocompletion. Fixes the bug - // https://github.com/nodejs/node/issues/2119 - } - } - // Works for non-objects - try { - let p; - if (typeof obj === 'object' || typeof obj === 'function') { - p = ObjectGetPrototypeOf(obj); - } else { - p = obj.constructor ? obj.constructor.prototype : null; - } - // Circular refs possible? Let's guard against that. - let sentinel = 5; - while (p !== null && sentinel-- !== 0) { - memberGroups.push(filteredOwnPropertyNames(p)); - p = ObjectGetPrototypeOf(p); - } - } catch {} + // Works for non-objects + try { + let p; + if (typeof obj === 'object' || typeof obj === 'function') { + p = ObjectGetPrototypeOf(obj); + } else { + p = obj.constructor ? obj.constructor.prototype : null; } - - if (memberGroups.length) { - for (let i = 0; i < memberGroups.length; i++) { - completionGroups.push( - memberGroups[i].map((member) => `${expr}.${member}`)); - } - if (filter) { - filter = `${expr}.${filter}`; - } + // Circular refs possible? Let's guard against that. + let sentinel = 5; + while (p !== null && sentinel-- !== 0) { + memberGroups.push(filteredOwnPropertyNames(p)); + p = ObjectGetPrototypeOf(p); } + } catch {} + } - completionGroupsLoaded(); - }); + if (memberGroups.length) { + for (let i = 0; i < memberGroups.length; i++) { + completionGroups.push( + memberGroups[i].map((member) => `${expr}.${member}`)); + } + if (filter) { + filter = `${expr}.${filter}`; + } } - } else { + completionGroupsLoaded(); - } + }); } else { completionGroupsLoaded(); } @@ -1361,32 +1361,32 @@ function complete(line, callback) { completionGroups = newCompletionGroups; } - let completions; - - if (completionGroups.length) { - const uniq = {}; // Unique completions across all groups - completions = []; - // Completion group 0 is the "closest" - // (least far up the inheritance chain) - // so we put its completions last: to be closest in the REPL. - for (let i = 0; i < completionGroups.length; i++) { - group = completionGroups[i]; - group.sort(); - for (let j = group.length - 1; j >= 0; j--) { - const c = group[j]; - if (!ObjectPrototypeHasOwnProperty(uniq, c)) { - completions.unshift(c); - uniq[c] = true; - } + const completions = []; + // Unique completions across all groups. + const uniqueSet = new Set(['']); + // Completion group 0 is the "closest" (least far up the inheritance + // chain) so we put its completions last: to be closest in the REPL. + for (const group of completionGroups) { + group.sort((a, b) => (b > a ? 1 : -1)); + const setSize = uniqueSet.size; + for (const entry of group) { + if (!uniqueSet.has(entry)) { + completions.unshift(entry); + uniqueSet.add(entry); } - completions.unshift(''); // Separator btwn groups } - while (completions.length && completions[0] === '') { - completions.shift(); + // Add a separator between groups. + if (uniqueSet.size !== setSize) { + completions.unshift(''); } } - callback(null, [completions || [], completeOn]); + // Remove obsolete group entry, if present. + if (completions[0] === '') { + completions.shift(); + } + + callback(null, [completions, completeOn]); } } @@ -1419,6 +1419,9 @@ REPLServer.prototype.memory = deprecate( 'REPLServer.memory() is deprecated', 'DEP0082'); +// TODO(BridgeAR): This should be replaced with acorn to build an AST. The +// language became more complex and using a simple approach like this is not +// sufficient anymore. function _memory(cmd) { const self = this; self.lines = self.lines || []; @@ -1426,7 +1429,6 @@ function _memory(cmd) { // Save the line so I can do magic later if (cmd) { - // TODO should I tab the level? const len = self.lines.level.length ? self.lines.level.length - 1 : 0; self.lines.push(' '.repeat(len) + cmd); } else { @@ -1434,61 +1436,55 @@ function _memory(cmd) { self.lines.push(''); } + if (!cmd) { + self.lines.level = []; + return; + } + // I need to know "depth." // Because I can not tell the difference between a } that // closes an object literal and a } that closes a function - if (cmd) { - // Going down is { and ( e.g. function() { - // going up is } and ) - let dw = cmd.match(/[{(]/g); - let up = cmd.match(/[})]/g); - up = up ? up.length : 0; - dw = dw ? dw.length : 0; - let depth = dw - up; - - if (depth) { - (function workIt() { - if (depth > 0) { - // Going... down. - // Push the line#, depth count, and if the line is a function. - // Since JS only has functional scope I only need to remove - // "function() {" lines, clearly this will not work for - // "function() - // {" but nothing should break, only tab completion for local - // scope will not work for this function. - self.lines.level.push({ - line: self.lines.length - 1, - depth: depth, - isFunction: /\bfunction\b/.test(cmd) - }); - } else if (depth < 0) { - // Going... up. - const curr = self.lines.level.pop(); - if (curr) { - const tmp = curr.depth + depth; - if (tmp < 0) { - // More to go, recurse - depth += curr.depth; - workIt(); - } else if (tmp > 0) { - // Remove and push back - curr.depth += depth; - self.lines.level.push(curr); - } + + // Going down is { and ( e.g. function() { + // going up is } and ) + let dw = cmd.match(/[{(]/g); + let up = cmd.match(/[})]/g); + up = up ? up.length : 0; + dw = dw ? dw.length : 0; + let depth = dw - up; + + if (depth) { + (function workIt() { + if (depth > 0) { + // Going... down. + // Push the line#, depth count, and if the line is a function. + // Since JS only has functional scope I only need to remove + // "function() {" lines, clearly this will not work for + // "function() + // {" but nothing should break, only tab completion for local + // scope will not work for this function. + self.lines.level.push({ + line: self.lines.length - 1, + depth: depth, + isFunction: /\bfunction\b/.test(cmd) + }); + } else if (depth < 0) { + // Going... up. + const curr = self.lines.level.pop(); + if (curr) { + const tmp = curr.depth + depth; + if (tmp < 0) { + // More to go, recurse + depth += curr.depth; + workIt(); + } else if (tmp > 0) { + // Remove and push back + curr.depth += depth; + self.lines.level.push(curr); } } - }()); - } - - // It is possible to determine a syntax error at this point. - // if the REPL still has a bufferedCommand and - // self.lines.level.length === 0 - // TODO? keep a log of level so that any syntax breaking lines can - // be cleared on .break and in the case of a syntax error? - // TODO? if a log was kept, then I could clear the bufferedCommand and - // eval these lines and throw the syntax error - } else { - self.lines.level = []; + } + }()); } }