Simplify the import regex (microsoft#6535)

Author: brettcannon

news/2 Fixes/6319.md

@@ -0,0 +1 @@
+Simplify the import regex to minimize performance overhead.

src/client/telemetry/constants.ts

@@ -72,6 +72,7 @@ export enum EventName {
  LINTER_NOT_INSTALLED_PROMPT = 'LINTER_NOT_INSTALLED_PROMPT',
  CONFIGURE_AVAILABLE_LINTER_PROMPT = 'CONFIGURE_AVAILABLE_LINTER_PROMPT',
  HASHED_PACKAGE_NAME = 'HASHED_PACKAGE_NAME',
+ HASHED_PACKAGE_PERF = 'HASHED_PACKAGE_PERF',
 
  JEDI_MEMORY = 'JEDI_MEMORY'
 }

src/client/telemetry/importTracker.ts

@@ -5,7 +5,7 @@
 import { inject, injectable } from 'inversify';
 import * as path from 'path';
 import { TextDocument } from 'vscode';
-import { sendTelemetryEvent } from '.';
+import { captureTelemetry, sendTelemetryEvent } from '.';
 import { IDocumentManager } from '../common/application/types';
 import { isTestExecution } from '../common/constants';
 import '../common/extensions';
@@ -17,7 +17,8 @@ import { IImportTracker } from './types';
 Python has a fairly rich import statement. Originally the matching regexp was kept simple for
 performance worries, but it led to false-positives due to matching things like docstrings with
 phrases along the lines of "from the thing" or "import the thing". To minimize false-positives the
-regexp does its best to validate the structure of the import line. This leads to us supporting:
+regexp does its best to validate the structure of the import line _within reason_. This leads to
+us supporting the following (where `pkg` represents what we are actually capturing for telemetry):
 
 - `from pkg import _`
 - `from pkg import _, _`
@@ -26,18 +27,14 @@ regexp does its best to validate the structure of the import line. This leads to
 - `import pkg, pkg`
 - `import pkg as _`
 
-We can rely on the fact that the use of the `from` and `import` keywords from the start of a line are
-only usable for imports in valid code (`from` can also be used when raising an exception, but `raise`
-would be the first keyword on a line in that instance). We also get to rely on the fact that we only
-care about the top-level package, keeping the regex extremely greedy. This should lead to the regex
-failing fast and having low performance overhead.
+Things we are ignoring the following for simplicity/performance:
+
+- `from pkg import (...)` (this includes single-line and multi-line imports with parentheses)
+- `import pkg # ... and anything else with a trailing comment.`
+- Non-standard whitespace separators within the import statement (i.e. more than a single space, tabs)
 
-We can also ignore multi-line/parenthesized imports for simplicity since we don't' need 100% accuracy,
-just enough to be able to tell what packages user's rely on to make sure we are covering our bases
-in terms of support. This allows us to anchor the start and end of the regexp and not try to handle the
-parentheses case which adds a lot more optional parts to the regexp.
 */
-const ImportRegEx = /^\s*(from\s+(?<fromImport>\w+)(?:\.\w+)*\s+import\s+\w+(?:\s+as\s+\w+|(?:\s*,\s*\w+)+(?:\s*,)?)?|import\s+(?<importImport>(?:\w+(?:\s*,\s*)?)+)(?:\s+as\s+\w+)?)\s*(#.*)?$/;
+const ImportRegEx = /^\s*(from (?<fromImport>\w+)(?:\.\w+)* import \w+(?:, \w+)*(?: as \w+)?|import (?<importImport>\w+(?:, \w+)*)(?: as \w+)?)$/;
 const MAX_DOCUMENT_LINES = 1000;
 
 // Capture isTestExecution on module load so that a test can turn it off and still
@@ -100,6 +97,7 @@ export class ImportTracker implements IImportTracker {
  }
  }
 
+ @captureTelemetry(EventName.HASHED_PACKAGE_PERF)
  private checkDocument(document: TextDocument) {
  this.pendingDocs.delete(document.fileName);
  const lines = this.getDocumentLines(document);

src/client/telemetry/index.ts

@@ -287,6 +287,7 @@ export interface IEventNamePropertyMapping {
  [EventName.GO_TO_OBJECT_DEFINITION]: never | undefined;
  [EventName.HOVER_DEFINITION]: never | undefined;
  [EventName.HASHED_PACKAGE_NAME]: { hashedName: string };
+ [EventName.HASHED_PACKAGE_PERF]: never | undefined;
  [EventName.LINTER_NOT_INSTALLED_PROMPT]: LinterInstallPromptTelemetry;
  [EventName.PYTHON_INSTALL_PACKAGE]: { installer: string };
  [EventName.LINTING]: LintingTelemetry;

src/test/application/diagnostics/checks/updateTestSettings.unit.test.ts

@@ -25,7 +25,7 @@ suite('Application Diagnostics - Check Test Settings', () => {
  let appEnv: IApplicationEnvironment;
  let storage: IPersistentState<string[]>;
  let workspace: IWorkspaceService;
- const sandbox = sinon.sandbox.create();
+ const sandbox = sinon.createSandbox();
  setup(() => {
  fs = mock(FileSystem);
  appEnv = mock(ApplicationEnvironment);

src/test/telemetry/importTracker.unit.test.ts

@@ -21,20 +21,31 @@ suite('Import Tracker', () => {
  let documentManager: TypeMoq.IMock<IDocumentManager>;
  let openedEventEmitter: EventEmitter<TextDocument>;
  let savedEventEmitter: EventEmitter<TextDocument>;
- const pandasHash = hashJs.sha256().update('pandas').digest('hex');
- const elephasHash = hashJs.sha256().update('elephas').digest('hex');
- const kerasHash = hashJs.sha256().update('keras').digest('hex');
- const pysparkHash = hashJs.sha256().update('pyspark').digest('hex');
- const sparkdlHash = hashJs.sha256().update('sparkdl').digest('hex');
- const numpyHash = hashJs.sha256().update('numpy').digest('hex');
- const scipyHash = hashJs.sha256().update('scipy').digest('hex');
- const sklearnHash = hashJs.sha256().update('sklearn').digest('hex');
- const randomHash = hashJs.sha256().update('random').digest('hex');
+ const pandasHash: string = hashJs.sha256().update('pandas').digest('hex');
+ const elephasHash: string = hashJs.sha256().update('elephas').digest('hex');
+ const kerasHash: string = hashJs.sha256().update('keras').digest('hex');
+ const pysparkHash: string = hashJs.sha256().update('pyspark').digest('hex');
+ const sparkdlHash: string = hashJs.sha256().update('sparkdl').digest('hex');
+ const numpyHash: string = hashJs.sha256().update('numpy').digest('hex');
+ const scipyHash: string = hashJs.sha256().update('scipy').digest('hex');
+ const sklearnHash: string = hashJs.sha256().update('sklearn').digest('hex');
+ const randomHash: string = hashJs.sha256().update('random').digest('hex');
 
  class Reporter {
  public static eventNames: string[] = [];
  public static properties: Record<string, string>[] = [];
  public static measures: {}[] = [];
+
+ public static expectHashes(...hashes: string[]) {
+ expect(Reporter.eventNames).to.contain(EventName.HASHED_PACKAGE_PERF);
+ if (hashes.length > 0) {
+ expect(Reporter.eventNames).to.contain(EventName.HASHED_PACKAGE_NAME);
+ }
+
+ Reporter.properties.pop(); // HASHED_PACKAGE_PERF
+ expect(Reporter.properties).to.deep.equal(hashes.map(hash => ({ hashedName: hash })));
+ }
+
  public sendTelemetryEvent(eventName: string, properties?: {}, measures?: {}) {
  Reporter.eventNames.push(eventName);
  Reporter.properties.push(properties!);
@@ -76,24 +87,21 @@ suite('Import Tracker', () => {
  test('Open document', () => {
  emitDocEvent('import pandas\r\n', openedEventEmitter);
 
- expect(Reporter.eventNames).to.deep.equal([EventName.HASHED_PACKAGE_NAME]);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(pandasHash);
  });
 
  test('Already opened documents', async () => {
  const doc = createDocument('import pandas\r\n', 'foo.py', 1, TypeMoq.Times.atMost(100), true);
  documentManager.setup(d => d.textDocuments).returns(() => [doc.object]);
  await importTracker.activate();
 
- expect(Reporter.eventNames).to.deep.equal([EventName.HASHED_PACKAGE_NAME]);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(pandasHash);
  });
 
  test('Save document', () => {
  emitDocEvent('import pandas\r\n', savedEventEmitter);
 
- expect(Reporter.eventNames).to.deep.equal([EventName.HASHED_PACKAGE_NAME]);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(pandasHash);
  });
 
  test('from <pkg>._ import _, _', () => {
@@ -120,7 +128,7 @@ suite('Import Tracker', () => {
  model.set_weights(weights)`;
 
  emitDocEvent(elephas, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: elephasHash}, {hashedName: kerasHash}]);
+ Reporter.expectHashes(elephasHash, kerasHash);
  });
 
  test('from <pkg>._ import _', () => {
@@ -142,7 +150,7 @@ suite('Import Tracker', () => {
  print("Training set accuracy = " + str(evaluator.evaluate(predictionAndLabels)))`;
 
  emitDocEvent(pyspark, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pysparkHash}, {hashedName: sparkdlHash}]);
+ Reporter.expectHashes(pysparkHash, sparkdlHash);
  });
 
  test('import <pkg> as _', () => {
@@ -158,7 +166,7 @@ def simplify_ages(df):
  df.Age = categories
  return df`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}, {hashedName: numpyHash}, {hashedName: randomHash}]);
+ Reporter.expectHashes(pandasHash, numpyHash, randomHash);
  });
 
  test('from <pkg> import _', () => {
@@ -172,14 +180,13 @@ x = np.array([r * np.cos(theta) for r in radius])
 y = np.array([r * np.sin(theta) for r in radius])
 z = np.array([drumhead_height(1, 1, r, theta, 0.5) for r in radius])`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: scipyHash}]);
+ Reporter.expectHashes(scipyHash);
  });
 
-
  test('from <pkg> import _ as _', () => {
  const code = `from pandas import DataFrame as df`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(pandasHash);
  });
 
  test('import <pkg1>, <pkg2>', () => {
@@ -193,27 +200,7 @@ x = np.array([r * np.cos(theta) for r in radius])
 y = np.array([r * np.sin(theta) for r in radius])
 z = np.array([drumhead_height(1, 1, r, theta, 0.5) for r in radius])`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: sklearnHash}, {hashedName: pandasHash}]);
- });
-
- /*test('from <pkg> import (_, _)', () => {
- const code = `from pandas import (DataFrame, Series)`;
- emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
- });
-
- test('from <pkg> import (_,', () => {
- const code = `
-from pandas import (DataFrame,
-Series)`;
- emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
- });*/
-
- test('import pkg # Comment', () => {
- const code = `import pandas # Because we wants it.`;
- emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(sklearnHash, pandasHash);
  });
 
  test('Import from within a function', () => {
@@ -227,36 +214,36 @@ x = np.array([r * np.cos(theta) for r in radius])
 y = np.array([r * np.sin(theta) for r in radius])
 z = np.array([drumhead_height(1, 1, r, theta, 0.5) for r in radius])`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: sklearnHash}]);
+ Reporter.expectHashes(sklearnHash);
  });
 
  test('Do not send the same package twice', () => {
  const code = `
 import pandas
 import pandas`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([{hashedName: pandasHash}]);
+ Reporter.expectHashes(pandasHash);
  });
 
  test('Ignore relative imports', () => {
  const code = 'from .pandas import not_real';
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([]);
+ Reporter.expectHashes();
  });
 
  test('Ignore docstring for `from` imports', () => {
  const code = `"""
 from numpy import the random function
 """`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([]);
+ Reporter.expectHashes();
  });
 
  test('Ignore docstring for `import` imports', () => {
  const code = `"""
 import numpy for all the things
 """`;
  emitDocEvent(code, savedEventEmitter);
- expect(Reporter.properties).to.deep.equal([]);
+ Reporter.expectHashes();
  });
 });