Skip to content

Commit

Permalink
Fixed a bug that results in confusing hover information for an attrs … (
Browse files Browse the repository at this point in the history
#8883)

* Fixed a bug that results in confusing hover information for an attrs field that uses a converter. This addresses #8881.

* Restored previous behavior for modules.
  • Loading branch information
erictraut authored Sep 3, 2024
1 parent 354d467 commit 7abaf19
Show file tree
Hide file tree
Showing 12 changed files with 84 additions and 32 deletions.
8 changes: 4 additions & 4 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1584,7 +1584,7 @@ export class Checker extends ParseTreeWalker {
return false;
}

const decls = this._evaluator.getDeclarationsForNameNode(node.d.name);
const decls = this._evaluator.getDeclInfoForNameNode(node.d.name)?.decls;
if (!decls) {
return false;
}
Expand Down Expand Up @@ -2727,7 +2727,7 @@ export class Checker extends ParseTreeWalker {
// earlier overload. Typeshed stubs contain type: ignore comments on these
// lines, so it is important for us to report them in the same manner.
private _findNodeForOverload(functionNode: FunctionNode, overloadType: FunctionType): FunctionNode | undefined {
const decls = this._evaluator.getDeclarationsForNameNode(functionNode.d.name);
const decls = this._evaluator.getDeclInfoForNameNode(functionNode.d.name)?.decls;
if (!decls) {
return undefined;
}
Expand Down Expand Up @@ -4655,8 +4655,8 @@ export class Checker extends ParseTreeWalker {
// any variable declarations that are bound using nonlocal
// or global explicit bindings.
const declarations = this._evaluator
.getDeclarationsForNameNode(node)
?.filter((decl) => decl.type !== DeclarationType.Variable || !decl.isExplicitBinding);
.getDeclInfoForNameNode(node)
?.decls?.filter((decl) => decl.type !== DeclarationType.Variable || !decl.isExplicitBinding);

let primaryDeclaration =
declarations && declarations.length > 0 ? declarations[declarations.length - 1] : undefined;
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/analyzer/sourceMapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ export class SourceMapper {
return result;
}

const functionStubDecls = this._evaluator.getDeclarationsForNameNode(functionNode.d.name);
const functionStubDecls = this._evaluator.getDeclInfoForNameNode(functionNode.d.name)?.decls;
if (!functionStubDecls) {
return result;
}
Expand Down
21 changes: 14 additions & 7 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ import {
Reachability,
ResolveAliasOptions,
SolveConstraintsOptions,
SymbolDeclInfo,
TypeEvaluator,
TypeResult,
TypeResultWithNode,
Expand Down Expand Up @@ -21361,7 +21362,7 @@ export function createTypeEvaluator(
// In general, string nodes don't have any declarations associated with them, but
// we need to handle the special case of string literals used as keys within a
// dictionary expression where those keys are associated with a known TypedDict.
function getDeclarationsForStringNode(node: StringNode): Declaration[] | undefined {
function getDeclInfoForStringNode(node: StringNode): SymbolDeclInfo | undefined {
const declarations: Declaration[] = [];
const expectedType = getExpectedType(node)?.type;

Expand All @@ -21384,7 +21385,7 @@ export function createTypeEvaluator(
});
}

return declarations.length === 0 ? undefined : declarations;
return declarations.length === 0 ? undefined : { decls: declarations, synthesizedTypes: [] };
}

function getAliasFromImport(node: NameNode): NameNode | undefined {
Expand All @@ -21399,12 +21400,13 @@ export function createTypeEvaluator(
return undefined;
}

function getDeclarationsForNameNode(node: NameNode, skipUnreachableCode = true): Declaration[] | undefined {
function getDeclInfoForNameNode(node: NameNode, skipUnreachableCode = true): SymbolDeclInfo | undefined {
if (skipUnreachableCode && AnalyzerNodeInfo.isCodeUnreachable(node)) {
return undefined;
}

const declarations: Declaration[] = [];
const synthesizedTypes: Type[] = [];

// If the node is part of a "from X import Y as Z" statement and the node
// is the "Y" (non-aliased) name, we need to look up the alias symbol
Expand Down Expand Up @@ -21479,7 +21481,12 @@ export function createTypeEvaluator(
if (typedDecls.length > 0) {
appendArray(declarations, typedDecls);
} else {
appendArray(declarations, symbol.getDeclarations());
const synthesizedType = symbol.getSynthesizedType();
if (synthesizedType) {
synthesizedTypes.push(synthesizedType);
} else {
appendArray(declarations, symbol.getDeclarations());
}
}
}
});
Expand Down Expand Up @@ -21577,7 +21584,7 @@ export function createTypeEvaluator(
}
}

return declarations;
return { decls: declarations, synthesizedTypes };
}

function getTypeForDeclaration(declaration: Declaration): DeclaredSymbolTypeInfo {
Expand Down Expand Up @@ -27586,8 +27593,8 @@ export function createTypeEvaluator(
isAsymmetricAccessorAssignment,
suppressDiagnostics,
isSpecialFormClass,
getDeclarationsForStringNode,
getDeclarationsForNameNode,
getDeclInfoForStringNode,
getDeclInfoForNameNode,
getTypeForDeclaration,
resolveAliasDeclaration,
resolveAliasDeclarationWithInfo,
Expand Down
9 changes: 7 additions & 2 deletions packages/pyright-internal/src/analyzer/typeEvaluatorTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,11 @@ export interface CallSiteEvaluationInfo {
args: ValidateArgTypeParams[];
}

export interface SymbolDeclInfo {
decls: Declaration[];
synthesizedTypes: Type[];
}

export interface TypeEvaluator {
runWithCancellationToken<T>(token: CancellationToken, callback: () => T): T;

Expand Down Expand Up @@ -567,8 +572,8 @@ export interface TypeEvaluator {
suppressDiagnostics: (node: ParseNode, callback: () => void) => void;
isSpecialFormClass: (classType: ClassType, flags: AssignTypeFlags) => boolean;

getDeclarationsForStringNode: (node: StringNode) => Declaration[] | undefined;
getDeclarationsForNameNode: (node: NameNode, skipUnreachableCode?: boolean) => Declaration[] | undefined;
getDeclInfoForStringNode: (node: StringNode) => SymbolDeclInfo | undefined;
getDeclInfoForNameNode: (node: NameNode, skipUnreachableCode?: boolean) => SymbolDeclInfo | undefined;
getTypeForDeclaration: (declaration: Declaration) => DeclaredSymbolTypeInfo;
resolveAliasDeclaration: (
declaration: Declaration,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ class FindOutgoingCallTreeWalker extends ParseTreeWalker {
}

if (nameNode) {
const declarations = this._evaluator.getDeclarationsForNameNode(nameNode);
const declarations = this._evaluator.getDeclInfoForNameNode(nameNode)?.decls;

if (declarations) {
// TODO - it would be better if we could match the call to the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1412,7 +1412,7 @@ export class CompletionProvider {
return false;
}

const decls = this.evaluator.getDeclarationsForNameNode(curNode);
const decls = this.evaluator.getDeclInfoForNameNode(curNode)?.decls;
if (decls?.length !== 1 || !isVariableDeclaration(decls[0]) || decls[0].node !== curNode) {
return false;
}
Expand Down Expand Up @@ -2196,7 +2196,7 @@ export class CompletionProvider {
}

// Must be local variable/parameter
const declarations = this.evaluator.getDeclarationsForNameNode(indexNode.d.leftExpr) ?? [];
const declarations = this.evaluator.getDeclInfoForNameNode(indexNode.d.leftExpr)?.decls ?? [];
const declaration = declarations.length > 0 ? declarations[0] : undefined;
if (
!declaration ||
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,10 +169,10 @@ class DefinitionProviderBase {
// There should be only one 'definition', so only if extensions failed should we try again.
if (definitions.length === 0) {
if (node.nodeType === ParseNodeType.Name) {
const declarations = this.evaluator.getDeclarationsForNameNode(node);
const declarations = this.evaluator.getDeclInfoForNameNode(node)?.decls;
this.resolveDeclarations(declarations, definitions);
} else if (node.nodeType === ParseNodeType.String) {
const declarations = this.evaluator.getDeclarationsForStringNode(node);
const declarations = this.evaluator.getDeclInfoForStringNode(node)?.decls;
this.resolveDeclarations(declarations, definitions);
}
}
Expand Down Expand Up @@ -270,13 +270,13 @@ export class TypeDefinitionProvider extends DefinitionProviderBase {
// Fall back to Go To Definition if the type can't be found (ex. Go To Type Definition
// was executed on a type name)
if (declarations.length === 0) {
declarations = this.evaluator.getDeclarationsForNameNode(this.node) ?? [];
declarations = this.evaluator.getDeclInfoForNameNode(this.node)?.decls ?? [];
}

this.resolveDeclarations(declarations, definitions);
}
} else if (this.node.nodeType === ParseNodeType.String) {
const declarations = this.evaluator.getDeclarationsForStringNode(this.node);
const declarations = this.evaluator.getDeclInfoForStringNode(this.node)?.decls;
this.resolveDeclarations(declarations, definitions);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ function _getDeclarationsForNonModuleNameNode(
): Declaration[] {
assert(node.parent?.nodeType !== ParseNodeType.ModuleName);

let decls = evaluator.getDeclarationsForNameNode(node, skipUnreachableCode) || [];
let decls = evaluator.getDeclInfoForNameNode(node, skipUnreachableCode)?.decls || [];
if (node.parent?.nodeType === ParseNodeType.ImportFromAs) {
// Make sure we get the decl for this specific "from import" statement
decls = decls.filter((d) => d.node === node.parent);
Expand Down Expand Up @@ -467,7 +467,7 @@ function _getDeclarationsForNonModuleNameNode(

appendArray(
decls,
evaluator.getDeclarationsForNameNode(node.d.module.d.nameParts[0], skipUnreachableCode) || []
evaluator.getDeclInfoForNameNode(node.d.module.d.nameParts[0], skipUnreachableCode)?.decls || []
);
}

Expand Down Expand Up @@ -497,8 +497,9 @@ function _getDeclarationsForModuleNameNode(evaluator: TypeEvaluator, node: NameN
// we can match both "import X" and "from X import ..."
appendArray(
decls,
evaluator.getDeclarationsForNameNode(moduleName.d.nameParts[0])?.filter((d) => isAliasDeclaration(d)) ||
[]
evaluator
.getDeclInfoForNameNode(moduleName.d.nameParts[0])
?.decls?.filter((d) => isAliasDeclaration(d)) || []
);

if (decls.length === 0 || moduleName.parent.nodeType !== ParseNodeType.ImportAs) {
Expand Down Expand Up @@ -567,7 +568,7 @@ function _getDeclarationsForModuleNameNode(evaluator: TypeEvaluator, node: NameN
// "import X.Y" hold onto synthesized module type (without any decl).
// And "from X.Y import ..." doesn't have any symbol associated module names.
// they can't be referenced in the module.
return evaluator.getDeclarationsForNameNode(moduleName.d.nameParts[index]) || [];
return evaluator.getDeclInfoForNameNode(moduleName.d.nameParts[index])?.decls || [];
}

return [];
Expand Down
45 changes: 42 additions & 3 deletions packages/pyright-internal/src/languageService/hoverProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -254,11 +254,18 @@ export class HoverProvider {
node = node.parent.d.valueExpr;
}

const declarations = this._evaluator.getDeclarationsForNameNode(node);
const declInfo = this._evaluator.getDeclInfoForNameNode(node);
const declarations = declInfo?.decls;

if (declarations && declarations.length > 0) {
const primaryDeclaration = HoverProvider.getPrimaryDeclaration(declarations);
this._addResultsForDeclaration(results.parts, primaryDeclaration, node);
} else if (declInfo && declInfo.synthesizedTypes.length > 0) {
const name = node.d.value;
declInfo?.synthesizedTypes.forEach((type) => {
this._addResultsForSynthesizedType(results.parts, type, name);
});
this._addDocumentationPart(results.parts, node, /* resolvedDecl */ undefined);
} else if (!node.parent || node.parent.nodeType !== ParseNodeType.ModuleName) {
// If we had no declaration, see if we can provide a minimal tooltip. We'll skip
// this if it's part of a module name, since a module name part with no declaration
Expand Down Expand Up @@ -306,8 +313,14 @@ export class HoverProvider {
}

private _addResultsForDeclaration(parts: HoverTextPart[], declaration: Declaration, node: NameNode): void {
const resolvedDecl = this._evaluator.resolveAliasDeclaration(declaration, /* resolveLocalNames */ true);
if (!resolvedDecl || isUnresolvedAliasDeclaration(resolvedDecl)) {
const resolvedDecl =
declaration.type === DeclarationType.Alias
? this._evaluator.resolveAliasDeclaration(declaration, /* resolveLocalNames */ true)
: declaration;
if (
!resolvedDecl ||
(resolvedDecl.type === DeclarationType.Alias && isUnresolvedAliasDeclaration(resolvedDecl))
) {
this._addResultsPart(parts, `(import) ` + node.d.value + this._getTypeText(node), /* python */ true);
return;
}
Expand Down Expand Up @@ -442,6 +455,32 @@ export class HoverProvider {
}
}

private _addResultsForSynthesizedType(parts: HoverTextPart[], type: Type, name: string) {
let typeText: string;

if (isModule(type)) {
typeText = '(module) ' + name;
} else {
// Treat it as a function declaration if it's a function or overloaded type.
let label = 'variable';

if (isFunction(type) || isOverloaded(type)) {
label = 'function';
}

typeText = getToolTipForType(
type,
label,
name,
this._evaluator,
/* isProperty */ false,
this._functionSignatureDisplay
);
}

this._addResultsPart(parts, typeText, /* python */ true);
}

private _tryAddPartsForTypedDictKey(node: StringNode, type: Type, parts: HoverTextPart[]) {
// If the expected type is a TypedDict and the current node is a key entry then we can provide a tooltip
// with the type of the TypedDict key and its docstring, if available.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export class SignatureHelpProvider {
return undefined;
}

for (const decl of this._evaluator.getDeclarationsForNameNode(name) ?? []) {
for (const decl of this._evaluator.getDeclInfoForNameNode(name)?.decls ?? []) {
const resolveDecl = this._evaluator.resolveAliasDeclaration(decl, /* resolveLocalNames */ true);
if (!resolveDecl) {
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ export function getDocumentationPartsForTypeAndDecl(
) {
const name = resolvedDecl.node.d.module.d.nameParts.find((n) => n.d.value === optional.name);
if (name) {
const aliasDecls = evaluator.getDeclarationsForNameNode(name) ?? [resolvedDecl];
const aliasDecls = evaluator.getDeclInfoForNameNode(name)?.decls ?? [resolvedDecl];
resolvedDecl = aliasDecls.length > 0 ? aliasDecls[0] : resolvedDecl;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ test('resolve alias of not needed file', () => {
const markerUri = marker.fileUri;
const parseResults = state.workspace.service.getParseResults(markerUri)!;
const nameNode = findNodeByOffset(parseResults.parserOutput.parseTree, marker.position) as NameNode;
const aliasDecls = evaluator.getDeclarationsForNameNode(nameNode)!;
const aliasDecls = evaluator.getDeclInfoForNameNode(nameNode)!.decls;

// Unroot the file. we can't explicitly close the file since it will unload the file from test program.
state.workspace.service.test_program.getSourceFileInfo(markerUri)!.isOpenByClient = false;
Expand Down

0 comments on commit 7abaf19

Please sign in to comment.