Skip to content

fix(43030): undefined[] converted to any[] in JS #43933

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

Merged
merged 1 commit into from
May 11, 2021

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #43030

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 3, 2021
@DanielRosenwasser
Copy link
Member

Now I really don't understand the subtleties here.

@DanielRosenwasser DanielRosenwasser requested a review from sandersn May 6, 2021 08:20
@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels May 6, 2021
@a-tarasyuk
Copy link
Contributor Author

@DanielRosenwasser I think this issue is a little bit tricky. The problem here is that for Nullable types (only in JavaScript files and in both strict and not strict modes), the checker returns any. It doesn't matter if it's defined explicitly or implicitly.

/** @type {() => undefined} */
function f1() {
    return undefined;
}
const a = f1()
>a : any
>f1() : undefined
>f1 : () => undefined
/** @type {() => null} */
function f1() {
    return null;
}
const a = f1()
>a : any
>f1() : undefined
>f1 : () => undefined

For instance for a similar case for arrays (line #32342) the checker checks to implicitNeverType in strict mode and for undefinedWideningType in non-strict mode., maybe we need to use a similar logic here.

Or use strictNullChecks option and return any in non-strict mode (!strictNullChecks && (widened.flags & TypeFlags.Nullable))

// @strictNullChecks: false
/** @type {() => undefined} */
function f1() {
    return undefined;
}
const a = f1()
>a : any
>f1() : undefined
>f1 : () => undefined
// @strictNullChecks: true
/** @type {() => undefined} */
function f1() {
    return undefined;
}
const a = f1()
>a : undefined
>f1() : undefined
>f1 : () => undefined

Does that make sense?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test changes look good. I have some questions about the implementation though. Also, I think we should hold this change for 4.4 in order to make it available to users to test for a long time.

  • I looked at the similar code that you pointed out in isEmptyArrayLiteralType, and it only applies to strictNullChecks. Why don't you need the same choice between === implicitNeverType vs === undefinedWideningType in this fix?
  • why neverType vs implicitNeverType, which is what isEmptyArrayLiteralType uses?

I thought it might be time to revisit the choice of having []: any[] in strict null checks in js instead of []: never[] like in TS, but decided that the additional annotations would still not be appropriate for even strict JS. I suspect that switching isEmptyArrayLiteralType to use the check introduced in this PR would get us that. Is that right?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So..making both use the same check doesn't make []: never[] under strictnullchecks. Interesting, there must be an ad-hoc conversion I've forgotten about. This is a good change for strictnullchecks, since authors probably care about var x = null having the correct type in that case. We should do some more thinking and maybe usage analysis for the empty array case in the future.

@sandersn
Copy link
Member

Update: looking closer at the baselines, the conversion must happen for initialisers, because in var x = [], []: never[] but x: any[].

@sandersn sandersn merged commit 463c794 into microsoft:master May 11, 2021
@weswigham weswigham added the Breaking Change Would introduce errors in existing code label Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Would introduce errors in existing code For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined[] converted to any[] in JS
5 participants