repl: improve repl autocompletion for require calls

This improves the autocompletion for require calls. It had multiple small issues so far. Most important: it won't suggest completions for require statements that are fully written out. Second, it'll detect require calls that have whitespace behind the opening bracket. Third, it makes sure node modules are detected as such instead of only suggesting them as folders. Last, it adds suggestions for input that starts with backticks. Fixes: #33238 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
nodejs · BridgeAR · May 4, 2020 · May 4, 2020 · May 7, 2020 · May 7, 2020
commit 00cd4d83db48d2201fcf87b96c7af77e966d2725
diff --git a/lib/repl.js b/lib/repl.js
@@ -1047,7 +1047,7 @@ REPLServer.prototype.turnOffEditorMode = deprecate(
   'REPLServer.turnOffEditorMode() is deprecated',
   'DEP0078');
 
-const requireRE = /\brequire\s*\(['"](([\w@./-]+\/)?(?:[\w@./-]*))/;
+const requireRE = /\brequire\s*\(\s*['"`](([\w@./-]+\/)?(?:[\w@./-]*))(?![^'"`])$/;
 const fsAutoCompleteRE = /fs(?:\.promises)?\.\s*[a-z][a-zA-Z]+\(\s*["'](.*)/;
 const simpleExpressionRE =
     /(?:[a-zA-Z_$](?:\w|\$)*\.)*[a-zA-Z_$](?:\w|\$)*\.?$/;
@@ -1095,8 +1095,13 @@ REPLServer.prototype.complete = function() {
   this.completer.apply(this, arguments);
 };
 
-// TODO: Native module names should be auto-resolved.
-// That improves the auto completion.
+function gracefulOperation(fn, args, alternative) {
+  try {
+    return fn(...args);
+  } catch {
+    return alternative;
+  }
+}
 
 // Provide a list of completions for the given leading text. This is
 // given to the readline interface for handling tab completion.
@@ -1118,26 +1123,25 @@ function complete(line, callback) {
 
   // REPL commands (e.g. ".break").
   let filter;
-  let match = line.match(/^\s*\.(\w*)$/);
-  if (match) {
+  if (/^\s*\.(\w*)$/.test(line)) {
     completionGroups.push(ObjectKeys(this.commands));
-    completeOn = match[1];
-    if (match[1].length) {
-      filter = match[1];
+    completeOn = line.match(/^\s*\.(\w*)$/)[1];
+    if (completeOn.length) {
+      filter = completeOn;
     }
 
     completionGroupsLoaded();
-  } else if (match = line.match(requireRE)) {
+  } else if (requireRE.test(line)) {
     // require('...<Tab>')
-    const exts = ObjectKeys(this.context.require.extensions);
-    const indexRe = new RegExp('^index(?:' + exts.map(regexpEscape).join('|') +
-                             ')$');
+    const extensions = ObjectKeys(this.context.require.extensions);
+    const indexes = extensions.map((extension) => `index${extension}`);
+    indexes.push('package.json', 'index');
     const versionedFileNamesRe = /-\d+\.\d+/;
 
+    const match = line.match(requireRE);
     completeOn = match[1];
     const subdir = match[2] || '';
-    filter = match[1];
-    let dir, files, subfiles, isDirectory;
+    filter = completeOn;
     group = [];
     let paths = [];
 
@@ -1151,41 +1155,31 @@ function complete(line, callback) {
       paths = module.paths.concat(CJSModule.globalPaths);
     }
 
-    for (let i = 0; i < paths.length; i++) {
-      dir = path.resolve(paths[i], subdir);
-      try {
-        files = fs.readdirSync(dir);
-      } catch {
-        continue;
-      }
-      for (let f = 0; f < files.length; f++) {
-        const name = files[f];
+    for (let dir of paths) {
+      dir = path.resolve(dir, subdir);
+      const files = gracefulOperation(fs.readdirSync, [dir], []);
+      for (const name of files) {
         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;
         }
         const abs = path.resolve(dir, name);
-        try {
-          isDirectory = fs.statSync(abs).isDirectory();
-        } catch {
+        const stats = gracefulOperation(fs.statSync, [abs]);
+        if (!stats || !stats.isDirectory()) {
+          if (extensions.includes(ext) && (!subdir || base !== 'index')) {
+            group.push(`${subdir}${base}`);
+          }
           continue;
         }
-        if (isDirectory) {
-          group.push(subdir + name + '/');
-          try {
-            subfiles = fs.readdirSync(abs);
-          } catch {
-            continue;
-          }
-          for (let s = 0; s < subfiles.length; s++) {
-            if (indexRe.test(subfiles[s])) {
-              group.push(subdir + name);
-            }
+        group.push(`${subdir}${name}/`);
+        const subfiles = gracefulOperation(fs.readdirSync, [abs], []);
+        for (const subfile of subfiles) {
+          if (indexes.includes(subfile)) {
+            group.push(`${subdir}${name}`);
+            break;
           }
-        } else if (exts.includes(ext) && (!subdir || base !== 'index')) {
-          group.push(subdir + base);
         }
       }
     }
@@ -1198,11 +1192,10 @@ function complete(line, callback) {
     }
 
     completionGroupsLoaded();
-  } else if (match = line.match(fsAutoCompleteRE)) {
-
-    let filePath = match[1];
-    let fileList;
+  } else if (fsAutoCompleteRE.test(line)) {
     filter = '';
+    let filePath = line.match(fsAutoCompleteRE)[1];
+    let fileList;
 
     try {
       fileList = fs.readdirSync(filePath, { withFileTypes: true });
@@ -1233,7 +1226,7 @@ function complete(line, callback) {
   //   foo<|>         # all scope vars with filter 'foo'
   //   foo.<|>        # completions for 'foo' with filter ''
   } else if (line.length === 0 || /\w|\.|\$/.test(line[line.length - 1])) {
-    match = simpleExpressionRE.exec(line);
+    const match = simpleExpressionRE.exec(line);
     if (line.length !== 0 && !match) {
       completionGroupsLoaded();
       return;
@@ -1583,10 +1576,6 @@ function defineDefaultCommands(repl) {
   }
 }
 
-function regexpEscape(s) {
-  return s.replace(/[-[\]{}()*+?.,\\^$|#\s]/g, '\\$&');
-}
-
 function Recoverable(err) {
   this.err = err;
 }

diff --git a/test/fixtures/node_modules/no_index/lib/index.js b/test/fixtures/node_modules/no_index/lib/index.js
diff --git a/test/fixtures/node_modules/no_index/package.json b/test/fixtures/node_modules/no_index/package.json
diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js
@@ -229,24 +229,46 @@ testMe.complete('require(\'', common.mustCall(function(error, data) {
   });
 }));
 
-testMe.complete('require(\'n', common.mustCall(function(error, data) {
+testMe.complete("require\t( 'n", common.mustCall(function(error, data) {
   assert.strictEqual(error, null);
   assert.strictEqual(data.length, 2);
   assert.strictEqual(data[1], 'n');
-  assert(data[0].includes('net'));
+  // There is only one Node.js module that starts with n:
+  assert.strictEqual(data[0][0], 'net');
+  assert.strictEqual(data[0][1], '');
   // It's possible to pick up non-core modules too
-  data[0].forEach(function(completion) {
-    if (completion)
-      assert(/^n/.test(completion));
+  data[0].slice(2).forEach((completion) => {
+    assert.match(completion, /^n/);
   });
 }));
 
 {
   const expected = ['@nodejsscope', '@nodejsscope/'];
+  // Require calls should handle all types of quotation marks.
+  for (const quotationMark of ["'", '"', '`']) {
+    putIn.run(['.clear']);
+    testMe.complete('require(`@nodejs', common.mustCall((err, data) => {
+      assert.strictEqual(err, null);
+      assert.deepStrictEqual(data, [expected, '@nodejs']);
+    }));
+
+    putIn.run(['.clear']);
+    // Completions should not be greedy in case the quotation ends.
+    const input = `require(${quotationMark}@nodejsscope${quotationMark}`;
+    testMe.complete(input, common.mustCall((err, data) => {
+      assert.strictEqual(err, null);
+      assert.deepStrictEqual(data, [[], undefined]);
+    }));
+  }
+}
+
+{
   putIn.run(['.clear']);
-  testMe.complete('require(\'@nodejs', common.mustCall((err, data) => {
+  // Completions should find modules and handle whitespace after the opening
+  // bracket.
+  testMe.complete('require \t("no_ind', common.mustCall((err, data) => {
     assert.strictEqual(err, null);
-    assert.deepStrictEqual(data, [expected, '@nodejs']);
+    assert.deepStrictEqual(data, [['no_index', 'no_index/'], 'no_ind']);
   }));
 }