Ensure navigation to definitons follows imports and is transparent to decoration (#1712)

* Add baseline function definition navigation tests

This provides a starting point for more interesting tests.

* Add tests for current cases

Specifically this covers some basic cases as well as the
reproductions of #1638 and #1033 plus the variant of #1033 which
always worked.

* Make navigation to definitions follow imports

This fixes #1638 and simplifies the code (to what I believe that
@davidhalter had in mind in
#1033 (comment)).

This change means that all of the test cases recently added to
'navigation.tests.ts' now pass, meaning that navigtion to the
definition of functions works through imports and goes to the
original function, even when that function is decorated.

* Add news entry for PR

* Improve framing of this

Author: PeterJCLaw

news/2 Fixes/1638.md

@@ -0,0 +1 @@
+Ensure navigation to definitons follows imports and is transparent to decoration ([#1638](https://github.com/Microsoft/vscode-python/issues/1638); thanks [Peter Law](https://github.com/PeterJCLaw))

pythonFiles/completion.py

@@ -570,21 +570,7 @@ def _process_request(self, request):
             sys_path=sys.path, environment=self.environment)
 
         if lookup == 'definitions':
-            defs = []
-            try:
-                defs = self._get_definitionsx(script.goto_definitions(follow_imports=False), request['id'])
-            except:
-                pass
-            try:
-                if len(defs) == 0:
-                    defs = self._get_definitionsx(script.goto_definitions(), request['id'])
-            except:
-                pass
-            try:
-                if len(defs) == 0:
-                    defs = self._get_definitionsx(script.goto_assignments(), request['id'])
-            except:
-                pass
+            defs = self._get_definitionsx(script.goto_assignments(follow_imports=True), request['id'])
             return json.dumps({'id': request['id'], 'results': defs})
         if lookup == 'tooltip':
             if jediPreview:

src/test/definitions/navigation.test.ts

@@ -0,0 +1,126 @@
+// Licensed under the MIT License.
+
+'use strict';
+import * as assert from 'assert';
+import * as path from 'path';
+import * as vscode from 'vscode';
+import { closeActiveWindows, initialize, initializeTest } from '../initialize';
+
+const decoratorsPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'pythonFiles', 'definition', 'navigation');
+const fileDefinitions = path.join(decoratorsPath, 'definitions.py');
+const fileUsages = path.join(decoratorsPath, 'usages.py');
+
+// tslint:disable-next-line:max-func-body-length
+suite('Definition Navigation', () => {
+    suiteSetup(initialize);
+    setup(initializeTest);
+    suiteTeardown(closeActiveWindows);
+    teardown(closeActiveWindows);
+
+    const assertFile = (expectedLocation: string, location: vscode.Uri) => {
+        const relLocation = vscode.workspace.asRelativePath(location);
+        const expectedRelLocation = vscode.workspace.asRelativePath(expectedLocation);
+        assert.equal(expectedRelLocation, relLocation, 'Position is in wrong file');
+    };
+
+    const formatPosition = (position: vscode.Position) => {
+        return `${position.line},${position.character}`;
+    };
+
+    const assertRange = (expectedRange: vscode.Range, range: vscode.Range) => {
+        assert.equal(formatPosition(expectedRange.start), formatPosition(range.start), 'Start position is incorrect');
+        assert.equal(formatPosition(expectedRange.end), formatPosition(range.end), 'End position is incorrect');
+    };
+
+    const buildTest = (startFile: string, startPosition: vscode.Position, expectedFile: string, expectedRange: vscode.Range) => {
+        return async () => {
+            const textDocument = await vscode.workspace.openTextDocument(startFile);
+            await vscode.window.showTextDocument(textDocument);
+            assert(vscode.window.activeTextEditor, 'No active editor');
+
+            const locations = await vscode.commands.executeCommand<vscode.Location[]>('vscode.executeDefinitionProvider', textDocument.uri, startPosition);
+            assert.equal(1, locations!.length, 'Wrong number of results');
+
+            const def = locations![0];
+            assertFile(expectedFile, def.uri);
+            assertRange(expectedRange, def.range!);
+        };
+    };
+
+    test('From own definition', buildTest(
+        fileDefinitions,
+        new vscode.Position(2, 6),
+        fileDefinitions,
+        new vscode.Range(2, 0, 11, 17)
+    ));
+
+    test('Nested function', buildTest(
+        fileDefinitions,
+        new vscode.Position(11, 16),
+        fileDefinitions,
+        new vscode.Range(6, 4, 10, 16)
+    ));
+
+    test('Decorator usage', buildTest(
+        fileDefinitions,
+        new vscode.Position(13, 1),
+        fileDefinitions,
+        new vscode.Range(2, 0, 11, 17)
+    ));
+
+    test('Function decorated by stdlib', buildTest(
+        fileDefinitions,
+        new vscode.Position(29, 6),
+        fileDefinitions,
+        new vscode.Range(21, 0, 27, 17)
+    ));
+
+    test('Function decorated by local decorator', buildTest(
+        fileDefinitions,
+        new vscode.Position(30, 6),
+        fileDefinitions,
+        new vscode.Range(14, 0, 18, 7)
+    ));
+
+    test('Module imported decorator usage', buildTest(
+        fileUsages,
+        new vscode.Position(3, 15),
+        fileDefinitions,
+        new vscode.Range(2, 0, 11, 17)
+    ));
+
+    test('Module imported function decorated by stdlib', buildTest(
+        fileUsages,
+        new vscode.Position(11, 19),
+        fileDefinitions,
+        new vscode.Range(21, 0, 27, 17)
+    ));
+
+    test('Module imported function decorated by local decorator', buildTest(
+        fileUsages,
+        new vscode.Position(12, 19),
+        fileDefinitions,
+        new vscode.Range(14, 0, 18, 7)
+    ));
+
+    test('Specifically imported decorator usage', buildTest(
+        fileUsages,
+        new vscode.Position(7, 1),
+        fileDefinitions,
+        new vscode.Range(2, 0, 11, 17)
+    ));
+
+    test('Specifically imported function decorated by stdlib', buildTest(
+        fileUsages,
+        new vscode.Position(14, 6),
+        fileDefinitions,
+        new vscode.Range(21, 0, 27, 17)
+    ));
+
+    test('Specifically imported function decorated by local decorator', buildTest(
+        fileUsages,
+        new vscode.Position(15, 6),
+        fileDefinitions,
+        new vscode.Range(14, 0, 18, 7)
+    ));
+});

src/test/pythonFiles/definition/navigation/__init__.py

@@ -0,0 +1,31 @@
+from contextlib import contextmanager
+
+def my_decorator(fn):
+    """
+    This is my decorator.
+    """
+    def wrapper(*args, **kwargs):
+        """
+        This is the wrapper.
+        """
+        return 42
+    return wrapper
+
+@my_decorator
+def thing(arg):
+    """
+    Thing which is decorated.
+    """
+    pass
+
+@contextmanager
+def my_context_manager():
+    """
+    This is my context manager.
+    """
+    print("before")
+    yield
+    print("after")
+
+with my_context_manager():
+    thing(19)

src/test/pythonFiles/definition/navigation/definitions.py

@@ -0,0 +1,16 @@
+import definitions
+from .definitions import my_context_manager, my_decorator, thing
+
+@definitions.my_decorator
+def one():
+    pass
+
+@my_decorator
+def two():
+    pass
+
+with definitions.my_context_manager():
+    definitions.thing(19)
+
+with my_context_manager():
+    thing(19)