From 5d4d8688b853b4dc5dc720976c3ad1fd955631d2 Mon Sep 17 00:00:00 2001 From: Varun Gandhi Date: Fri, 15 Aug 2025 13:18:38 +0800 Subject: [PATCH] fix: Avoid crash in the presence of decorators. --- .../src/commands/dumpFileDebugInfoCommand.ts | 3 +- .../snapshots/input/graph_1278/mwe.py | 21 +++ .../snapshots/input/unique/inherits_class.py | 4 +- .../snapshots/output/graph_1278/mwe.py | 61 ++++++++ .../snapshots/output/unique/inherits_class.py | 25 ++-- .../output/unique/multiinherits_test.py | 1 - packages/pyright-scip/src/treeVisitor.ts | 137 ++++++++---------- 7 files changed, 162 insertions(+), 90 deletions(-) create mode 100644 packages/pyright-scip/snapshots/input/graph_1278/mwe.py create mode 100644 packages/pyright-scip/snapshots/output/graph_1278/mwe.py diff --git a/packages/pyright-internal/src/commands/dumpFileDebugInfoCommand.ts b/packages/pyright-internal/src/commands/dumpFileDebugInfoCommand.ts index 02e2628b3..a46a39beb 100644 --- a/packages/pyright-internal/src/commands/dumpFileDebugInfoCommand.ts +++ b/packages/pyright-internal/src/commands/dumpFileDebugInfoCommand.ts @@ -549,7 +549,8 @@ function getTypeCategoryString(typeCategory: TypeCategory, type: any) { } } -class TreeDumper extends ParseTreeWalker { +// NOTE(scip-python): Exported for use in scip-python debugging +export class TreeDumper extends ParseTreeWalker { private _indentation = ''; private _output = ''; diff --git a/packages/pyright-scip/snapshots/input/graph_1278/mwe.py b/packages/pyright-scip/snapshots/input/graph_1278/mwe.py new file mode 100644 index 000000000..bdafcec6b --- /dev/null +++ b/packages/pyright-scip/snapshots/input/graph_1278/mwe.py @@ -0,0 +1,21 @@ +from typing import TypeVar, Generic, Callable, Iterator, ParamSpec + +_T_co = TypeVar("_T_co") +_P = ParamSpec("_P") + +class X(Generic[_T_co]): + pass + +def decorate(func: Callable[_P, Iterator[_T_co]]) -> Callable[_P, X[_T_co]]: ... + +class Foo: + @decorate + def foo(self) -> Iterator[None]: ... + +@decorate +def noop(): + yield + +class FooImpl(Foo): + def foo(self): + return noop() \ No newline at end of file diff --git a/packages/pyright-scip/snapshots/input/unique/inherits_class.py b/packages/pyright-scip/snapshots/input/unique/inherits_class.py index ad5d43695..0c2707555 100644 --- a/packages/pyright-scip/snapshots/input/unique/inherits_class.py +++ b/packages/pyright-scip/snapshots/input/unique/inherits_class.py @@ -2,14 +2,14 @@ class A: def x(self) -> int: raise NotImplemented - def unmatched(self, x: int): + def matched_despite_different_type(self, x: int): pass class B(A): def x(self) -> int: return 5 - def unmatched(self, x: int, y: int): + def matched_despite_different_type(self, x: int, y: int): pass def unrelated(self): diff --git a/packages/pyright-scip/snapshots/output/graph_1278/mwe.py b/packages/pyright-scip/snapshots/output/graph_1278/mwe.py new file mode 100644 index 000000000..dede5734e --- /dev/null +++ b/packages/pyright-scip/snapshots/output/graph_1278/mwe.py @@ -0,0 +1,61 @@ +# < definition scip-python python snapshot-util 0.1 mwe/__init__: + +from typing import TypeVar, Generic, Callable, Iterator, ParamSpec +# ^^^^^^ reference python-stdlib 3.11 typing/__init__: +# ^^^^^^^ reference python-stdlib 3.11 typing/TypeVar# +# ^^^^^^^ reference python-stdlib 3.11 typing/Generic. +# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable. +# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator# +# ^^^^^^^^^ reference python-stdlib 3.11 typing/ParamSpec# + +_T_co = TypeVar("_T_co") +#^^^^ definition snapshot-util 0.1 mwe/_T_co. +# ^^^^^^^ reference python-stdlib 3.11 typing/TypeVar# +_P = ParamSpec("_P") +#^ definition snapshot-util 0.1 mwe/_P. +# ^^^^^^^^^ reference python-stdlib 3.11 typing/ParamSpec# + +class X(Generic[_T_co]): +# ^ definition snapshot-util 0.1 mwe/X# +# relationship implementation scip-python python python-stdlib 3.11 typing/Generic# +# ^^^^^^^ reference python-stdlib 3.11 typing/Generic. +# ^^^^^ reference snapshot-util 0.1 mwe/_T_co. + pass + +def decorate(func: Callable[_P, Iterator[_T_co]]) -> Callable[_P, X[_T_co]]: ... +# ^^^^^^^^ definition snapshot-util 0.1 mwe/decorate(). +# ^^^^ definition snapshot-util 0.1 mwe/decorate().(func) +# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable. +# ^^ reference snapshot-util 0.1 mwe/_P. +# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator# +# ^^^^^ reference snapshot-util 0.1 mwe/_T_co. +# ^^^^^^^^ reference python-stdlib 3.11 typing/Callable. +# ^^ reference snapshot-util 0.1 mwe/_P. +# ^ reference snapshot-util 0.1 mwe/X# +# ^^^^^ reference snapshot-util 0.1 mwe/_T_co. + +class Foo: +# ^^^ definition snapshot-util 0.1 mwe/Foo# + @decorate +# ^^^^^^^^ reference snapshot-util 0.1 mwe/decorate(). + def foo(self) -> Iterator[None]: ... +# ^^^ definition snapshot-util 0.1 mwe/Foo#foo(). +# ^^^^ definition snapshot-util 0.1 mwe/Foo#foo().(self) +# ^^^^^^^^ reference python-stdlib 3.11 typing/Iterator# + +@decorate +#^^^^^^^^ reference snapshot-util 0.1 mwe/decorate(). +def noop(): +# ^^^^ definition snapshot-util 0.1 mwe/noop(). + yield + +class FooImpl(Foo): +# ^^^^^^^ definition snapshot-util 0.1 mwe/FooImpl# +# relationship implementation scip-python python snapshot-util 0.1 mwe/Foo# +# ^^^ reference snapshot-util 0.1 mwe/Foo# + def foo(self): +# ^^^ definition snapshot-util 0.1 mwe/FooImpl#foo(). +# relationship implementation scip-python python snapshot-util 0.1 mwe/Foo#foo(). +# ^^^^ definition snapshot-util 0.1 mwe/FooImpl#foo().(self) + return noop() +# ^^^^ reference snapshot-util 0.1 mwe/noop(). diff --git a/packages/pyright-scip/snapshots/output/unique/inherits_class.py b/packages/pyright-scip/snapshots/output/unique/inherits_class.py index 54efa385c..1a83d316e 100644 --- a/packages/pyright-scip/snapshots/output/unique/inherits_class.py +++ b/packages/pyright-scip/snapshots/output/unique/inherits_class.py @@ -9,11 +9,11 @@ def x(self) -> int: raise NotImplemented # ^^^^^^^^^^^^^^ reference python-stdlib 3.11 builtins/NotImplemented# - def unmatched(self, x: int): -# ^^^^^^^^^ definition snapshot-util 0.1 inherits_class/A#unmatched(). -# ^^^^ definition snapshot-util 0.1 inherits_class/A#unmatched().(self) -# ^ definition snapshot-util 0.1 inherits_class/A#unmatched().(x) -# ^^^ reference python-stdlib 3.11 builtins/int# + def matched_despite_different_type(self, x: int): +# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type(). +# ^^^^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type().(self) +# ^ definition snapshot-util 0.1 inherits_class/A#matched_despite_different_type().(x) +# ^^^ reference python-stdlib 3.11 builtins/int# pass class B(A): @@ -27,13 +27,14 @@ def x(self) -> int: # ^^^ reference python-stdlib 3.11 builtins/int# return 5 - def unmatched(self, x: int, y: int): -# ^^^^^^^^^ definition snapshot-util 0.1 inherits_class/B#unmatched(). -# ^^^^ definition snapshot-util 0.1 inherits_class/B#unmatched().(self) -# ^ definition snapshot-util 0.1 inherits_class/B#unmatched().(x) -# ^^^ reference python-stdlib 3.11 builtins/int# -# ^ definition snapshot-util 0.1 inherits_class/B#unmatched().(y) -# ^^^ reference python-stdlib 3.11 builtins/int# + def matched_despite_different_type(self, x: int, y: int): +# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type(). +# relationship implementation scip-python python snapshot-util 0.1 inherits_class/A#matched_despite_different_type(). +# ^^^^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(self) +# ^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(x) +# ^^^ reference python-stdlib 3.11 builtins/int# +# ^ definition snapshot-util 0.1 inherits_class/B#matched_despite_different_type().(y) +# ^^^ reference python-stdlib 3.11 builtins/int# pass def unrelated(self): diff --git a/packages/pyright-scip/snapshots/output/unique/multiinherits_test.py b/packages/pyright-scip/snapshots/output/unique/multiinherits_test.py index 8901f3999..38ce5a4ae 100644 --- a/packages/pyright-scip/snapshots/output/unique/multiinherits_test.py +++ b/packages/pyright-scip/snapshots/output/unique/multiinherits_test.py @@ -54,7 +54,6 @@ def three(self): def shared(self) -> bool: # ^^^^^^ definition snapshot-util 0.1 multiinherits_test/Multi#shared(). # relationship implementation scip-python python snapshot-util 0.1 multiinherits_test/Left#shared(). -# relationship implementation scip-python python snapshot-util 0.1 multiinherits_test/Right#shared(). # ^^^^ definition snapshot-util 0.1 multiinherits_test/Multi#shared().(self) # ^^^^ reference python-stdlib 3.11 builtins/bool# return True diff --git a/packages/pyright-scip/src/treeVisitor.ts b/packages/pyright-scip/src/treeVisitor.ts index 8797b92e2..94827d924 100644 --- a/packages/pyright-scip/src/treeVisitor.ts +++ b/packages/pyright-scip/src/treeVisitor.ts @@ -6,6 +6,8 @@ import { TypeEvaluator } from 'pyright-internal/analyzer/typeEvaluatorTypes'; import { convertOffsetToPosition } from 'pyright-internal/common/positionUtils'; import { TextRange } from 'pyright-internal/common/textRange'; import { TextRangeCollection } from 'pyright-internal/common/textRangeCollection'; +import { printParseNodeType } from 'pyright-internal/analyzer/parseTreeUtils'; +import { TreeDumper } from 'pyright-internal/commands/dumpFileDebugInfoCommand'; import { AssignmentNode, ClassNode, @@ -38,7 +40,7 @@ import { SourceFile } from 'pyright-internal/analyzer/sourceFile'; import { extractParameterDocumentation } from 'pyright-internal/analyzer/docStringUtils'; import { Declaration, - DeclarationType, + DeclarationType, FunctionDeclaration, isAliasDeclaration, isIntrinsicDeclaration, } from 'pyright-internal/analyzer/declaration'; @@ -53,7 +55,7 @@ import { Event } from 'vscode-languageserver'; import { HoverResults } from 'pyright-internal/languageService/hoverProvider'; import { convertDocStringToMarkdown } from 'pyright-internal/analyzer/docStringConversion'; import { assert } from 'pyright-internal/common/debug'; -import { getClassFieldsRecursive } from 'pyright-internal/analyzer/typeUtils'; +import { ClassMemberLookupFlags, lookUpClassMember } from 'pyright-internal/analyzer/typeUtils'; import { PyrightFileSystem } from "pyright-internal/pyrightFileSystem"; import { createFromRealFileSystem } from "pyright-internal/common/realFileSystem"; import { normalizePathCase } from "pyright-internal/common/pathUtils"; @@ -218,6 +220,8 @@ export class TreeVisitor extends ParseTreeWalker { const fileInfo = getFileInfo(node); this.fileInfo = fileInfo; + // Pro tip: Use this.debugDumpAST(node, fileInfo) to see AST for debugging + // Insert definition at the top of the file const pythonPackage = this.getPackageInfo(node, fileInfo.moduleName); if (pythonPackage) { @@ -342,54 +346,26 @@ export class TreeVisitor extends ParseTreeWalker { let relationshipMap: Map = new Map(); let classType = enclosingClassType.classType; - // Use: getClassMemberIterator - // Could use this to handle each of the fields with the same name - // but it's a bit weird if you have A -> B -> C, and then you say - // that C implements A's & B's... that seems perhaps a bit too verbose. - // - // See: https://github.com/sourcegraph/scip-python/issues/50 - for (const base of classType.details.baseClasses) { - if (base.category !== TypeCategory.Class) { - continue; - } - - let parentMethod = base.details.fields.get(node.name.value); - if (!parentMethod) { - let fieldLookup = getClassFieldsRecursive(base).get(node.name.value); - if (fieldLookup && fieldLookup.classType.category !== TypeCategory.Unknown) { - parentMethod = fieldLookup.classType.details.fields.get(node.name.value)!; - } else { - continue; - } - } + let classMember = lookUpClassMember(classType, node.name.value, ClassMemberLookupFlags.SkipOriginalClass); + if (!classMember) { + return undefined; + } - let parentMethodType = this.evaluator.getEffectiveTypeOfSymbol(parentMethod); - if (parentMethodType.category !== TypeCategory.Function) { + const superDecls = classMember.symbol.getDeclarations(); + for (const superDecl of superDecls) { + if (superDecl.type !== DeclarationType.Function) { continue; } - - if ( - !ModifiedTypeUtils.isTypeImplementable( - functionType.functionType, - parentMethodType, - false, - true, - 0, - true - ) - ) { - continue; + let symbol = this.getFunctionSymbol(superDecl); + if (!symbol.isLocal()) { + relationshipMap.set( + symbol.value, + new scip.Relationship({ + symbol: symbol.value, + is_implementation: true, + }) + ); } - - let decl = parentMethodType.details.declaration!; - let symbol = this.typeToSymbol(decl.node.name, decl.node, parentMethodType); - relationshipMap.set( - symbol.value, - new scip.Relationship({ - symbol: symbol.value, - is_implementation: true, - }) - ); } let relationships = Array.from(relationshipMap.values()); @@ -515,7 +491,6 @@ export class TreeVisitor extends ParseTreeWalker { // (aliased_import and nested_items tests). So do both checks. const resolvedPath = path.resolve(importInfo.resolvedPaths[0]) assertSometimesNormalized(resolvedPath, 'visitImportAs.resolvedPath') - return resolvedPath.startsWith(this.cwd) || resolvedPath.startsWith( normalizePathCase(new PyrightFileSystem(createFromRealFileSystem()), this.cwd)) @@ -1320,44 +1295,49 @@ export class TreeVisitor extends ParseTreeWalker { } } + private getFunctionSymbol(decl: FunctionDeclaration): ScipSymbol { + const declModuleName = decl.moduleName; + let pythonPackage = this.guessPackage(declModuleName, decl.path); + if (!pythonPackage) { + return ScipSymbol.local(this.counter.next()); + } + + const enclosingClass = ParseTreeUtils.getEnclosingClass(decl.node); + if (enclosingClass) { + const enclosingClassType = this.evaluator.getTypeOfClass(enclosingClass); + if (enclosingClassType) { + let classType = enclosingClassType.classType; + const pythonPackage = this.guessPackage(classType.details.moduleName, classType.details.filePath)!; + const symbol = Symbols.makeClass(pythonPackage, classType.details.moduleName, classType.details.name); + return Symbols.makeMethod(symbol, decl.node.name.value); + } + return ScipSymbol.local(this.counter.next()); + } else { + return Symbols.makeMethod(Symbols.makeModule(pythonPackage, declModuleName), decl.node.name.value); + } + } + + // NOTE(tech-debt): typeToSymbol's signature doesn't make sense. It returns the + // symbol for a _function_ (not the function's _type_) despite the name being + // 'typeToSymbol'. More generally, we should have dedicated functions to get + // the symbol based on the specific declarations, like getFunctionSymbol. + // There can be a general function which gets the symbol for a variety of kinds + // of _declarations_, but it must not take a _Type_ as an argument + // (Python mostly doesn't use structural types, so ~only declarations should + // have symbols). + // Take a `Type` from pyright and turn that into an LSIF symbol. private typeToSymbol(node: NameNode, declNode: ParseNode, typeObj: Type): ScipSymbol { if (Types.isFunction(typeObj)) { // TODO: Possibly worth checking for parent declarations. // I'm not sure if that will actually work though for types. - const decl = typeObj.details.declaration; if (!decl) { // throw 'Unhandled missing declaration for type: function'; // console.warn('Missing Function Decl:', node.token.value, typeObj); return ScipSymbol.local(this.counter.next()); } - - const declModuleName = decl.moduleName; - let pythonPackage = this.guessPackage(declModuleName, decl.path); - if (!pythonPackage) { - return ScipSymbol.local(this.counter.next()); - } - - const enclosingClass = ParseTreeUtils.getEnclosingClass(declNode); - if (enclosingClass) { - const enclosingClassType = this.evaluator.getTypeOfClass(enclosingClass); - if (enclosingClassType) { - let classType = enclosingClassType.classType; - const pythonPackage = this.guessPackage(classType.details.moduleName, classType.details.filePath)!; - const symbol = Symbols.makeClass( - pythonPackage, - classType.details.moduleName, - classType.details.name - ); - - return Symbols.makeMethod(symbol, node.value); - } - - return ScipSymbol.local(this.counter.next()); - } else { - return Symbols.makeMethod(Symbols.makeModule(pythonPackage, typeObj.details.moduleName), node.value); - } + return this.getFunctionSymbol(decl); } else if (Types.isClass(typeObj)) { const pythonPackage = this.getPackageInfo(node, typeObj.details.moduleName)!; return Symbols.makeClass(pythonPackage, typeObj.details.moduleName, node.value); @@ -1700,6 +1680,15 @@ export class TreeVisitor extends ParseTreeWalker { this.rawSetLsifSymbol(node, symbol, symbol.isLocal()); return symbol; } + + private debugDumpAST(node: ModuleNode, fileInfo: AnalyzerFileInfo): void { + console.log("\n=== AST DUMP ==="); + const dumper = new TreeDumper("", fileInfo.lines); + dumper.walk(node); + console.log(dumper.output); + console.log("=== END AST DUMP ===\n"); + } + } function _formatModuleName(node: ModuleNameNode): string {