handle implicit protocols as well as implicit abc's

DetachHead · Oct 15, 2024 · 2ca13d8 · 2ca13d8
1 parent bc2994a
commit 2ca13d8
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 23 deletions.
diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts
@@ -373,7 +373,7 @@ export class Checker extends ParseTreeWalker {
 
             this._validateInstanceVariableInitialization(node, classTypeResult.classType);
 
-            this._validateFinalClassNotAbstract(classTypeResult.classType, node);
+            this._validateClassNotAbstract(classTypeResult.classType, node);
 
             this._validateDataClassPostInit(classTypeResult.classType);
 
@@ -5079,9 +5079,10 @@ export class Checker extends ParseTreeWalker {
         });
     }
 
-    // If a class is marked final, it must implement all abstract methods,
-    // otherwise it is of no use.
-    private _validateFinalClassNotAbstract(classType: ClassType, errorNode: ClassNode) {
+    /*
+     * If a class is marked final, or if `reportImplicitAbstractClass` is enabled, it must implement all abstract methods
+     */
+    private _validateClassNotAbstract(classType: ClassType, errorNode: ClassNode) {
         if (!ClassType.supportsAbstractMethods(classType)) {
             return;
         }
@@ -5121,19 +5122,28 @@ export class Checker extends ParseTreeWalker {
                 }) + diagAddendum.getString(),
                 errorNode.d.name
             );
-        } else if (
-            !this._getActualBaseClasses(classType).some((baseClass) => baseClass.shared.fullName === 'abc.ABC') &&
-            (!classType.shared.declaredMetaclass?.shared ||
-                !('fullName' in classType.shared.declaredMetaclass.shared) ||
-                classType.shared.declaredMetaclass.shared.fullName !== 'abc.ABCMeta')
-        ) {
-            this._evaluator.addDiagnostic(
-                DiagnosticRule.reportImplicitAbstractClass,
-                LocMessage.classImplicitlyAbstract().format({
-                    type: classType.shared.name,
-                }) + diagAddendum.getString(),
-                errorNode.d.name
-            );
+        } 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')
+                )
+            ) {
+                const errorMessage = classType.shared.mro.some(
+                    (baseClass) => isClass(baseClass) && ClassType.isBuiltIn(baseClass, 'Protocol')
+                )
+                    ? LocMessage.classImplicitlyProtocol()
+                    : LocMessage.classImplicitlyAbstract();
+                this._evaluator.addDiagnostic(
+                    DiagnosticRule.reportImplicitAbstractClass,
+                    errorMessage.format({
+                        type: classType.shared.name,
+                    }) + diagAddendum.getString(),
+                    errorNode.d.name
+                );
+            }
         }
     }
 

diff --git a/packages/pyright-internal/src/localization/localize.ts b/packages/pyright-internal/src/localization/localize.ts
@@ -506,6 +506,8 @@ export namespace Localizer {
             new ParameterizedString<{ type: string }>(getRawString('Diagnostic.finalClassIsAbstract'));
         export const classImplicitlyAbstract = () =>
             new ParameterizedString<{ type: string }>(getRawString('Diagnostic.classImplicitlyAbstract'));
+        export const classImplicitlyProtocol = () =>
+            new ParameterizedString<{ type: string }>(getRawString('Diagnostic.classImplicitlyProtocol'));
         export const finalContext = () => getRawString('Diagnostic.finalContext');
         export const finalInLoop = () => getRawString('Diagnostic.finalInLoop');
         export const finalMethodOverride = () =>

diff --git a/packages/pyright-internal/src/localization/package.nls.en-us.json b/packages/pyright-internal/src/localization/package.nls.en-us.json
@@ -459,7 +459,8 @@
             "message": "Class \"{type}\" is marked final and must implement all abstract symbols",
             "comment": "{Locked='final'}"
         },
-        "classImplicitlyAbstract": "Class \"{type}\" does not explicitly extend `ABC` or specify `metaclass=ABCMeta` so it must implement all abstract symbols",
+        "classImplicitlyAbstract": "Class \"{type}\" is implicitly abstract because it extends an abstract class without implementing all abstract symbols. If this is intentional, add `ABC` to its base classes or use `metaclass=ABCMeta`.",
+        "classImplicitlyProtocol": "Class \"{type}\" is implicitly a `Protocol` because it extends a `Protocol` without implementing all abstract symbols. If this is intentional, add `Protocol` or `ABC` to its base classes, or use `metaclass=ABCMeta`.",
         "finalContext": {
             "message": "\"Final\" is not allowed in this context",
             "comment": "{Locked='Final'}"

diff --git a/packages/pyright-internal/src/tests/checker.test.ts b/packages/pyright-internal/src/tests/checker.test.ts
@@ -730,6 +730,9 @@ test('reportImplicitAbstractClass', () => {
 
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['implicitAbstractClass.py'], configOptions);
     TestUtils.validateResultsButBased(analysisResults, {
-        errors: [{ code: DiagnosticRule.reportImplicitAbstractClass, line: 6 }],
+        errors: [
+            { code: DiagnosticRule.reportImplicitAbstractClass, line: 7 },
+            { code: DiagnosticRule.reportImplicitAbstractClass, line: 25 },
+        ],
     });
 });
diff --git a/packages/pyright-internal/src/tests/samples/implicitAbstractClass.py b/packages/pyright-internal/src/tests/samples/implicitAbstractClass.py
@@ -1,14 +1,30 @@
 from abc import ABC, abstractmethod, ABCMeta
+from typing import Protocol, override
 
-class Foo(ABC): 
+class A(ABC): 
     @abstractmethod
     def asdf(self) -> int: ...
 
-class Bar(Foo): # error
+class B(A): # error
     pass
 
-class Baz(Foo, metaclass=ABCMeta): 
+class C(A, metaclass=ABCMeta): 
     pass
 
-class Qux(Foo, ABC):
+class D(A, ABC):
     pass
+
+class E(A):
+    @override
+    def asdf(self) -> int:
+        ...
+
+class F(Protocol):
+    @abstractmethod
+    def asdf(self) -> int: ...
+
+class G(F): # error
+    pass
+
+class H(F, Protocol):
+    pass