From cec98ab8b45c1636d36f3a47bae59f0ab4851002 Mon Sep 17 00:00:00 2001 From: KotlinIsland <65446343+kotlinisland@users.noreply.github.com> Date: Thu, 5 Mar 2026 12:46:18 +1000 Subject: [PATCH] flag abstract base classes with no abstract methods --- .../new-diagnostic-rules.md | 34 +++++++++ docs/configuration/config-files.md | 3 +- .../pyright-internal/src/analyzer/checker.ts | 7 +- .../src/analyzer/typeEvaluator.ts | 70 +++++++++++-------- .../pyright-internal/src/analyzer/types.ts | 12 ++++ .../src/common/configOptions.ts | 10 +++ .../src/common/diagnosticRules.ts | 1 + .../src/localization/localize.ts | 1 + .../src/localization/package.nls.en-us.json | 3 + .../src/tests/checker.test.ts | 16 +++++ .../src/tests/samples/abstractClass12.py | 40 +++++++++++ .../src/tests/samples/async1.py | 2 +- .../src/tests/samples/property2.py | 2 +- .../src/tests/samples/pseudoGeneric3.py | 2 +- packages/vscode-pyright/package.json | 17 +++++ .../schemas/pyrightconfig.schema.json | 8 +++ 16 files changed, 189 insertions(+), 39 deletions(-) create mode 100644 packages/pyright-internal/src/tests/samples/abstractClass12.py diff --git a/docs/benefits-over-pyright/new-diagnostic-rules.md b/docs/benefits-over-pyright/new-diagnostic-rules.md index c6abe964eb..e23ae1562c 100644 --- a/docs/benefits-over-pyright/new-diagnostic-rules.md +++ b/docs/benefits-over-pyright/new-diagnostic-rules.md @@ -195,6 +195,40 @@ class FooImpl(AbstractFoo, ABC): print("hi") ``` +## `reportEmptyAbstractUsage` + +pyright only reports an error when you instantiate an abstract class that has unimplemented abstract methods. but a class that explicitly extends `ABC` (or uses `ABCMeta`) with no abstract methods can also be instantiated, and pyright has no issue with that: + +```py +from abc import ABC + + +class Foo(ABC): + """abstract class with no abstract methods""" + + +foo = Foo() # no error +``` + +but the author of the class likely intended this class not to be used directly, and instead subtyped. so if a class extends `ABC` but defines no abstract methods, instantiating it is likely unintentional. + +the `reportEmptyAbstractUsage` rule flags such instantiations. note that it only applies to classes that _directly_ extend `ABC` (or use `ABCMeta`), not to their subclasses, since subclasses may be intentionally concrete: + +```py +from abc import ABC + + +class AbstractFoo(ABC): + """abstract class with no abstract methods""" + + +class ConcreteFoo(AbstractFoo): ... + + +foo = AbstractFoo() # error: reportEmptyAbstractUsage +bar = ConcreteFoo() # no error +``` + ## `reportIncompatibleUnannotatedOverride` pyright's `reportIncompatibleVariableOverride` rule checks for class attribute overrides with an incompatible type: diff --git a/docs/configuration/config-files.md b/docs/configuration/config-files.md index bcb0a11763..4a57a42550 100644 --- a/docs/configuration/config-files.md +++ b/docs/configuration/config-files.md @@ -304,6 +304,8 @@ The following settings allow more fine grained control over the **typeCheckingMo - **reportImplicitAbstractClass** [boolean or string, optional]: Diagnostics for classes that extend abstract classes without also explicitly declaring themselves as abstract or implementing all of the required abstract methods. [more info](../benefits-over-pyright/new-diagnostic-rules.md#reportimplicitabstractclass) +- **reportEmptyAbstractUsage** [boolean or string, optional]: Diagnostics for classes that have no abstract methods, but explicitly extend `ABC` or `ABCMeta`. [more info](../benefits-over-pyright/new-diagnostic-rules.md#reportemptyabstractusage) + - **reportIncompatibleUnannotatedOverride** [boolean or string, optional]: Generate or suppress diagnostics for class variable declarations that override a symbol of the same name in a base class with a type that is incompatible with the base class symbol type, when the base class' symbol does not have a type annotation. [more info](../benefits-over-pyright/new-diagnostic-rules.md#reportincompatibleunannotatedoverride) - **reportUnannotatedClassAttribute** [boolean or string, optional]: Generate or suppress diagnostics for class attribute declarations that do not have a type annotation. These are unsafe unless `reportIncompatibleUnannotatedOverride` is enabled. [more info](../benefits-over-pyright/new-diagnostic-rules.md#reportunannotatedclassattribute) @@ -312,7 +314,6 @@ The following settings allow more fine grained control over the **typeCheckingMo - **reportSelfClsDefault** [boolean or string, optional]: Generate or suppress diagnostics for a class or instance method having a default value for the first parameter. - ## Execution Environment Options Pyright allows multiple “execution environments” to be defined for different portions of your source tree. For example, a subtree may be designed to run with different import search paths or a different version of the python interpreter than the rest of the source base. diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts index 39cc9b90ea..f429c6c664 100644 --- a/packages/pyright-internal/src/analyzer/checker.ts +++ b/packages/pyright-internal/src/analyzer/checker.ts @@ -5281,11 +5281,8 @@ export class Checker extends ParseTreeWalker { } else { const baseClasses = classType.shared.baseClasses.filter(isClass); if ( - (classType.shared.declaredMetaclass?.category !== TypeCategory.Class || - !ClassType.isBuiltIn(classType.shared.declaredMetaclass, 'ABCMeta')) && - !baseClasses.some( - (baseClass) => baseClass.shared.fullName === 'abc.ABC' || ClassType.isBuiltIn(baseClass, 'Protocol') - ) + !ClassType.isDirectSubtypeOfAbstractClass(classType) && + !baseClasses.some((baseClass) => ClassType.isBuiltIn(baseClass, 'Protocol')) ) { const errorMessage = classType.shared.mro.some( (baseClass) => isClass(baseClass) && ClassType.isBuiltIn(baseClass, 'Protocol') diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts index 09e782fe94..3677d74f92 100644 --- a/packages/pyright-internal/src/analyzer/typeEvaluator.ts +++ b/packages/pyright-internal/src/analyzer/typeEvaluator.ts @@ -10787,42 +10787,52 @@ export function createTypeEvaluator( if (ClassType.supportsAbstractMethods(expandedCallType)) { const abstractSymbols = getAbstractSymbols(expandedCallType); - if ( - abstractSymbols.length > 0 && - !expandedCallType.priv.includeSubclasses && - !isTypeVar(unexpandedCallType) - ) { - // If the class is abstract, it can't be instantiated. - const diagAddendum = new DiagnosticAddendum(); - const errorsToDisplay = 2; + if (!expandedCallType.priv.includeSubclasses && !isTypeVar(unexpandedCallType)) { + // Handle abstract classes with abstract methods + if (abstractSymbols.length > 0) { + // If the class is abstract, it can't be instantiated. + const diagAddendum = new DiagnosticAddendum(); + const errorsToDisplay = 2; - abstractSymbols.forEach((abstractMethod, index) => { - if (index === errorsToDisplay) { - diagAddendum.addMessage( - LocAddendum.memberIsAbstractMore().format({ - count: abstractSymbols.length - errorsToDisplay, - }) - ); - } else if (index < errorsToDisplay) { - if (isInstantiableClass(abstractMethod.classType)) { - const className = abstractMethod.classType.shared.name; + abstractSymbols.forEach((abstractMethod, index) => { + if (index === errorsToDisplay) { diagAddendum.addMessage( - LocAddendum.memberIsAbstract().format({ - type: className, - name: abstractMethod.symbolName, + LocAddendum.memberIsAbstractMore().format({ + count: abstractSymbols.length - errorsToDisplay, }) ); + } else if (index < errorsToDisplay) { + if (isInstantiableClass(abstractMethod.classType)) { + const className = abstractMethod.classType.shared.name; + diagAddendum.addMessage( + LocAddendum.memberIsAbstract().format({ + type: className, + name: abstractMethod.symbolName, + }) + ); + } } - } - }); + }); - addDiagnostic( - DiagnosticRule.reportAbstractUsage, - LocMessage.instantiateAbstract().format({ - type: expandedCallType.shared.name, - }) + diagAddendum.getString(), - errorNode - ); + addDiagnostic( + DiagnosticRule.reportAbstractUsage, + LocMessage.instantiateAbstract().format({ + type: expandedCallType.shared.name, + }) + diagAddendum.getString(), + errorNode + ); + } else if (ClassType.isDirectSubtypeOfAbstractClass(expandedCallType)) { + // Handle abstract classes with no abstract methods + const diag = new DiagnosticAddendum(); + diag.addMessage(LocAddendum.classIsExplicitlyAbstract()); + addDiagnostic( + DiagnosticRule.reportEmptyAbstractUsage, + LocMessage.instantiateAbstract().format({ + type: expandedCallType.shared.name, + }) + diag.getString(), + errorNode + ); + } } } diff --git a/packages/pyright-internal/src/analyzer/types.ts b/packages/pyright-internal/src/analyzer/types.ts index b1695db2b5..6a4f087203 100644 --- a/packages/pyright-internal/src/analyzer/types.ts +++ b/packages/pyright-internal/src/analyzer/types.ts @@ -1175,6 +1175,18 @@ export namespace ClassType { return !!(classType.shared.flags & ClassTypeFlags.SupportsAbstractMethods); } + export function isDirectSubtypeOfAbstractClass(classType: ClassType): boolean { + const derivesDirectlyFromABC = classType.shared.baseClasses.some( + (baseClass) => isClass(baseClass) && baseClass.shared.fullName === 'abc.ABC' + ); + const hasABCMetaMetaclass = + classType.shared.declaredMetaclass !== undefined && + isClass(classType.shared.declaredMetaclass) && + classType.shared.declaredMetaclass.shared.fullName === 'abc.ABCMeta'; + + return derivesDirectlyFromABC || hasABCMetaMetaclass; + } + export function isDataClass(classType: ClassType) { return !!classType.shared.dataClassBehaviors; } diff --git a/packages/pyright-internal/src/common/configOptions.ts b/packages/pyright-internal/src/common/configOptions.ts index 6dbf559fa5..56b36b0319 100644 --- a/packages/pyright-internal/src/common/configOptions.ts +++ b/packages/pyright-internal/src/common/configOptions.ts @@ -189,6 +189,9 @@ export interface DiagnosticRuleSet { // Report use of abstract method or variable? reportAbstractUsage: DiagnosticLevel; + // Report instantiation of abstract class with no abstract methods? + reportEmptyAbstractUsage: DiagnosticLevel; + // Report argument type incompatibilities? reportArgumentType: DiagnosticLevel; @@ -481,6 +484,7 @@ export function getDiagLevelDiagnosticRules() { DiagnosticRule.reportDuplicateImport, DiagnosticRule.reportWildcardImportFromLibrary, DiagnosticRule.reportAbstractUsage, + DiagnosticRule.reportEmptyAbstractUsage, DiagnosticRule.reportArgumentType, DiagnosticRule.reportAssertTypeFailure, DiagnosticRule.reportAssignmentType, @@ -617,6 +621,7 @@ export function getOffDiagnosticRuleSet(): DiagnosticRuleSet { reportDuplicateImport: 'none', reportWildcardImportFromLibrary: 'none', reportAbstractUsage: 'none', + reportEmptyAbstractUsage: 'none', reportArgumentType: 'none', reportAssertTypeFailure: 'none', reportAssignmentType: 'none', @@ -737,6 +742,7 @@ export function getBasicDiagnosticRuleSet(): DiagnosticRuleSet { reportDuplicateImport: 'none', reportWildcardImportFromLibrary: 'warning', reportAbstractUsage: 'error', + reportEmptyAbstractUsage: 'error', reportArgumentType: 'error', reportAssertTypeFailure: 'error', reportAssignmentType: 'error', @@ -857,6 +863,7 @@ export function getStandardDiagnosticRuleSet(): DiagnosticRuleSet { reportDuplicateImport: 'none', reportWildcardImportFromLibrary: 'warning', reportAbstractUsage: 'error', + reportEmptyAbstractUsage: 'error', reportArgumentType: 'error', reportAssertTypeFailure: 'error', reportAssignmentType: 'error', @@ -976,6 +983,7 @@ export const getRecommendedDiagnosticRuleSet = (): DiagnosticRuleSet => ({ reportDuplicateImport: 'warning', reportWildcardImportFromLibrary: 'warning', reportAbstractUsage: 'error', + reportEmptyAbstractUsage: 'error', reportArgumentType: 'error', reportAssertTypeFailure: 'error', reportAssignmentType: 'error', @@ -1092,6 +1100,7 @@ export const getAllDiagnosticRuleSet = (): DiagnosticRuleSet => ({ reportDuplicateImport: 'error', reportWildcardImportFromLibrary: 'error', reportAbstractUsage: 'error', + reportEmptyAbstractUsage: 'error', reportArgumentType: 'error', reportAssertTypeFailure: 'error', reportAssignmentType: 'error', @@ -1209,6 +1218,7 @@ export function getStrictDiagnosticRuleSet(): DiagnosticRuleSet { reportDuplicateImport: 'error', reportWildcardImportFromLibrary: 'error', reportAbstractUsage: 'error', + reportEmptyAbstractUsage: 'error', reportArgumentType: 'error', reportAssertTypeFailure: 'error', reportAssignmentType: 'error', diff --git a/packages/pyright-internal/src/common/diagnosticRules.ts b/packages/pyright-internal/src/common/diagnosticRules.ts index 7738588855..102536777e 100644 --- a/packages/pyright-internal/src/common/diagnosticRules.ts +++ b/packages/pyright-internal/src/common/diagnosticRules.ts @@ -38,6 +38,7 @@ export enum DiagnosticRule { reportDuplicateImport = 'reportDuplicateImport', reportWildcardImportFromLibrary = 'reportWildcardImportFromLibrary', reportAbstractUsage = 'reportAbstractUsage', + reportEmptyAbstractUsage = 'reportEmptyAbstractUsage', reportArgumentType = 'reportArgumentType', reportAssertTypeFailure = 'reportAssertTypeFailure', reportAssignmentType = 'reportAssignmentType', diff --git a/packages/pyright-internal/src/localization/localize.ts b/packages/pyright-internal/src/localization/localize.ts index a439f95ce4..e30e7d691b 100644 --- a/packages/pyright-internal/src/localization/localize.ts +++ b/packages/pyright-internal/src/localization/localize.ts @@ -1393,6 +1393,7 @@ export namespace Localizer { ); export const memberIsAbstractMore = () => new ParameterizedString<{ count: number }>(getRawString('DiagnosticAddendum.memberIsAbstractMore')); + export const classIsExplicitlyAbstract = () => getRawString('DiagnosticAddendum.classIsExplicitlyAbstract'); export const memberIsClassVarInProtocol = () => new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.memberIsClassVarInProtocol')); export const memberIsInitVar = () => diff --git a/packages/pyright-internal/src/localization/package.nls.en-us.json b/packages/pyright-internal/src/localization/package.nls.en-us.json index 7a5a4ac91d..c99266ce5b 100644 --- a/packages/pyright-internal/src/localization/package.nls.en-us.json +++ b/packages/pyright-internal/src/localization/package.nls.en-us.json @@ -1934,6 +1934,9 @@ "message": "and {count} more...", "comment": "{StrEnds='...'}" }, + "classIsExplicitlyAbstract": { + "message": "class has no abstract members, but is explicitly denoted with \"ABC\" or \"ABCMeta\"" + }, "memberIsClassVarInProtocol": { "message": "\"{name}\" is defined as a ClassVar in protocol", "comment": "{Locked='ClassVar'}" diff --git a/packages/pyright-internal/src/tests/checker.test.ts b/packages/pyright-internal/src/tests/checker.test.ts index eca1df3f74..fc509ff105 100644 --- a/packages/pyright-internal/src/tests/checker.test.ts +++ b/packages/pyright-internal/src/tests/checker.test.ts @@ -128,6 +128,22 @@ test('AbstractClass11', () => { TestUtils.validateResults(analysisResults, 2); }); +test('AbstractClass12', () => { + const analysisResults = TestUtils.typeAnalyzeSampleFiles(['abstractClass12.py']); + + TestUtils.validateResultsButBased(analysisResults, { + // one error to validate the message, the rest use `pyright: ignore` + errors: [ + { + line: 11, + message: + 'Cannot instantiate abstract class "ExplicitlyAbstract"\n  class has no abstract members, but is explicitly denoted with "ABC" or "ABCMeta"', + code: DiagnosticRule.reportEmptyAbstractUsage, + }, + ], + }); +}); + test('Constants1', () => { const analysisResults = TestUtils.typeAnalyzeSampleFiles(['constants1.py']); diff --git a/packages/pyright-internal/src/tests/samples/abstractClass12.py b/packages/pyright-internal/src/tests/samples/abstractClass12.py new file mode 100644 index 0000000000..03722bf68e --- /dev/null +++ b/packages/pyright-internal/src/tests/samples/abstractClass12.py @@ -0,0 +1,40 @@ +# this sample tests the type analyzer's ability to flag attempts +# to instantiate abstract base classes that have no abstract methods + +from abc import ABC, ABCMeta + + +class ExplicitlyAbstract(ABC): + """an abstract class with no abstract methods""" + +# this should generate an error because it +# is an abstract class even though it has no abstract methods +a = ExplicitlyAbstract() + +class NoLongerExplicitlyAbstract(ExplicitlyAbstract): + """inherits from an explicitly abstract class""" + + +# this should not generate an error because NoLongerExplicitlyAbstract +# doesn't directly inherit from ABC +f = NoLongerExplicitlyAbstract() + + +class NotAbstract: + """a regular class that doesn't derive from ABC""" + +# This should not generate an error because NotAbstract is not abstract +e = NotAbstract() + +class AbstractWithMetaclass(metaclass=ABCMeta): + """abstract class using ABCMeta metaclass with no abstract methods""" + +class ConcreteWithMetaclass(AbstractWithMetaclass): + """concrete subclass of AbstractWithMetaclass""" + + +# this should generate an error because AbstractWithMetaclass uses ABCMeta +i = AbstractWithMetaclass() # pyright: ignore[reportEmptyAbstractUsage] + +# this should not generate an error +j = ConcreteWithMetaclass() diff --git a/packages/pyright-internal/src/tests/samples/async1.py b/packages/pyright-internal/src/tests/samples/async1.py index 47abf92d1a..a6be2710c3 100644 --- a/packages/pyright-internal/src/tests/samples/async1.py +++ b/packages/pyright-internal/src/tests/samples/async1.py @@ -9,7 +9,7 @@ async def b(): yield i -cm = AsyncExitStack() +cm = AsyncExitStack() # pyright: ignore[reportEmptyAbstractUsage] # typeshed moment def func1(): diff --git a/packages/pyright-internal/src/tests/samples/property2.py b/packages/pyright-internal/src/tests/samples/property2.py index 3b87aec4bd..158c6ce9f8 100644 --- a/packages/pyright-internal/src/tests/samples/property2.py +++ b/packages/pyright-internal/src/tests/samples/property2.py @@ -21,7 +21,7 @@ def y(self) -> float: raise NotImplementedError -a = Foo() +a = Foo() # pyright: ignore[reportEmptyAbstractUsage] requires_int(a.x) a.x = 3 diff --git a/packages/pyright-internal/src/tests/samples/pseudoGeneric3.py b/packages/pyright-internal/src/tests/samples/pseudoGeneric3.py index 4cf33269a6..ec42cf9154 100644 --- a/packages/pyright-internal/src/tests/samples/pseudoGeneric3.py +++ b/packages/pyright-internal/src/tests/samples/pseudoGeneric3.py @@ -25,6 +25,6 @@ def __getattr__(self, attr): return self.__getattribute__(attr) -b1 = ClassB("test") +b1 = ClassB("test") # pyright: ignore[reportEmptyAbstractUsage] reveal_type(b1.value, expected_text="Unknown | Any | None") del b1.cache diff --git a/packages/vscode-pyright/package.json b/packages/vscode-pyright/package.json index 4627bc1717..0700e06735 100644 --- a/packages/vscode-pyright/package.json +++ b/packages/vscode-pyright/package.json @@ -490,6 +490,23 @@ false ] }, + "reportEmptyAbstractUsage": { + "type": [ + "string", + "boolean" + ], + "description": "Diagnostics for an attempt to instantiate an abstract class that has no abstract members.", + "default": "error", + "enum": [ + "none", + "hint", + "information", + "warning", + "error", + true, + false + ] + }, "reportArgumentType": { "type": [ "string", diff --git a/packages/vscode-pyright/schemas/pyrightconfig.schema.json b/packages/vscode-pyright/schemas/pyrightconfig.schema.json index 23348336d9..1207bf8b50 100644 --- a/packages/vscode-pyright/schemas/pyrightconfig.schema.json +++ b/packages/vscode-pyright/schemas/pyrightconfig.schema.json @@ -185,6 +185,11 @@ "title": "Controls reporting of attempted instantiation of abstract class", "default": "error" }, + "reportEmptyAbstractUsage": { + "$ref": "#/definitions/diagnostic", + "title": "Controls reporting of attempted instantiation of empty abstract class", + "default": "error" + }, "reportArgumentType": { "$ref": "#/definitions/diagnostic", "title": "Controls reporting of incompatible argument type", @@ -744,6 +749,9 @@ "reportAbstractUsage": { "$ref": "#/definitions/reportAbstractUsage" }, + "reportEmptyAbstractUsage": { + "$ref": "#/definitions/reportEmptyAbstractUsage" + }, "reportArgumentType": { "$ref": "#/definitions/reportArgumentType" },