Skip to content

Commit b62386a

Browse files
KotlinIslandKotlinIsland
committed
flag abstract base classes with no abstract methods
1 parent 264b247 commit b62386a

14 files changed

Lines changed: 155 additions & 38 deletions

File tree

‎packages/pyright-internal/src/analyzer/checker.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5281,11 +5281,8 @@ export class Checker extends ParseTreeWalker {
52815281
} else {
52825282
const baseClasses = classType.shared.baseClasses.filter(isClass);
52835283
if (
5284-
(classType.shared.declaredMetaclass?.category !== TypeCategory.Class ||
5285-
!ClassType.isBuiltIn(classType.shared.declaredMetaclass, 'ABCMeta')) &&
5286-
!baseClasses.some(
5287-
(baseClass) => baseClass.shared.fullName === 'abc.ABC' || ClassType.isBuiltIn(baseClass, 'Protocol')
5288-
)
5284+
!ClassType.isDirectSubtypeOfAbstractClass(classType) &&
5285+
!baseClasses.some((baseClass) => ClassType.isBuiltIn(baseClass, 'Protocol'))
52895286
) {
52905287
const errorMessage = classType.shared.mro.some(
52915288
(baseClass) => isClass(baseClass) && ClassType.isBuiltIn(baseClass, 'Protocol')

‎packages/pyright-internal/src/analyzer/typeEvaluator.ts‎

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -10787,42 +10787,54 @@ export function createTypeEvaluator(
1078710787
if (ClassType.supportsAbstractMethods(expandedCallType)) {
1078810788
const abstractSymbols = getAbstractSymbols(expandedCallType);
1078910789

10790-
if (
10791-
abstractSymbols.length > 0 &&
10792-
!expandedCallType.priv.includeSubclasses &&
10793-
!isTypeVar(unexpandedCallType)
10794-
) {
10795-
// If the class is abstract, it can't be instantiated.
10796-
const diagAddendum = new DiagnosticAddendum();
10797-
const errorsToDisplay = 2;
10790+
// check for abstract usages.
10791+
// only report an error if it's an exact type (ie. we know it's definitely not a subclass/impl)
10792+
if (!expandedCallType.priv.includeSubclasses && !isTypeVar(unexpandedCallType)) {
10793+
// Handle abstract classes with abstract methods (reportAbstractUsage)
10794+
if (abstractSymbols.length > 0) {
10795+
// If the class is abstract, it can't be instantiated.
10796+
const diagAddendum = new DiagnosticAddendum();
10797+
const errorsToDisplay = 2;
1079810798

10799-
abstractSymbols.forEach((abstractMethod, index) => {
10800-
if (index === errorsToDisplay) {
10801-
diagAddendum.addMessage(
10802-
LocAddendum.memberIsAbstractMore().format({
10803-
count: abstractSymbols.length - errorsToDisplay,
10804-
})
10805-
);
10806-
} else if (index < errorsToDisplay) {
10807-
if (isInstantiableClass(abstractMethod.classType)) {
10808-
const className = abstractMethod.classType.shared.name;
10799+
abstractSymbols.forEach((abstractMethod, index) => {
10800+
if (index === errorsToDisplay) {
1080910801
diagAddendum.addMessage(
10810-
LocAddendum.memberIsAbstract().format({
10811-
type: className,
10812-
name: abstractMethod.symbolName,
10802+
LocAddendum.memberIsAbstractMore().format({
10803+
count: abstractSymbols.length - errorsToDisplay,
1081310804
})
1081410805
);
10806+
} else if (index < errorsToDisplay) {
10807+
if (isInstantiableClass(abstractMethod.classType)) {
10808+
const className = abstractMethod.classType.shared.name;
10809+
diagAddendum.addMessage(
10810+
LocAddendum.memberIsAbstract().format({
10811+
type: className,
10812+
name: abstractMethod.symbolName,
10813+
})
10814+
);
10815+
}
1081510816
}
10816-
}
10817-
});
10817+
});
1081810818

10819-
addDiagnostic(
10820-
DiagnosticRule.reportAbstractUsage,
10821-
LocMessage.instantiateAbstract().format({
10822-
type: expandedCallType.shared.name,
10823-
}) + diagAddendum.getString(),
10824-
errorNode
10825-
);
10819+
addDiagnostic(
10820+
DiagnosticRule.reportAbstractUsage,
10821+
LocMessage.instantiateAbstract().format({
10822+
type: expandedCallType.shared.name,
10823+
}) + diagAddendum.getString(),
10824+
errorNode
10825+
);
10826+
} else if (ClassType.isDirectSubtypeOfAbstractClass(expandedCallType)) {
10827+
// Handle abstract classes with no abstract methods (reportEmptyAbstractClass)
10828+
const diag = new DiagnosticAddendum();
10829+
diag.addMessage(LocAddendum.classIsExplicitlyAbstract());
10830+
addDiagnostic(
10831+
DiagnosticRule.reportExplicitAbstractUsage,
10832+
LocMessage.instantiateAbstract().format({
10833+
type: expandedCallType.shared.name,
10834+
}) + diag.getString(),
10835+
errorNode
10836+
);
10837+
}
1082610838
}
1082710839
}
1082810840

‎packages/pyright-internal/src/analyzer/types.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1175,6 +1175,18 @@ export namespace ClassType {
11751175
return !!(classType.shared.flags & ClassTypeFlags.SupportsAbstractMethods);
11761176
}
11771177

1178+
export function isDirectSubtypeOfAbstractClass(classType: ClassType): boolean {
1179+
const derivesDirectlyFromABC = classType.shared.baseClasses.some(
1180+
(baseClass) => isClass(baseClass) && baseClass.shared.fullName === 'abc.ABC'
1181+
);
1182+
const hasABCMetaMetaclass =
1183+
classType.shared.declaredMetaclass !== undefined &&
1184+
isClass(classType.shared.declaredMetaclass) &&
1185+
classType.shared.declaredMetaclass.shared.fullName === 'abc.ABCMeta';
1186+
1187+
return derivesDirectlyFromABC || hasABCMetaMetaclass;
1188+
}
1189+
11781190
export function isDataClass(classType: ClassType) {
11791191
return !!classType.shared.dataClassBehaviors;
11801192
}

‎packages/pyright-internal/src/common/configOptions.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,9 @@ export interface DiagnosticRuleSet {
189189
// Report use of abstract method or variable?
190190
reportAbstractUsage: DiagnosticLevel;
191191

192+
// Report instantiation of abstract class with no abstract methods?
193+
reportExplicitAbstractUsage: DiagnosticLevel;
194+
192195
// Report argument type incompatibilities?
193196
reportArgumentType: DiagnosticLevel;
194197

@@ -481,6 +484,7 @@ export function getDiagLevelDiagnosticRules() {
481484
DiagnosticRule.reportDuplicateImport,
482485
DiagnosticRule.reportWildcardImportFromLibrary,
483486
DiagnosticRule.reportAbstractUsage,
487+
DiagnosticRule.reportExplicitAbstractUsage,
484488
DiagnosticRule.reportArgumentType,
485489
DiagnosticRule.reportAssertTypeFailure,
486490
DiagnosticRule.reportAssignmentType,
@@ -617,6 +621,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet {
617621
reportDuplicateImport: 'none',
618622
reportWildcardImportFromLibrary: 'none',
619623
reportAbstractUsage: 'none',
624+
reportExplicitAbstractUsage: 'none',
620625
reportArgumentType: 'none',
621626
reportAssertTypeFailure: 'none',
622627
reportAssignmentType: 'none',
@@ -737,6 +742,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet {
737742
reportDuplicateImport: 'none',
738743
reportWildcardImportFromLibrary: 'warning',
739744
reportAbstractUsage: 'error',
745+
reportExplicitAbstractUsage: 'error',
740746
reportArgumentType: 'error',
741747
reportAssertTypeFailure: 'error',
742748
reportAssignmentType: 'error',
@@ -857,6 +863,7 @@ export function getStandardDiagnosticRuleSet(): DiagnosticRuleSet {
857863
reportDuplicateImport: 'none',
858864
reportWildcardImportFromLibrary: 'warning',
859865
reportAbstractUsage: 'error',
866+
reportExplicitAbstractUsage: 'error',
860867
reportArgumentType: 'error',
861868
reportAssertTypeFailure: 'error',
862869
reportAssignmentType: 'error',
@@ -976,6 +983,7 @@ export const getRecommendedDiagnosticRuleSet = (): DiagnosticRuleSet => ({
976983
reportDuplicateImport: 'warning',
977984
reportWildcardImportFromLibrary: 'warning',
978985
reportAbstractUsage: 'error',
986+
reportExplicitAbstractUsage: 'error',
979987
reportArgumentType: 'error',
980988
reportAssertTypeFailure: 'error',
981989
reportAssignmentType: 'error',
@@ -1092,6 +1100,7 @@ export const getAllDiagnosticRuleSet = (): DiagnosticRuleSet => ({
10921100
reportDuplicateImport: 'error',
10931101
reportWildcardImportFromLibrary: 'error',
10941102
reportAbstractUsage: 'error',
1103+
reportExplicitAbstractUsage: 'error',
10951104
reportArgumentType: 'error',
10961105
reportAssertTypeFailure: 'error',
10971106
reportAssignmentType: 'error',
@@ -1209,6 +1218,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet {
12091218
reportDuplicateImport: 'error',
12101219
reportWildcardImportFromLibrary: 'error',
12111220
reportAbstractUsage: 'error',
1221+
reportExplicitAbstractUsage: 'error',
12121222
reportArgumentType: 'error',
12131223
reportAssertTypeFailure: 'error',
12141224
reportAssignmentType: 'error',

‎packages/pyright-internal/src/common/diagnosticRules.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export enum DiagnosticRule {
3838
reportDuplicateImport = 'reportDuplicateImport',
3939
reportWildcardImportFromLibrary = 'reportWildcardImportFromLibrary',
4040
reportAbstractUsage = 'reportAbstractUsage',
41+
reportExplicitAbstractUsage = 'reportExplicitAbstractUsage',
4142
reportArgumentType = 'reportArgumentType',
4243
reportAssertTypeFailure = 'reportAssertTypeFailure',
4344
reportAssignmentType = 'reportAssignmentType',

‎packages/pyright-internal/src/localization/localize.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1393,6 +1393,7 @@ export namespace Localizer {
13931393
);
13941394
export const memberIsAbstractMore = () =>
13951395
new ParameterizedString<{ count: number }>(getRawString('DiagnosticAddendum.memberIsAbstractMore'));
1396+
export const classIsExplicitlyAbstract = () => getRawString('DiagnosticAddendum.classIsExplicitlyAbstract');
13961397
export const memberIsClassVarInProtocol = () =>
13971398
new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.memberIsClassVarInProtocol'));
13981399
export const memberIsInitVar = () =>

‎packages/pyright-internal/src/localization/package.nls.en-us.json‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1934,6 +1934,9 @@
19341934
"message": "and {count} more...",
19351935
"comment": "{StrEnds='...'}"
19361936
},
1937+
"classIsExplicitlyAbstract": {
1938+
"message": "class has no abstract members, but is explicitly denoted with \"ABC\" or \"ABCMeta\""
1939+
},
19371940
"memberIsClassVarInProtocol": {
19381941
"message": "\"{name}\" is defined as a ClassVar in protocol",
19391942
"comment": "{Locked='ClassVar'}"

‎packages/pyright-internal/src/tests/checker.test.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,22 @@ test('AbstractClass11', () => {
128128
TestUtils.validateResults(analysisResults, 2);
129129
});
130130

131+
test('AbstractClass12', () => {
132+
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['abstractClass12.py']);
133+
134+
TestUtils.validateResultsButBased(analysisResults, {
135+
// one error to validate the message, the rest use `pyright: ignore`
136+
errors: [
137+
{
138+
line: 11,
139+
message:
140+
'Cannot instantiate abstract class "ExplicitlyAbstract"\n  class has no abstract members, but is explicitly denoted with "ABC" or "ABCMeta"',
141+
code: DiagnosticRule.reportExplicitAbstractUsage,
142+
},
143+
],
144+
});
145+
});
146+
131147
test('Constants1', () => {
132148
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['constants1.py']);
133149

‎packages/pyright-internal/src/tests/samples/abstractClass12.py‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# this sample tests the type analyzer's ability to flag attempts
2+
# to instantiate abstract base classes that have no abstract methods
3+
4+
from abc import ABC, ABCMeta
5+
6+
7+
class ExplicitlyAbstract(ABC):
8+
"""an abstract class with no abstract methods"""
9+
10+
# this should generate an error because `AbstractFoo`
11+
# is an abstract class even though it has no abstract methods
12+
a = ExplicitlyAbstract()
13+
14+
class NoLongerExplicitlyAbstract(ExplicitlyAbstract):
15+
"""inherits from an explicitly abstract class"""
16+
17+
18+
# this should not generate an error because NoLongerExplicitlyAbstract
19+
# doesn't directly inherit from ABC
20+
f = NoLongerExplicitlyAbstract()
21+
22+
23+
class NotAbstract:
24+
"""a regular class that doesn't derive from ABC"""
25+
26+
# This should not generate an error because NotAbstract is not abstract
27+
e = NotAbstract()
28+
29+
class AbstractWithMetaclass(metaclass=ABCMeta):
30+
"""abstract class using ABCMeta metaclass with no abstract methods"""
31+
32+
class ConcreteWithMetaclass(AbstractWithMetaclass):
33+
"""concrete subclass of AbstractWithMetaclass"""
34+
35+
36+
# this should generate an error because AbstractWithMetaclass uses ABCMeta
37+
i = AbstractWithMetaclass() # pyright: ignore[reportExplicitAbstractUsage]
38+
39+
# this should not generate an error
40+
j = ConcreteWithMetaclass()

‎packages/pyright-internal/src/tests/samples/async1.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ async def b():
99
yield i
1010

1111

12-
cm = AsyncExitStack()
12+
cm = AsyncExitStack() # pyright: ignore[reportExplicitAbstractUsage] # typeshed moment
1313

1414

1515
def func1():

0 commit comments

Comments
 (0)