Fix #20873: Enforce strictNullChecks for RHS of empty destructuring assignment#21110
Conversation
When strictNullChecks is on, check the RHS of the following
destructuring assignments for possible null or undefined:
const {} = ...
const [] = ...
let {} = ...
let [] = ...
({} = ...)
|
|
|
@sandersn can you please review this change. |
sandersn
left a comment
There was a problem hiding this comment.
1 question and 1 refactoring request.
| if (strictNullChecks && node.name.elements.length === 0) { | ||
| initializerType = checkNonNullType(initializerType, node); | ||
| } | ||
| checkTypeAssignableTo(initializerType, getWidenedTypeForVariableLikeDeclaration(node), node, /*headMessage*/ undefined); |
There was a problem hiding this comment.
Is checkTypeAssignableTo needed if there aren't any elements? Seems like it could in an else, and then initializerType wouldn't need to be updated in the if block.
There was a problem hiding this comment.
Good point. I did consider whether it would be possible to just return (or use an else) immediately after the null check. I was just unsure if checkTypeAssignableTo performs some side-effects (like caching in checkTypeRelatedTo) that might give different results if skipped.
Making that change doesn't break any tests so from what I can tell I think this would be ok. I'm still learning the details of the internals so I may be missing something.
There was a problem hiding this comment.
You're right, the cache might be filled earlier this way, but there's no point in doing work that isn't needed.
You can't return because the code still needs to checkParameterInitalizer, although maybe that's redundant with the new call to checkExpressionCached.
There was a problem hiding this comment.
I don't think checkParameterInitalizer gets called via checkExpressionCached, so unless it does some equivalent work, I need to move the call after the if-then-else, rather than in the else branch. My most recent commit got this wrong - will fix now.
| target = (<BinaryExpression>target).left; | ||
| } | ||
| if (target.kind === SyntaxKind.ObjectLiteralExpression) { | ||
| if (strictNullChecks && (<ObjectLiteralExpression>target).properties.length === 0) { |
There was a problem hiding this comment.
I would move this code into checkObjectLiteralAssignment.
Move object destructuring assignment to checkObjectLiteralAssignment Only check assignability of types in checkVariableLikeDeclaration for object/array destructuring when there are properties present in the pattern.
|
Just pinging @sandersn to say that I've addressed the suggested changes (I hope). I appreciate this is low priority so just look over it when you can (I just wanted to indicate that I hadn't forgotten about the PR). Is there a workflow for marking review comments as addressed/resolved? |
|
Thanks for the ping. I missed the new commits. We don't have an established workflow for marking comment state. I sometimes reply "done" but I think we are basically waiting for github to add that feature. Anyway, thanks for the change. Looks good! |
|
No problem! Thanks for reviewing it :) |
When strictNullChecks is on, check the RHS of the following destructuring assignments for possible null or undefined:
const {} = ...
const [] = ...
let {} = ...
let [] = ...
({} = ...)
Fixes #20873