flag abstract base classes with no abstract methods

Author: KotlinIsland

docs/benefits-over-pyright/new-diagnostic-rules.md

@@ -191,6 +191,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:

docs/configuration/config-files.md

@@ -304,6 +304,8 @@ The following settings allow more fine grained control over the **typeCheckingMo
 
 - <a name="reportImplicitAbstractClass"></a> **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)
 
+- <a name="reportEmptyAbstractUsage"></a> **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)
+
 - <a name="reportIncompatibleUnannotatedOverride"></a> **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)
 
 - <a name="reportUnannotatedClassAttribute"></a> **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
 
 - <a name="reportSelfClsDefault"></a> **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.
 

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')

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
+                    );
+                }
             }
         }
 

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;
     }

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',

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',

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 = () =>

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'}"

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']);