Skip to content

Commit

Permalink
fix @override completion text edit being added to `__init_subclass_…
Browse files Browse the repository at this point in the history
…_` and metaclass members
  • Loading branch information
DetachHead committed Feb 24, 2025
1 parent 9d9de89 commit dd6875f
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 23 deletions.
2 changes: 1 addition & 1 deletion packages/browser-pyright/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"compilerOptions": {
"outDir": "./out",
"lib": [
"es2023",
"esnext",
"WebWorker",
"ScriptHost"
],
Expand Down
12 changes: 3 additions & 9 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ import { OperatorType, StringTokenFlags, TokenType } from '../parser/tokenizerTy
import { AnalyzerFileInfo } from './analyzerFileInfo';
import * as AnalyzerNodeInfo from './analyzerNodeInfo';
import { ConstraintTracker } from './constraintTracker';
import { getBoundCallMethod, getBoundInitMethod, getBoundNewMethod } from './constructors';
import { getBoundCallMethod, getBoundInitMethod, getBoundNewMethod, isMethodExemptFromLsp } from './constructors';
import { addInheritedDataClassEntries } from './dataClasses';
import { Declaration, DeclarationType, isAliasDeclaration, isVariableDeclaration } from './declaration';
import { getNameNodeForDeclaration, hasTypeForDeclaration } from './declarationUtils';
Expand Down Expand Up @@ -6656,7 +6656,7 @@ export class Checker extends ParseTreeWalker {
}

// Constructors are exempt.
if (this._isMethodExemptFromLsp(overrideFunction.shared.name)) {
if (isMethodExemptFromLsp(overrideFunction.shared.name)) {
return;
}

Expand All @@ -6682,12 +6682,6 @@ export class Checker extends ParseTreeWalker {
);
}

// Determines whether the name is exempt from Liskov Substitution Principle rules.
private _isMethodExemptFromLsp(name: string): boolean {
const exemptMethods = ['__init__', '__new__', '__init_subclass__', '__post_init__'];
return exemptMethods.some((n) => n === name);
}

// Determines whether the type is a function or overloaded function with an @override
// decorator. In this case, an error is reported because no base class has declared
// a method of the same name.
Expand Down Expand Up @@ -6819,7 +6813,7 @@ export class Checker extends ParseTreeWalker {
// are synthesized, and they can result in many overloads. We assume they
// are correct and will not produce any errors.
if (
!this._isMethodExemptFromLsp(memberName) &&
!isMethodExemptFromLsp(memberName) &&
!SymbolNameUtils.isPrivateName(memberName) &&
!ClassType.isTypedDictClass(childClassType)
) {
Expand Down
8 changes: 8 additions & 0 deletions packages/pyright-internal/src/analyzer/constructors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1114,3 +1114,11 @@ function isDefaultNewMethod(newMethod?: Type): boolean {

return true;
}

/**
* Determines whether the name is exempt from Liskov Substitution Principle rules.
*/
export function isMethodExemptFromLsp(name: string): boolean {
const exemptMethods = ['__init__', '__new__', '__init_subclass__', '__post_init__'];
return exemptMethods.some((n) => n === name);
}
39 changes: 34 additions & 5 deletions packages/pyright-internal/src/analyzer/typeUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2540,7 +2540,30 @@ export function convertToInstantiable(type: Type, includeSubclasses = true): Typ
return result;
}

export function getMembersForClass(classType: ClassType, symbolTable: SymbolTable, includeInstanceVars: boolean) {
// TODO: this is kinda gross. maybe this info can be a flag on the symbol instead?
/**
* @returns member names that came from the metaclass
*/
export function getMembersForClass(
classType: ClassType,
symbolTable: SymbolTable,
includeInstanceVars: false
): Set<string>;
export function getMembersForClass(
classType: ClassType,
symbolTable: SymbolTable,
includeInstanceVars: true
): undefined;
export function getMembersForClass(
classType: ClassType,
symbolTable: SymbolTable,
includeInstanceVars: boolean
): Set<string> | undefined;
export function getMembersForClass(
classType: ClassType,
symbolTable: SymbolTable,
includeInstanceVars: boolean
): Set<string> | undefined {
classType.shared.mro.forEach((mroClass) => {
if (isInstantiableClass(mroClass)) {
// Add any new member variables from this class.
Expand All @@ -2566,27 +2589,33 @@ export function getMembersForClass(classType: ClassType, symbolTable: SymbolTabl
});

// Add members of the metaclass as well.
if (!includeInstanceVars) {
if (includeInstanceVars) {
return undefined;
} else {
const metaclassMembers = new Set<string>();
const metaclass = classType.shared.effectiveMetaclass;
if (metaclass && isInstantiableClass(metaclass)) {
for (const mroClass of metaclass.shared.mro) {
if (isInstantiableClass(mroClass)) {
ClassType.getSymbolTable(mroClass).forEach((symbol, name) => {
const existingSymbol = symbolTable.get(name);

if (!existingSymbol) {
symbolTable.set(name, symbol);
} else if (!existingSymbol.hasTypedDeclarations() && symbol.hasTypedDeclarations()) {
if (
!existingSymbol ||
// If the existing symbol is unannotated but a parent class
// has an annotation for the symbol, use the parent type instead.
(!existingSymbol.hasTypedDeclarations() && symbol.hasTypedDeclarations())
) {
symbolTable.set(name, symbol);
metaclassMembers.add(name);
}
});
} else {
break;
}
}
}
return metaclassMembers;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ import { DocumentSymbolCollector } from './documentSymbolCollector';
import { getAutoImportText, getDocumentationPartsForTypeAndDecl } from './tooltipUtils';
import { ImportGroup } from '../analyzer/importStatementUtils';
import { TextEditAction } from '../common/editAction';
import { isMethodExemptFromLsp } from '../analyzer/constructors';

namespace Keywords {
const base: string[] = [
Expand Down Expand Up @@ -438,10 +439,13 @@ export class CompletionProvider {
}

const symbolTable = new Map<string, Symbol>();
let metaclassMemberNames = new Set<string>();
for (let i = 1; i < classResults.classType.shared.mro.length; i++) {
const mroClass = classResults.classType.shared.mro[i];
if (isInstantiableClass(mroClass)) {
getMembersForClass(mroClass, symbolTable, /* includeInstanceVars */ false);
metaclassMemberNames = metaclassMemberNames.union(
getMembersForClass(mroClass, symbolTable, /* includeInstanceVars */ false)
);
}
}

Expand Down Expand Up @@ -513,7 +517,13 @@ export class CompletionProvider {

// add the @override decorator if neeed
const additionalTextEdits: TextEditAction[] = [];
if (!['__init__', '__new__'].includes(name)) {

if (
// constructors don't need the override decorator
!isMethodExemptFromLsp(name) &&
// metaclass members should not have the override decorator because they aren't present on the instance
!metaclassMemberNames.has(name)
) {
const overrideDecorator = this.evaluator.getTypingType(decl.node, 'override');
if (
// this should always be true, but just in case
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ const additionalTextEdits = (markerNumber: number) => [
range: helper.getPositionRange('marker3'),
newText: '__call__(self, *args: Any, **kwds: Any) -> Any:\n ${0:pass}',
},
additionalTextEdits: additionalTextEdits(3),
},
],
},
Expand Down
4 changes: 2 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ requires-python = ">=3.8"
dependencies = [
# required by the basedpyright cli & langserver wrapper scripts. only binaries are required (no cli)
# since the user shouldn't have a node/npm binary unknowingly added to their PATH
"nodejs-wheel-binaries>=20.13.1",
"nodejs-wheel-binaries>=22.0.0",
]
[dependency-groups]
dev = [
"pylint>=3.0.0a7",
"ruff>=0.2.2",
"nodejs-wheel>=20.13.1",
"nodejs-wheel>=22.0.0",
"pdm-backend>=2.3.0",
"typing_extensions>=4.12.2",
"pytest>=8.2.2",
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"target": "es2021",
"module": "node16",
"lib": [
"es2023"
// when updating this, remember to also update the one in browser-pyright
"ESNext"
],
"skipLibCheck": true,
"esModuleInterop": true,
Expand Down
4 changes: 2 additions & 2 deletions uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit dd6875f

Please sign in to comment.