Synthesize typed NamedTuple._replace so extra fields are errors - #11642
Synthesize typed NamedTuple._replace so extra fields are errors#11642Henry Su (hsusul) wants to merge 1 commit into
Conversation
…ors. typeshed exposes _replace as **kwargs: Any, so unknown field names and incompatible values were accepted even though CPython raises TypeError.
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
Rich Chiodo (rchiodo)
added
the
review-auto:changes-requested
| if (addGenericGetAttribute) { | ||
| FunctionType.addDefaultParams(replaceType); | ||
| } else { | ||
| constructorType.shared.parameters.forEach((param) => { |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Filtering parameters by the names self and cls removes legal NamedTuple fields with those names. For example, NamedTuple("NT", [("self", int)]) will not accept _replace(self=2). Skip only the actual receiver parameter and add regression coverage for both field names.
| replaceType, | ||
| FunctionParam.create( | ||
| param.category, | ||
| param._type, |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
This derives replacement fields from the synthesized constructor while the class-syntax NamedTuple path builds the corresponding signature in dataClasses.ts. The duplicated policy can drift between class and factory NamedTuples; extract a shared field-signature builder or otherwise centralize this logic.
Rich Chiodo (rchiodo)
left a comment
Rich Chiodo (rchiodo)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Rich Chiodo (rchiodo)
added
review-auto:approved
Heejae Chang (heejaechang)
left a comment
Heejae Chang (heejaechang)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
|
||
| if (addGenericGetAttribute) { | ||
| FunctionType.addDefaultParams(replaceType); | ||
| } else { |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Does FunctionType.addDefaultParams add an *args: Any parameter here? If so, dynamically shaped NamedTuples will accept positional _replace updates even though _replace is keyword-only. Please add only permissive **kwargs support and cover rejection of _replace(123).
| } | ||
|
|
||
| if (synthesizeDunderReplace) { | ||
| classFields.set('__replace__', Symbol.createWithType(SymbolFlags.ClassMember, replaceType)); |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
On Python 3.13, this installs the function named __replace__ under _replace as well. Consumers of shared.name, including signature and diagnostic rendering, can therefore identify _replace as __replace__. Please synthesize separate function instances while sharing the parameter construction.
| @@ -0,0 +1,43 @@ | |||
| # This sample tests that NamedTuple _replace rejects unknown fields | |||
| # and type-incompatible field values, matching runtime TypeError behavior. | |||
There was a problem hiding this comment.
Info · Optional note
The comment says incompatible annotated values match runtime TypeError behavior, but NamedTuple annotations are not enforced at runtime. Please limit the runtime claim to unexpected field names and describe incompatible values as static type-checking errors.
|
Verification: The relevant tests could not be fully run in the isolated environment; this review is not fully verified. |
3 participants
Summary
._replace()is typed in typeshed as**kwargs: Any, so extra field names and incompatible values were accepted.TypeError: Got unexpected field names(and type mismatches fail similarly).__replace__was already synthesized with a keyword-only field signature on 3.13+;_replacenow uses the same signature for class-syntax and factory NamedTuples.__replace__behavior is unchanged.Test plan
npx jest typeEvaluator4.test.ts -t DataClassReplace1 --forceExit(frompackages/pyright-internal)npx jest typeEvaluator8.test.ts -t NamedTuple --forceExitnpx jest checker.test.ts -t Private1 --forceExitNT(1)._replace(y=2)reports an unknown-parameter error in the language server