Use file system API for path comparisons when performing code navigation (microsoft#1812)

Fixes microsoft#1811

Author: DonJayamanne

news/2 Fixes/1811.md

@@ -0,0 +1,2 @@
+Use file system API to perform file path comparisons when performing code navigation.
+(thanks to [bstaint](https://github.com/bstaint) for the initial patch)

src/client/activation/classic.ts

@@ -6,7 +6,7 @@ import { DocumentFilter, languages } from 'vscode';
 import { PYTHON } from '../common/constants';
 import { IConfigurationService, IExtensionContext, ILogger } from '../common/types';
 import { IShebangCodeLensProvider } from '../interpreter/contracts';
-import { IServiceManager } from '../ioc/types';
+import { IServiceContainer, IServiceManager } from '../ioc/types';
 import { JediFactory } from '../languageServices/jediProxyFactory';
 import { PythonCompletionItemProvider } from '../providers/completionProvider';
 import { PythonDefinitionProvider } from '../providers/definitionProvider';
@@ -46,7 +46,7 @@ export class ClassicExtensionActivator implements IExtensionActivator {
  context.subscriptions.push(languages.registerCompletionItemProvider(this.documentSelector, new PythonCompletionItemProvider(jediFactory, this.serviceManager), '.'));
  context.subscriptions.push(languages.registerCodeLensProvider(this.documentSelector, this.serviceManager.get<IShebangCodeLensProvider>(IShebangCodeLensProvider)));
 
- const symbolProvider = new PythonSymbolProvider(jediFactory);
+ const symbolProvider = new PythonSymbolProvider(this.serviceManager.get<IServiceContainer>(IServiceContainer), jediFactory);
  context.subscriptions.push(languages.registerDocumentSymbolProvider(this.documentSelector, symbolProvider));
 
  const pythonSettings = this.serviceManager.get<IConfigurationService>(IConfigurationService).getSettings();

src/client/providers/symbolProvider.ts

@@ -2,30 +2,19 @@
 
 import { CancellationToken, DocumentSymbolProvider, Location, Range, SymbolInformation, TextDocument, Uri } from 'vscode';
 import { createDeferred, Deferred } from '../common/helpers';
+import { IFileSystem } from '../common/platform/types';
+import { IServiceContainer } from '../ioc/types';
 import { JediFactory } from '../languageServices/jediProxyFactory';
 import { captureTelemetry } from '../telemetry';
 import { SYMBOL } from '../telemetry/constants';
 import * as proxy from './jediProxy';
 
 export class PythonSymbolProvider implements DocumentSymbolProvider {
  private debounceRequest: Map<string, { timer: NodeJS.Timer; deferred: Deferred<SymbolInformation[]> }>;
- public constructor(private jediFactory: JediFactory, private readonly debounceTimeoutMs = 500) {
+ private readonly fs: IFileSystem;
+ public constructor(serviceContainer: IServiceContainer, private jediFactory: JediFactory, private readonly debounceTimeoutMs = 500) {
  this.debounceRequest = new Map<string, { timer: NodeJS.Timer; deferred: Deferred<SymbolInformation[]> }>();
- }
- private static parseData(document: TextDocument, data?: proxy.ISymbolResult): SymbolInformation[] {
- if (data) {
- const symbols = data.definitions.filter(sym => sym.fileName === document.fileName);
- return symbols.map(sym => {
- const symbol = sym.kind;
- const range = new Range(
- sym.range.startLine, sym.range.startColumn,
- sym.range.endLine, sym.range.endColumn);
- const uri = Uri.file(sym.fileName);
- const location = new Location(uri, range);
- return new SymbolInformation(sym.text, symbol, sym.container, location);
- });
- }
- return [];
+ this.fs = serviceContainer.get<IFileSystem>(IFileSystem);
  }
  @captureTelemetry(SYMBOL)
  public provideDocumentSymbols(document: TextDocument, token: CancellationToken): Thenable<SymbolInformation[]> {
@@ -55,7 +44,7 @@ export class PythonSymbolProvider implements DocumentSymbolProvider {
  }
 
  this.jediFactory.getJediProxyHandler<proxy.ISymbolResult>(document.uri).sendCommand(cmd, token)
- .then(data => PythonSymbolProvider.parseData(document, data))
+ .then(data => this.parseData(document, data))
  .then(items => deferred.resolve(items))
  .catch(ex => deferred.reject(ex));
 
@@ -89,6 +78,21 @@ export class PythonSymbolProvider implements DocumentSymbolProvider {
  }
 
  return this.jediFactory.getJediProxyHandler<proxy.ISymbolResult>(document.uri).sendCommandNonCancellableCommand(cmd, token)
- .then(data => PythonSymbolProvider.parseData(document, data));
+ .then(data => this.parseData(document, data));
+ }
+ private parseData(document: TextDocument, data?: proxy.ISymbolResult): SymbolInformation[] {
+ if (data) {
+ const symbols = data.definitions.filter(sym => this.fs.arePathsSame(sym.fileName, document.fileName));
+ return symbols.map(sym => {
+ const symbol = sym.kind;
+ const range = new Range(
+ sym.range.startLine, sym.range.startColumn,
+ sym.range.endLine, sym.range.endColumn);
+ const uri = Uri.file(sym.fileName);
+ const location = new Location(uri, range);
+ return new SymbolInformation(sym.text, symbol, sym.container, location);
+ });
+ }
+ return [];
  }
 }

src/test/providers/symbolProvider.test.ts

@@ -8,26 +8,39 @@
 import { expect, use } from 'chai';
 import * as TypeMoq from 'typemoq';
 import { CancellationToken, CancellationTokenSource, CompletionItemKind, DocumentSymbolProvider, SymbolKind, TextDocument, Uri } from 'vscode';
+import { IFileSystem } from '../../client/common/platform/types';
+import { IServiceContainer } from '../../client/ioc/types';
 import { JediFactory } from '../../client/languageServices/jediProxyFactory';
 import { IDefinition, ISymbolResult, JediProxyHandler } from '../../client/providers/jediProxy';
 import { PythonSymbolProvider } from '../../client/providers/symbolProvider';
+
 const assertArrays = require('chai-arrays');
 use(assertArrays);
 
 suite('Symbol Provider', () => {
- let symbolProvider: DocumentSymbolProvider;
+ let serviceContainer: TypeMoq.IMock<IServiceContainer>;
  let jediHandler: TypeMoq.IMock<JediProxyHandler<ISymbolResult>>;
  let jediFactory: TypeMoq.IMock<JediFactory>;
+ let fileSystem: TypeMoq.IMock<IFileSystem>;
+ let provider: DocumentSymbolProvider;
+ let uri: Uri;
+ let doc: TypeMoq.IMock<TextDocument>;
  setup(() => {
+ serviceContainer = TypeMoq.Mock.ofType<IServiceContainer>();
  jediFactory = TypeMoq.Mock.ofType(JediFactory);
  jediHandler = TypeMoq.Mock.ofType<JediProxyHandler<ISymbolResult>>();
 
+ fileSystem = TypeMoq.Mock.ofType<IFileSystem>();
+ doc = TypeMoq.Mock.ofType<TextDocument>();
  jediFactory.setup(j => j.getJediProxyHandler(TypeMoq.It.isAny()))
  .returns(() => jediHandler.object);
+
+ serviceContainer.setup(c => c.get(IFileSystem)).returns(() => fileSystem.object);
  });
 
  async function testDocumentation(requestId: number, fileName: string, expectedSize: number, token?: CancellationToken, isUntitled = false) {
- const doc = TypeMoq.Mock.ofType<TextDocument>();
+ fileSystem.setup(fs => fs.arePathsSame(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+ .returns(() => true);
  token = token ? token : new CancellationTokenSource().token;
  const symbolResult = TypeMoq.Mock.ofType<ISymbolResult>();
 
@@ -39,91 +52,130 @@ suite('Symbol Provider', () => {
  }
  ];
 
+ uri = Uri.file(fileName);
+ doc.setup(d => d.uri).returns(() => uri);
  doc.setup(d => d.fileName).returns(() => fileName);
  doc.setup(d => d.isUntitled).returns(() => isUntitled);
- doc.setup(d => d.uri).returns(() => Uri.file(fileName));
  doc.setup(d => d.getText(TypeMoq.It.isAny())).returns(() => '');
  symbolResult.setup(c => c.requestId).returns(() => requestId);
  symbolResult.setup(c => c.definitions).returns(() => definitions);
  symbolResult.setup((c: any) => c.then).returns(() => undefined);
  jediHandler.setup(j => j.sendCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
  .returns(() => Promise.resolve(symbolResult.object));
 
- const items = await symbolProvider.provideDocumentSymbols(doc.object, token);
+ const items = await provider.provideDocumentSymbols(doc.object, token);
  expect(items).to.be.array();
  expect(items).to.be.ofSize(expectedSize);
  }
 
  test('Ensure symbols are returned', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await testDocumentation(1, __filename, 1);
  });
  test('Ensure symbols are returned (for untitled documents)', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await testDocumentation(1, __filename, 1, undefined, true);
  });
  test('Ensure symbols are returned with a debounce of 100ms', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await testDocumentation(1, __filename, 1);
  });
  test('Ensure symbols are returned with a debounce of 100ms (for untitled documents)', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await testDocumentation(1, __filename, 1, undefined, true);
  });
  test('Ensure symbols are not returned when cancelled', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  const tokenSource = new CancellationTokenSource();
  tokenSource.cancel();
  await testDocumentation(1, __filename, 0, tokenSource.token);
  });
  test('Ensure symbols are not returned when cancelled (for untitled documents)', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  const tokenSource = new CancellationTokenSource();
  tokenSource.cancel();
  await testDocumentation(1, __filename, 0, tokenSource.token, true);
  });
  test('Ensure symbols are returned only for the last request', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 100);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 100);
  await Promise.all([
  testDocumentation(1, __filename, 0),
  testDocumentation(2, __filename, 0),
  testDocumentation(3, __filename, 1)
  ]);
  });
  test('Ensure symbols are returned for all the requests when the doc is untitled', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 100);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 100);
  await Promise.all([
  testDocumentation(1, __filename, 1, undefined, true),
  testDocumentation(2, __filename, 1, undefined, true),
  testDocumentation(3, __filename, 1, undefined, true)
  ]);
  });
  test('Ensure symbols are returned for multiple documents', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await Promise.all([
  testDocumentation(1, 'file1', 1),
  testDocumentation(2, 'file2', 1)
  ]);
  });
  test('Ensure symbols are returned for multiple untitled documents ', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 0);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
  await Promise.all([
  testDocumentation(1, 'file1', 1, undefined, true),
  testDocumentation(2, 'file2', 1, undefined, true)
  ]);
  });
  test('Ensure symbols are returned for multiple documents with a debounce of 100ms', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 100);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 100);
  await Promise.all([
  testDocumentation(1, 'file1', 1),
  testDocumentation(2, 'file2', 1)
  ]);
  });
  test('Ensure symbols are returned for multiple untitled documents with a debounce of 100ms', async () => {
- symbolProvider = new PythonSymbolProvider(jediFactory.object, 100);
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 100);
  await Promise.all([
  testDocumentation(1, 'file1', 1, undefined, true),
  testDocumentation(2, 'file2', 1, undefined, true)
  ]);
  });
+ test('Ensure IFileSystem.arePathsSame is used', async () => {
+ doc.setup(d => d.getText())
+ .returns(() => '')
+ .verifiable(TypeMoq.Times.once());
+ doc.setup(d => d.isDirty)
+ .returns(() => true)
+ .verifiable(TypeMoq.Times.once());
+ doc.setup(d => d.fileName)
+ .returns(() => __filename);
+
+ const symbols = TypeMoq.Mock.ofType<ISymbolResult>();
+ symbols.setup((s: any) => s.then).returns(() => undefined);
+ const definitions: IDefinition[] = [];
+ for (let counter = 0; counter < 3; counter += 1) {
+ const def = TypeMoq.Mock.ofType<IDefinition>();
+ def.setup(d => d.fileName).returns(() => counter.toString());
+ definitions.push(def.object);
+
+ fileSystem.setup(fs => fs.arePathsSame(TypeMoq.It.isValue(counter.toString()), TypeMoq.It.isValue(__filename)))
+ .returns(() => false)
+ .verifiable(TypeMoq.Times.exactly(1));
+ }
+ symbols.setup(s => s.definitions)
+ .returns(() => definitions)
+ .verifiable(TypeMoq.Times.atLeastOnce());
+
+ jediHandler.setup(j => j.sendCommand(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
+ .returns(() => Promise.resolve(symbols.object))
+ .verifiable(TypeMoq.Times.once());
+
+ provider = new PythonSymbolProvider(serviceContainer.object, jediFactory.object, 0);
+ await provider.provideDocumentSymbols(doc.object, new CancellationTokenSource().token);
+
+ doc.verifyAll();
+ symbols.verifyAll();
+ fileSystem.verifyAll();
+ jediHandler.verifyAll();
+ });
 });