-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Fix check for overwritten properties in object spreads #44696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -26627,11 +26625,12 @@ namespace ts { | |||
|
|||
function checkSpreadPropOverrides(type: Type, props: SymbolTable, spread: SpreadAssignment | JsxSpreadAttribute) { | |||
for (const right of getPropertiesOfType(type)) { | |||
const left = props.get(right.escapedName); | |||
const rightType = getTypeOfSymbol(right); | |||
if (left && !maybeTypeOfKind(rightType, TypeFlags.Nullable) && !(maybeTypeOfKind(rightType, TypeFlags.AnyOrUnknown) && right.flags & SymbolFlags.Optional)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sandersn I'm not sure why we were checking nullable and any/unknown flags here. Seems to me any non-optional property that duplicates a property to the left should trigger the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a corner case where a type that included undefined
was actually for an optional property, but wasn't marked as such. For the others, it might be that unknown
and (sort of) any
include undefined
.
We can ship this in the beta and see if people complain. It's likely that with better optionality tracking, this is not a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to ship a stricter check to go with the stricter optional properties. If people complain in the beta we can revisit the decision.
@@ -26627,11 +26625,12 @@ namespace ts { | |||
|
|||
function checkSpreadPropOverrides(type: Type, props: SymbolTable, spread: SpreadAssignment | JsxSpreadAttribute) { | |||
for (const right of getPropertiesOfType(type)) { | |||
const left = props.get(right.escapedName); | |||
const rightType = getTypeOfSymbol(right); | |||
if (left && !maybeTypeOfKind(rightType, TypeFlags.Nullable) && !(maybeTypeOfKind(rightType, TypeFlags.AnyOrUnknown) && right.flags & SymbolFlags.Optional)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was a corner case where a type that included undefined
was actually for an optional property, but wasn't marked as such. For the others, it might be that unknown
and (sort of) any
include undefined
.
We can ship this in the beta and see if people complain. It's likely that with better optionality tracking, this is not a problem.
This PR fixes two issues:
undefined
.undefined
from a spread property type in--exactOptionalPropertyTypes
mode.Fixes #44438.