Skip to content

Commit 9bffde3

Browse files
varungandhi-srcofeki-neosec
authored andcommitted
fix: Avoid crash in the presence of decorators. (sourcegraph#184)
Changes the method lookup logic to use the same logic as Pyright internals instead of our own ad-hoc implementation of name resolution, which does not match Python's method resolution order (MRO), which is based purely on names, and not on types. This gets rid of a code path where we were doing a `!` operation, which triggered a crash when indexing a prospect's codebase. Fixes GRAPH-1278.
1 parent 476fae4 commit 9bffde3

7 files changed

Lines changed: 173 additions & 90 deletions

File tree

packages/pyright-internal/src/commands/dumpFileDebugInfoCommand.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -549,7 +549,8 @@ function getTypeCategoryString(typeCategory: TypeCategory, type: any) {
549549
}
550550
}
551551

552-
class TreeDumper extends ParseTreeWalker {
552+
// NOTE(scip-python): Exported for use in scip-python debugging
553+
export class TreeDumper extends ParseTreeWalker {
553554
private _indentation = '';
554555
private _output = '';
555556

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
from typing import TypeVar, Generic, Callable, Iterator, ParamSpec
2+
3+
_T_co = TypeVar("_T_co")
4+
_P = ParamSpec("_P")
5+
6+
class X(Generic[_T_co]):
7+
pass
8+
9+
def decorate(func: Callable[_P, Iterator[_T_co]]) -> Callable[_P, X[_T_co]]: ...
10+
11+
class Foo:
12+
@decorate
13+
def foo(self) -> Iterator[None]: ...
14+
15+
@decorate
16+
def noop():
17+
yield
18+
19+
class FooImpl(Foo):
20+
def foo(self):
21+
return noop()

packages/pyright-scip/snapshots/input/unique/inherits_class.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ class A:
22
def x(self) -> int:
33
raise NotImplemented
44

5-
def unmatched(self, x: int):
5+
def matched_despite_different_type(self, x: int):
66
pass
77

88
class B(A):
99
def x(self) -> int:
1010
return 5
1111

12-
def unmatched(self, x: int, y: int):
12+
def matched_despite_different_type(self, x: int, y: int):
1313
pass
1414

1515
def unrelated(self):
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
# < definition scip-python python snapshot-util 0.1 mwe/__init__:
2+
3+
from typing import TypeVar, Generic, Callable, Iterator, ParamSpec
4+
# ^^^^^^ reference python-stdlib 3.11 typing/__init__:
5+
# ^^^^^^^ reference python-stdlib 3.11 typing/TypeVar#
6+
# ^^^^^^^ reference python-stdlib 3.11 typing/Generic.
7+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable.
8+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator#
9+
# ^^^^^^^^^ reference python-stdlib 3.11 typing/ParamSpec#
10+
11+
_T_co = TypeVar("_T_co")
12+
#^^^^ definition snapshot-util 0.1 mwe/_T_co.
13+
# ^^^^^^^ reference python-stdlib 3.11 typing/TypeVar#
14+
_P = ParamSpec("_P")
15+
#^ definition snapshot-util 0.1 mwe/_P.
16+
# ^^^^^^^^^ reference python-stdlib 3.11 typing/ParamSpec#
17+
18+
class X(Generic[_T_co]):
19+
# ^ definition snapshot-util 0.1 mwe/X#
20+
# relationship implementation scip-python python python-stdlib 3.11 typing/Generic#
21+
# ^^^^^^^ reference python-stdlib 3.11 typing/Generic.
22+
# ^^^^^ reference snapshot-util 0.1 mwe/_T_co.
23+
pass
24+
25+
def decorate(func: Callable[_P, Iterator[_T_co]]) -> Callable[_P, X[_T_co]]: ...
26+
# ^^^^^^^^ definition snapshot-util 0.1 mwe/decorate().
27+
# ^^^^ definition snapshot-util 0.1 mwe/decorate().(func)
28+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable.
29+
# ^^ reference snapshot-util 0.1 mwe/_P.
30+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator#
31+
# ^^^^^ reference snapshot-util 0.1 mwe/_T_co.
32+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable.
33+
# ^^ reference snapshot-util 0.1 mwe/_P.
34+
# ^ reference snapshot-util 0.1 mwe/X#
35+
# ^^^^^ reference snapshot-util 0.1 mwe/_T_co.
36+
37+
class Foo:
38+
# ^^^ definition snapshot-util 0.1 mwe/Foo#
39+
@decorate
40+
# ^^^^^^^^ reference snapshot-util 0.1 mwe/decorate().
41+
def foo(self) -> Iterator[None]: ...
42+
# ^^^ definition snapshot-util 0.1 mwe/Foo#foo().
43+
# ^^^^ definition snapshot-util 0.1 mwe/Foo#foo().(self)
44+
# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator#
45+
46+
@decorate
47+
#^^^^^^^^ reference snapshot-util 0.1 mwe/decorate().
48+
def noop():
49+
# ^^^^ definition snapshot-util 0.1 mwe/noop().
50+
yield
51+
52+
class FooImpl(Foo):
53+
# ^^^^^^^ definition snapshot-util 0.1 mwe/FooImpl#
54+
# relationship implementation scip-python python snapshot-util 0.1 mwe/Foo#
55+
# ^^^ reference snapshot-util 0.1 mwe/Foo#
56+
def foo(self):
57+
# ^^^ definition snapshot-util 0.1 mwe/FooImpl#foo().
58+
# relationship implementation scip-python python snapshot-util 0.1 mwe/Foo#foo().
59+
# ^^^^ definition snapshot-util 0.1 mwe/FooImpl#foo().(self)
60+
return noop()
61+
# ^^^^ reference snapshot-util 0.1 mwe/noop().

packages/pyright-scip/snapshots/output/unique/inherits_class.py

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ def x(self) -> int:
99
raise NotImplemented
1010
# ^^^^^^^^^^^^^^ reference python-stdlib 3.11 builtins/NotImplemented#
1111

12-
def unmatched(self, x: int):
13-
# ^^^^^^^^^ definition snapshot-util 0.1 inherits_class/A#unmatched().
14-
# ^^^^ definition snapshot-util 0.1 inherits_class/A#unmatched().(self)
15-
# ^ definition snapshot-util 0.1 inherits_class/A#unmatched().(x)
16-
# ^^^ reference python-stdlib 3.11 builtins/int#
12+
def matched_despite_different_type(self, x: int):
13+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type().
14+
# ^^^^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type().(self)
15+
# ^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type().(x)
16+
# ^^^ reference python-stdlib 3.11 builtins/int#
1717
pass
1818

1919
class B(A):
@@ -27,13 +27,14 @@ def x(self) -> int:
2727
# ^^^ reference python-stdlib 3.11 builtins/int#
2828
return 5
2929

30-
def unmatched(self, x: int, y: int):
31-
# ^^^^^^^^^ definition snapshot-util 0.1 inherits_class/B#unmatched().
32-
# ^^^^ definition snapshot-util 0.1 inherits_class/B#unmatched().(self)
33-
# ^ definition snapshot-util 0.1 inherits_class/B#unmatched().(x)
34-
# ^^^ reference python-stdlib 3.11 builtins/int#
35-
# ^ definition snapshot-util 0.1 inherits_class/B#unmatched().(y)
36-
# ^^^ reference python-stdlib 3.11 builtins/int#
30+
def matched_despite_different_type(self, x: int, y: int):
31+
# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().
32+
# relationship implementation scip-python python snapshot-util 0.1 inherits_class/A#matched_despite_different_type().
33+
# ^^^^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(self)
34+
# ^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(x)
35+
# ^^^ reference python-stdlib 3.11 builtins/int#
36+
# ^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(y)
37+
# ^^^ reference python-stdlib 3.11 builtins/int#
3738
pass
3839

3940
def unrelated(self):

packages/pyright-scip/snapshots/output/unique/multiinherits_test.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ def three(self):
5454
def shared(self) -> bool:
5555
# ^^^^^^ definition snapshot-util 0.1 multiinherits_test/Multi#shared().
5656
# relationship implementation scip-python python snapshot-util 0.1 multiinherits_test/Left#shared().
57-
# relationship implementation scip-python python snapshot-util 0.1 multiinherits_test/Right#shared().
5857
# ^^^^ definition snapshot-util 0.1 multiinherits_test/Multi#shared().(self)
5958
# ^^^^ reference python-stdlib 3.11 builtins/bool#
6059
return True

packages/pyright-scip/src/treeVisitor.ts

Lines changed: 74 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import { TypeEvaluator } from 'pyright-internal/analyzer/typeEvaluatorTypes';
66
import { convertOffsetToPosition } from 'pyright-internal/common/positionUtils';
77
import { TextRange } from 'pyright-internal/common/textRange';
88
import { TextRangeCollection } from 'pyright-internal/common/textRangeCollection';
9+
import { printParseNodeType } from 'pyright-internal/analyzer/parseTreeUtils';
10+
import { TreeDumper } from 'pyright-internal/commands/dumpFileDebugInfoCommand';
911
import {
1012
AssignmentNode,
1113
CallNode,
@@ -40,7 +42,7 @@ import { SourceFile } from 'pyright-internal/analyzer/sourceFile';
4042
import { extractParameterDocumentation } from 'pyright-internal/analyzer/docStringUtils';
4143
import {
4244
Declaration,
43-
DeclarationType,
45+
DeclarationType, FunctionDeclaration,
4446
isAliasDeclaration,
4547
isIntrinsicDeclaration,
4648
} from 'pyright-internal/analyzer/declaration';
@@ -55,7 +57,7 @@ import { Event } from 'vscode-languageserver';
5557
import { HoverResults } from 'pyright-internal/languageService/hoverProvider';
5658
import { convertDocStringToMarkdown } from 'pyright-internal/analyzer/docStringConversion';
5759
import { assert } from 'pyright-internal/common/debug';
58-
import { getClassFieldsRecursive } from 'pyright-internal/analyzer/typeUtils';
60+
import { ClassMemberLookupFlags, lookUpClassMember } from 'pyright-internal/analyzer/typeUtils';
5961

6062
// Useful functions for later, but haven't gotten far enough yet to use them.
6163
// extractParameterDocumentation
@@ -222,6 +224,8 @@ export class TreeVisitor extends ParseTreeWalker {
222224
const fileInfo = getFileInfo(node);
223225
this.fileInfo = fileInfo;
224226

227+
// Pro tip: Use this.debugDumpAST(node, fileInfo) to see AST for debugging
228+
225229
// Insert definition at the top of the file
226230
const pythonPackage = this.getPackageInfo(node, fileInfo.moduleName);
227231
if (pythonPackage) {
@@ -440,54 +444,26 @@ export class TreeVisitor extends ParseTreeWalker {
440444
let relationshipMap: Map<string, scip.Relationship> = new Map();
441445
let classType = enclosingClassType.classType;
442446

443-
// Use: getClassMemberIterator
444-
// Could use this to handle each of the fields with the same name
445-
// but it's a bit weird if you have A -> B -> C, and then you say
446-
// that C implements A's & B's... that seems perhaps a bit too verbose.
447-
//
448-
// See: https://github.com/sourcegraph/scip-python/issues/50
449-
for (const base of classType.details.baseClasses) {
450-
if (base.category !== TypeCategory.Class) {
451-
continue;
452-
}
453-
454-
let parentMethod = base.details.fields.get(node.name.value);
455-
if (!parentMethod) {
456-
let fieldLookup = getClassFieldsRecursive(base).get(node.name.value);
457-
if (fieldLookup && fieldLookup.classType.category !== TypeCategory.Unknown) {
458-
parentMethod = fieldLookup.classType.details.fields.get(node.name.value)!;
459-
} else {
460-
continue;
461-
}
462-
}
447+
let classMember = lookUpClassMember(classType, node.name.value, ClassMemberLookupFlags.SkipOriginalClass);
448+
if (!classMember) {
449+
return undefined;
450+
}
463451

464-
let parentMethodType = this.evaluator.getEffectiveTypeOfSymbol(parentMethod);
465-
if (parentMethodType.category !== TypeCategory.Function) {
452+
const superDecls = classMember.symbol.getDeclarations();
453+
for (const superDecl of superDecls) {
454+
if (superDecl.type !== DeclarationType.Function) {
466455
continue;
467456
}
468-
469-
if (
470-
!ModifiedTypeUtils.isTypeImplementable(
471-
functionType.functionType,
472-
parentMethodType,
473-
false,
474-
true,
475-
0,
476-
true
477-
)
478-
) {
479-
continue;
457+
let symbol = this.getFunctionSymbol(superDecl);
458+
if (!symbol.isLocal()) {
459+
relationshipMap.set(
460+
symbol.value,
461+
new scip.Relationship({
462+
symbol: symbol.value,
463+
is_implementation: true,
464+
})
465+
);
480466
}
481-
482-
let decl = parentMethodType.details.declaration!;
483-
let symbol = this.typeToSymbol(decl.node.name, decl.node, parentMethodType);
484-
relationshipMap.set(
485-
symbol.value,
486-
new scip.Relationship({
487-
symbol: symbol.value,
488-
is_implementation: true,
489-
})
490-
);
491467
}
492468

493469
let relationships = Array.from(relationshipMap.values());
@@ -610,7 +586,17 @@ export class TreeVisitor extends ParseTreeWalker {
610586
if (
611587
importInfo &&
612588
importInfo.resolvedPaths[0] &&
613-
path.resolve(importInfo.resolvedPaths[0]).startsWith(this.cwd)
589+
((): boolean => {
590+
// HACK(id: inconsistent-casing-of-resolved-paths):
591+
// Sometimes the resolvedPath is normalized and sometimes it is not.
592+
// If we remove one of the two checks below, existing tests start failing
593+
// (aliased_import and nested_items tests). So do both checks.
594+
const resolvedPath = path.resolve(importInfo.resolvedPaths[0])
595+
assertSometimesNormalized(resolvedPath, 'visitImportAs.resolvedPath')
596+
return resolvedPath.startsWith(this.cwd) ||
597+
resolvedPath.startsWith(
598+
normalizePathCase(new PyrightFileSystem(createFromRealFileSystem()), this.cwd))
599+
})()
614600
) {
615601
const symbol = Symbols.makeModuleInit(this.projectPackage, moduleName);
616602
this.pushNewOccurrence(node.module, symbol);
@@ -1527,44 +1513,49 @@ export class TreeVisitor extends ParseTreeWalker {
15271513
}
15281514
}
15291515

1516+
private getFunctionSymbol(decl: FunctionDeclaration): ScipSymbol {
1517+
const declModuleName = decl.moduleName;
1518+
let pythonPackage = this.guessPackage(declModuleName, decl.path);
1519+
if (!pythonPackage) {
1520+
return ScipSymbol.local(this.counter.next());
1521+
}
1522+
1523+
const enclosingClass = ParseTreeUtils.getEnclosingClass(decl.node);
1524+
if (enclosingClass) {
1525+
const enclosingClassType = this.evaluator.getTypeOfClass(enclosingClass);
1526+
if (enclosingClassType) {
1527+
let classType = enclosingClassType.classType;
1528+
const pythonPackage = this.guessPackage(classType.details.moduleName, classType.details.filePath)!;
1529+
const symbol = Symbols.makeClass(pythonPackage, classType.details.moduleName, classType.details.name);
1530+
return Symbols.makeMethod(symbol, decl.node.name.value);
1531+
}
1532+
return ScipSymbol.local(this.counter.next());
1533+
} else {
1534+
return Symbols.makeMethod(Symbols.makeModule(pythonPackage, declModuleName), decl.node.name.value);
1535+
}
1536+
}
1537+
1538+
// NOTE(tech-debt): typeToSymbol's signature doesn't make sense. It returns the
1539+
// symbol for a _function_ (not the function's _type_) despite the name being
1540+
// 'typeToSymbol'. More generally, we should have dedicated functions to get
1541+
// the symbol based on the specific declarations, like getFunctionSymbol.
1542+
// There can be a general function which gets the symbol for a variety of kinds
1543+
// of _declarations_, but it must not take a _Type_ as an argument
1544+
// (Python mostly doesn't use structural types, so ~only declarations should
1545+
// have symbols).
1546+
15301547
// Take a `Type` from pyright and turn that into an LSIF symbol.
15311548
private typeToSymbol(node: NameNode, declNode: ParseNode, typeObj: Type): ScipSymbol {
15321549
if (Types.isFunction(typeObj)) {
15331550
// TODO: Possibly worth checking for parent declarations.
15341551
// I'm not sure if that will actually work though for types.
1535-
15361552
const decl = typeObj.details.declaration;
15371553
if (!decl) {
15381554
// throw 'Unhandled missing declaration for type: function';
15391555
// console.warn('Missing Function Decl:', node.token.value, typeObj);
15401556
return ScipSymbol.local(this.counter.next());
15411557
}
1542-
1543-
const declModuleName = decl.moduleName;
1544-
let pythonPackage = this.guessPackage(declModuleName, decl.path);
1545-
if (!pythonPackage) {
1546-
return ScipSymbol.local(this.counter.next());
1547-
}
1548-
1549-
const enclosingClass = ParseTreeUtils.getEnclosingClass(declNode);
1550-
if (enclosingClass) {
1551-
const enclosingClassType = this.evaluator.getTypeOfClass(enclosingClass);
1552-
if (enclosingClassType) {
1553-
let classType = enclosingClassType.classType;
1554-
const pythonPackage = this.guessPackage(classType.details.moduleName, classType.details.filePath)!;
1555-
const symbol = Symbols.makeClass(
1556-
pythonPackage,
1557-
classType.details.moduleName,
1558-
classType.details.name
1559-
);
1560-
1561-
return Symbols.makeMethod(symbol, node.value);
1562-
}
1563-
1564-
return ScipSymbol.local(this.counter.next());
1565-
} else {
1566-
return Symbols.makeMethod(Symbols.makeModule(pythonPackage, typeObj.details.moduleName), node.value);
1567-
}
1558+
return this.getFunctionSymbol(decl);
15681559
} else if (Types.isClass(typeObj)) {
15691560
const pythonPackage = this.getPackageInfo(node, typeObj.details.moduleName)!;
15701561
return Symbols.makeClass(pythonPackage, typeObj.details.moduleName, node.value);
@@ -1936,6 +1927,15 @@ export class TreeVisitor extends ParseTreeWalker {
19361927
}
19371928
return undefined;
19381929
}
1930+
1931+
private debugDumpAST(node: ModuleNode, fileInfo: AnalyzerFileInfo): void {
1932+
console.log("\n=== AST DUMP ===");
1933+
const dumper = new TreeDumper("", fileInfo.lines);
1934+
dumper.walk(node);
1935+
console.log(dumper.output);
1936+
console.log("=== END AST DUMP ===\n");
1937+
}
1938+
19391939
}
19401940

19411941
function _formatModuleName(node: ModuleNameNode): string {

0 commit comments

Comments
 (0)