Skip to content

Commit

Permalink
Modified the reportMissingSuperCall diagnostic based on feedback. I…
Browse files Browse the repository at this point in the history
…t now emits an error for classes that include an `__init__` and don't call through to `super().__init__` even if that class derives from `object`, unless it's marked `@final`.
  • Loading branch information
msfterictraut committed Jan 16, 2022
1 parent 55ef66b commit 846cee3
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 73 deletions.
80 changes: 21 additions & 59 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4489,24 +4489,23 @@ export class Checker extends ParseTreeWalker {
return;
}

// Determine which parent classes expose a same-named method.
let baseClassesToCall: ClassType[] = [];
classType.details.baseClasses.forEach((baseClass) => {
if (isClass(baseClass)) {
const methodMember = lookUpClassMember(
baseClass,
methodType.details.name,
ClassMemberLookupFlags.SkipInstanceVariables | ClassMemberLookupFlags.SkipObjectBaseClass
);
// If the class is marked final, we can skip the "object" base class
// because we know that the `__init__` method in `object` doesn't do
// anything. It's not safe to do this if the class isn't final because
// it could be combined with other classes in a multi-inheritance
// situation that effectively adds new superclasses that we don't know
// about statically.
let effectiveFlags = ClassMemberLookupFlags.SkipInstanceVariables | ClassMemberLookupFlags.SkipOriginalClass;
if (ClassType.isFinal(classType)) {
effectiveFlags |= ClassMemberLookupFlags.SkipObjectBaseClass;
}

const methodMember = lookUpClassMember(classType, methodType.details.name, effectiveFlags);
if (!methodMember) {
return;
}

if (methodMember && isClass(methodMember.classType)) {
const methodClass = methodMember.classType;
if (!baseClassesToCall.find((cls) => ClassType.isSameGenericClass(cls, methodClass))) {
baseClassesToCall.push(methodClass);
}
}
}
});
let foundCallOfMember = false;

// Now scan the implementation of the method to determine whether
// super().<method> has been called for all of the required base classes.
Expand All @@ -4522,67 +4521,30 @@ export class Checker extends ParseTreeWalker {
memberBaseExpr.leftExpression.nodeType === ParseNodeType.Name &&
memberBaseExpr.leftExpression.value === 'super'
) {
let targetClassType: Type | undefined;

// Is this a zero-argument call to 'super', or is an explicit
// target class provided?
if (memberBaseExpr.arguments.length === 0) {
targetClassType = classType;
} else {
targetClassType = this._evaluator.getTypeOfExpression(
memberBaseExpr.arguments[0].valueExpression
).type;
targetClassType = this._evaluator.makeTopLevelTypeVarsConcrete(targetClassType);
}

if (isAnyOrUnknown(targetClassType)) {
// If the target class is Any or unknown, all bets are off.
baseClassesToCall = [];
} else if (isInstantiableClass(targetClassType)) {
const lookupResults = lookUpClassMember(
targetClassType,
methodType.details.name,
ClassMemberLookupFlags.SkipOriginalClass |
ClassMemberLookupFlags.SkipInstanceVariables |
ClassMemberLookupFlags.SkipObjectBaseClass
);

if (lookupResults && isInstantiableClass(lookupResults.classType)) {
const baseType = lookupResults.classType;

// Note that we've called this base class.
baseClassesToCall = baseClassesToCall.filter(
(cls) => !ClassType.isSameGenericClass(cls, baseType)
);
}
}
foundCallOfMember = true;
} else {
// Is it an X.<method> direct call?
const baseType = this._evaluator.getType(memberBaseExpr);
if (baseType && isInstantiableClass(baseType)) {
// Note that we've called this base class.
baseClassesToCall = baseClassesToCall.filter(
(cls) => !ClassType.isSameGenericClass(cls, baseType)
);
foundCallOfMember = true;
}
}
}
}
});
callNodeWalker.walk(node.suite);

// If there are base classes that haven't yet been called, report it as an error.
baseClassesToCall.forEach((baseClass) => {
// If we didn't find a call to at least one base class, report the problem.
if (!foundCallOfMember) {
this._evaluator.addDiagnostic(
this._fileInfo.diagnosticRuleSet.reportMissingSuperCall,
DiagnosticRule.reportMissingSuperCall,
Localizer.Diagnostic.missingSuperCall().format({
methodName: methodType.details.name,
classType: this._evaluator.printType(convertToInstance(baseClass)),
}),
node.name
);
});
}
}

// Validates that the annotated type of a "self" or "cls" parameter is
Expand Down
4 changes: 1 addition & 3 deletions packages/pyright-internal/src/localization/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -510,9 +510,7 @@ export namespace Localizer {
export const methodReturnsNonObject = () =>
new ParameterizedString<{ name: string }>(getRawString('Diagnostic.methodReturnsNonObject'));
export const missingSuperCall = () =>
new ParameterizedString<{ methodName: string; classType: string }>(
getRawString('Diagnostic.missingSuperCall')
);
new ParameterizedString<{ methodName: string }>(getRawString('Diagnostic.missingSuperCall'));
export const moduleAsType = () => getRawString('Diagnostic.moduleAsType');
export const moduleNotCallable = () => getRawString('Diagnostic.moduleNotCallable');
export const moduleUnknownMember = () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@
"methodOrdering": "Cannot create consistent method ordering",
"methodOverridden": "\"{name}\" overrides method of same name in class \"{className}\" with incompatible type \"{type}\"",
"methodReturnsNonObject": "\"{name}\" method does not return an object",
"missingSuperCall": "Method \"{methodName}\" does not call the method of the same name in parent class \"{classType}\"",
"missingSuperCall": "Method \"{methodName}\" does not call the method of the same name in parent class",
"moduleAsType": "Module cannot be used as a type",
"moduleNotCallable": "Module is not callable",
"moduleUnknownMember": "\"{name}\" is not a known member of module",
Expand Down
32 changes: 22 additions & 10 deletions packages/pyright-internal/src/tests/samples/missingSuper1.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
# This sample tests the reportMissingSuperCall diagnostic check.

from typing import final

class ParentA:
def __init__(self):
pass

def __init_subclass__(cls) -> None:
pass
class ParentA:
pass


class ParentB:
# This should generate an error because it's missing a super().__init__ call.
def __init__(self):
pass

Expand All @@ -22,25 +21,32 @@ class ParentC:
pass


@final
class ParentD:
def __init__(self):
pass

def __init_subclass__(cls) -> None:
pass


class ChildA(ParentA, ParentB):
# This should generate two errors.
# This should generate an error.
def __init__(self):
pass

# This should generate one error.
# This should generate an error.
def __init_subclass__(cls) -> None:
pass


class ChildB(ParentA, ParentB):
# This should generate one error.
def __init__(self):
super().__init__()


class ChildC1(ParentA, ParentB):
def __init__(self):
super().__init__()
ParentB.__init__(self)


Expand All @@ -52,10 +58,16 @@ def __init__(self):

class ChildCPrime(ParentA, ParentBPrime, ParentC):
def __init__(self):
super().__init__()
super(ParentBPrime).__init__()


class ChildD(ParentC):
# This should generate an error.
def __init__(self):
pass


@final
class ChildE(ParentC):
def __init__(self):
pass

0 comments on commit 846cee3

Please sign in to comment.