Error on destructured nullables #60121
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@typescript-bot test it |
Collaborator
Andarist
commented
Oct 2, 2024
Comment on lines
15
to
+19
| const { p1 } = f(); // error | ||
| ~~ | ||
| !!! error TS2339: Property 'p1' does not exist on type 'unknown'. | ||
| ~~~ | ||
| !!! error TS2571: Object is of type 'unknown'. |
Contributor
Author
There was a problem hiding this comment.
This gets a little bit noisy because we essentially report the same problem twice.
There are cases in the changed baselines that require the error to be reported on the initializer, like here:
So if there is desire to limit the amount of reported errors I think it would be good to report consistently them on the initializer at all times. I think handling this differently for different kind of patterns will complicate the code. But if there is a strong belief that this would provide a better DX - I can implement the required changes.
Collaborator
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
Collaborator
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Contributor
benjaminjkraft
left a comment
benjaminjkraft
left a comment
There was a problem hiding this comment.
Neat, thanks! I'm not competent to review the code but I took a glance at the tests.
Does this work right if it's deeper inside the restructuring? The actual code that led to this is more like
const { f: { [k]: v } } = /* value that could be `{ f: undefined }` */(Maybe if you know the code better it obviously does handle this, but my quick glance made me think it might not.)
Contributor
Author
|
@benjaminjkraft good catch, this wasn't handled - I just pushed out a fix for this. @jakebailey could you rerun tests? the webpack break seems to be OK but the Effect's one is fishy - if you could build a playground for me that would help me with bisecting their repo to a repro case |
Member
|
@typescript-bot test it |
Collaborator
Collaborator
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Collaborator
|
@jakebailey Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
Collaborator
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: openfin |
Contributor
Author
|
DT break comes from a declaration like this: export declare function focus({
emitSynthFocused,
}?: {
emitSynthFocused: boolean;
}): Promise<void>;that can be legally emitted from (TS playground): export async function focus(
{
emitSynthFocused,
}: {
emitSynthFocused: boolean;
} = { emitSynthFocused: true },
) {}I'll have to ignore this check for declaration nodes |
Collaborator
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Contributor
Author
|
Effect's break is spooky. I can't repro it in the IDE, I can't repro it by running |
Collaborator
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR addresses destructuring errors with nullable initializers by implementing proper null checking for destructured parameters and values. The fix ensures that destructuring patterns with potentially null or undefined values generate appropriate error messages.
Key Changes:
- Adds null checking for destructuring patterns when no default initializer is provided
- Replaces expression checking with explicit non-null expression checking for destructuring initializers
- Updates error handling to provide specific nullable error messages (TS18047, TS18048, TS18049, TS18050)
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/compiler/checker.ts | Core implementation that adds null checking for binding element parents and non-null expression checking for destructuring initializers |
| tests/cases/conformance/es6/destructuring/bindingPatternWithNullableInitializer*.ts | New test cases covering destructuring with nullable types including undefined, null, and union types |
| tests/baselines/reference/*.errors.txt | Updated baseline files showing new error messages for destructuring nullable values |
Contributor
Author
|
@jakebailey, could you run the tests and build the playground for this again? |
Member
|
@typescript-bot test it |
Collaborator
Collaborator
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Collaborator
|
Hey @jakebailey, the results of running the DT tests are ready. There were interesting changes: Branch only errors:Package: openfin |
Collaborator
|
@jakebailey Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
Collaborator
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Collaborator
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Member
|
The DT one implies that this shouldn't apply in declaration files. focus({ emitSynthFocused }?: {
emitSynthFocused: boolean;
}): Promise<void>; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #60119
fixes #60179