Improved error message for TypedDict base classes that are not closed…

… but specify an `extra_items`. Also added check for cases where `closed` and `extra_items` are both specified. This addresses #9804. (#9827)
microsoft · Feb 6, 2025 · 74b11d4 · 74b11d4
1 parent 99a625e
commit 74b11d4
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 33 deletions.
diff --git a/packages/pyright-internal/src/analyzer/typeEvaluator.ts b/packages/pyright-internal/src/analyzer/typeEvaluator.ts
@@ -17208,6 +17208,7 @@ export function createTypeEvaluator(
             if (fileInfo.isStubFile) {
                 exprFlags |= EvalFlags.ForwardRefs;
             }
+            let sawClosedOrExtraItems = false;
 
             node.d.arguments.forEach((arg) => {
                 // Ignore unpacked arguments.
@@ -17478,20 +17479,33 @@ export function createTypeEvaluator(
                             );
                         } else if (arg.d.name.d.value === 'total' && !constArgValue) {
                             classType.shared.flags |= ClassTypeFlags.CanOmitDictValues;
-                        } else if (arg.d.name.d.value === 'closed' && constArgValue) {
-                            // This is an experimental feature because PEP 728 hasn't been accepted yet.
-                            if (AnalyzerNodeInfo.getFileInfo(node).diagnosticRuleSet.enableExperimentalFeatures) {
-                                classType.shared.flags |=
-                                    ClassTypeFlags.TypedDictMarkedClosed | ClassTypeFlags.TypedDictEffectivelyClosed;
-
-                                if (classType.shared.typedDictExtraItemsExpr) {
-                                    addDiagnostic(
-                                        DiagnosticRule.reportGeneralTypeIssues,
-                                        LocMessage.typedDictExtraItemsClosed(),
-                                        classType.shared.typedDictExtraItemsExpr
-                                    );
+                        } else if (arg.d.name.d.value === 'closed') {
+                            if (constArgValue) {
+                                // This is an experimental feature because PEP 728 hasn't been accepted yet.
+                                if (AnalyzerNodeInfo.getFileInfo(node).diagnosticRuleSet.enableExperimentalFeatures) {
+                                    classType.shared.flags |=
+                                        ClassTypeFlags.TypedDictMarkedClosed |
+                                        ClassTypeFlags.TypedDictEffectivelyClosed;
+
+                                    if (classType.shared.typedDictExtraItemsExpr) {
+                                        addDiagnostic(
+                                            DiagnosticRule.reportGeneralTypeIssues,
+                                            LocMessage.typedDictExtraItemsClosed(),
+                                            classType.shared.typedDictExtraItemsExpr
+                                        );
+                                    }
                                 }
                             }
+
+                            if (sawClosedOrExtraItems) {
+                                addDiagnostic(
+                                    DiagnosticRule.reportGeneralTypeIssues,
+                                    LocMessage.typedDictExtraItemsClosed(),
+                                    arg.d.valueExpr
+                                );
+                            }
+
+                            sawClosedOrExtraItems = true;
                         }
                     } else if (arg.d.name.d.value === 'extra_items') {
                         // This is an experimental feature because PEP 728 hasn't been accepted yet.
@@ -17509,6 +17523,16 @@ export function createTypeEvaluator(
                                 );
                             }
                         }
+
+                        if (sawClosedOrExtraItems) {
+                            addDiagnostic(
+                                DiagnosticRule.reportGeneralTypeIssues,
+                                LocMessage.typedDictExtraItemsClosed(),
+                                arg.d.valueExpr
+                            );
+                        }
+
+                        sawClosedOrExtraItems = true;
                     } else {
                         addDiagnostic(
                             DiagnosticRule.reportGeneralTypeIssues,

diff --git a/packages/pyright-internal/src/analyzer/typedDicts.ts b/packages/pyright-internal/src/analyzer/typedDicts.ts
@@ -187,6 +187,7 @@ export function createTypedDictType(
 
     if (usingDictSyntax) {
         const argsToConsider = argList.slice(2);
+        let sawClosedOrExtraItems = false;
 
         for (const arg of argsToConsider) {
             if (arg.name?.d.value === 'total' || arg.name?.d.value === 'closed') {
@@ -205,16 +206,38 @@ export function createTypedDictType(
                     );
                 } else if (arg.name.d.value === 'total' && arg.valueExpression.d.constType === KeywordType.False) {
                     classType.shared.flags |= ClassTypeFlags.CanOmitDictValues;
-                } else if (arg.name.d.value === 'closed' && arg.valueExpression.d.constType === KeywordType.True) {
-                    // This is an experimental feature because PEP 728 hasn't been accepted yet.
-                    if (AnalyzerNodeInfo.getFileInfo(errorNode).diagnosticRuleSet.enableExperimentalFeatures) {
-                        classType.shared.flags |=
-                            ClassTypeFlags.TypedDictMarkedClosed | ClassTypeFlags.TypedDictEffectivelyClosed;
+                } else if (arg.name.d.value === 'closed') {
+                    if (arg.valueExpression.d.constType === KeywordType.True) {
+                        // This is an experimental feature because PEP 728 hasn't been accepted yet.
+                        if (AnalyzerNodeInfo.getFileInfo(errorNode).diagnosticRuleSet.enableExperimentalFeatures) {
+                            classType.shared.flags |=
+                                ClassTypeFlags.TypedDictMarkedClosed | ClassTypeFlags.TypedDictEffectivelyClosed;
+                        }
                     }
+
+                    if (sawClosedOrExtraItems) {
+                        evaluator.addDiagnostic(
+                            DiagnosticRule.reportGeneralTypeIssues,
+                            LocMessage.typedDictExtraItemsClosed(),
+                            arg.valueExpression || errorNode
+                        );
+                    }
+
+                    sawClosedOrExtraItems = true;
                 }
             } else if (arg.name?.d.value === 'extra_items') {
                 classType.shared.typedDictExtraItemsExpr = arg.valueExpression;
                 classType.shared.flags |= ClassTypeFlags.TypedDictEffectivelyClosed;
+
+                if (sawClosedOrExtraItems) {
+                    evaluator.addDiagnostic(
+                        DiagnosticRule.reportGeneralTypeIssues,
+                        LocMessage.typedDictExtraItemsClosed(),
+                        arg.valueExpression || errorNode
+                    );
+                }
+
+                sawClosedOrExtraItems = true;
             } else {
                 evaluator.addDiagnostic(
                     DiagnosticRule.reportCallIssue,
@@ -223,17 +246,6 @@ export function createTypedDictType(
                 );
             }
         }
-
-        if (ClassType.isTypedDictMarkedClosed(classType) && classType.shared.typedDictExtraItemsExpr) {
-            const arg = argsToConsider.find((arg) => arg.name?.d.value === 'extra_items');
-
-            // A TypedDict cannot be "closed" and allow extra_items at the same time.
-            evaluator.addDiagnostic(
-                DiagnosticRule.reportGeneralTypeIssues,
-                LocMessage.typedDictExtraItemsClosed(),
-                arg?.valueExpression ?? errorNode
-            );
-        }
     }
 
     synthesizeTypedDictClassMethods(evaluator, errorNode, classType);

diff --git a/packages/pyright-internal/src/localization/package.nls.en-us.json b/packages/pyright-internal/src/localization/package.nls.en-us.json
@@ -1476,7 +1476,7 @@
             "comment": "{Locked='True','False'}"
         },
         "typedDictClosedExtras": {
-            "message": "Base class \"{name}\" is a closed TypedDict; extra items must be type \"{type}\"",
+            "message": "Base class \"{name}\" is a TypedDict that limits the type of extra items to type \"{type}\"",
             "comment": "{Locked='closed','TypedDict'}"
         },
         "typedDictClosedNoExtras": {
@@ -1498,8 +1498,8 @@
             "comment": "{Locked='TypedDict'}"
         },
         "typedDictExtraItemsClosed": {
-            "message": "A TypedDict cannot be closed if it supports extra items",
-            "comment": "{Locked='TypedDict','closed'}"
+            "message": "TypedDict can use either \"closed\" or \"extra_items\" but not both",
+            "comment": "{Locked='TypedDict','closed','extra_items'}"
         },
         "typedDictFieldNotRequiredRedefinition": {
             "message": "TypedDict item \"{name}\" cannot be redefined as NotRequired",

diff --git a/packages/pyright-internal/src/tests/samples/typedDictClosed1.py b/packages/pyright-internal/src/tests/samples/typedDictClosed1.py
@@ -5,7 +5,7 @@
 from typing_extensions import ReadOnly  # pyright: ignore[reportMissingModuleSource]
 
 
-class Movie(TypedDict, closed=False, extra_items=bool):
+class Movie(TypedDict, extra_items=bool):
     name: str
 
 
@@ -52,3 +52,9 @@ class BadTD2(TypedDict, extra_items=NotRequired[str]):
 # This should generate an error.
 class BadTD3(TypedDict, closed=True, extra_items=str):
     pass
+
+
+# This should generate an error because "closed" and
+# "extra_items" cannot both be specified.
+class BadTD4(TypedDict, closed=False, extra_items=bool):
+    name: str
diff --git a/packages/pyright-internal/src/tests/typeEvaluator5.test.ts b/packages/pyright-internal/src/tests/typeEvaluator5.test.ts
@@ -357,7 +357,7 @@ test('TypedDictClosed1', () => {
     configOptions.diagnosticRuleSet.enableExperimentalFeatures = true;
 
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typedDictClosed1.py'], configOptions);
-    TestUtils.validateResults(analysisResults, 6);
+    TestUtils.validateResults(analysisResults, 7);
 });
 
 test('TypedDictClosed2', () => {