Check destructuring validity the same way element accesses and indexed accesses are checked #24700
Conversation
…d accesses are checked
| // 1: | ||
| for (let {[a]: a} of [{ }]) continue; | ||
| ~ | ||
| !!! error TS2448: Block-scoped variable 'a' used before its declaration. | ||
| ~ | ||
| !!! error TS2538: Type 'any' cannot be used as an index type. |
There was a problem hiding this comment.
This doesn't feel like a legal error, but it seems like we're only issuing it when there's already an error on the same node. Is there an issue with the error type leaking out into an error message?
There was a problem hiding this comment.
Most certainly. It's because we choose to consider indexing with any an error today (esp. if the thing being indexed has no index signatures), but destructuring accesses don't flag that right now. This PR aligns them.
| ? getLateBoundNameFromType(indexType) | ||
| : accessExpression && checkThatExpressionIsProperSymbolReference(accessExpression.argumentExpression, indexType, /*reportError*/ false) | ||
| ? getPropertyNameForKnownSymbolName(idText((<PropertyAccessExpression>accessExpression.argumentExpression).name)) | ||
| : accessNode && isPropertyName(accessNode) |
There was a problem hiding this comment.
I don't think you need this case.
There was a problem hiding this comment.
Needed to handle well-known symbol names, which are not normal property names or unique symbols:
let { [Symbol.iterator]: destructured } = []| @@ -9521,7 +9499,7 @@ namespace ts { | |||
| return instantiateType(getTemplateTypeFromMappedType(objectType), templateMapper); | |||
| } | |||
|
|
|||
| function getIndexedAccessType(objectType: Type, indexType: Type, accessNode?: ElementAccessExpression | IndexedAccessTypeNode, missingType = accessNode ? errorType : unknownType): Type { | |||
| function getIndexedAccessType(objectType: Type, indexType: Type, accessNode?: ElementAccessExpression | IndexedAccessTypeNode | PropertyName, useApparentObjectType?: boolean, missingType = accessNode ? errorType : unknownType): Type { | |||
There was a problem hiding this comment.
It's now gone, and I'm using the raw apparent type as the input directly instead. The downside is that error messages now reference the apparent type instead of the original type, too.
| return errorType; | ||
| } | ||
| const exprType = isComputedPropertyName(name) | ||
| ? checkExpression(name.expression) |
There was a problem hiding this comment.
Use checkComputedPropertyName here.
Fixes #24661
This causes some error messages to change to match the indexing case (IMO, good for consistency, since the messages were almost the same). To preserve some semantics destructuring had with private/protected members of generics (eg,
this), some shuffling of apparent types had to be done as well. Lastly, we seem to actually pick up a few error cases we somehow just missed before, eg, destructuring with astringcomputed name when there's nostringindex signature.