Skip to content

Is this a regression in control flow-based type analysis for --strictNullChecks? #25109

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

Closed
seanpoulter opened this issue Jun 20, 2018 · 4 comments
Labels
Duplicate An existing issue was already created

Comments

@seanpoulter
Copy link

seanpoulter commented Jun 20, 2018

TypeScript Version: 2.9.2

Search Terms: typeof undefined guard property strictnullchecks

Code

With --strictNullChecks:

const obj: { [key: string]: string | undefined } = {}

const withAssignment = (key: string): string | null => {
    const value = obj[key];
    return typeof value === 'undefined'
        ? null
        : value;  
}

const withTernary = (key: string): string | null =>
    return typeof obj[key] === 'undefined'
        ? null
        : obj[key];
// Type 'string | null | undefined' is not assignable to type 'string | null'.
// Type 'undefined' is not assignable to type 'string | null'.

const withIf = (key: string): string | null => {
    if (typeof obj[key] === 'undefined') {
        return null;
    } else {
        return obj[key];
        // Type 'string | undefined' is not assignable to type 'string | null'.
        // Type 'undefined' is not assignable to type 'string | null'.
    }
}

Expected behavior:

  • Just like the example in the docs but with an undefined instead of a string.
  • No error.
  • No additional explicit check.
  • The property does not change in this simple statement so the type assertion should be OK.

Actual behavior: 😓
untitled

Playground Link:
http://www.typescriptlang.org/play/#src=...

Related Issues:

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jun 20, 2018
@RyanCavanaugh
Copy link
Member

See #11483 / #10530

@seanpoulter
Copy link
Author

seanpoulter commented Jun 20, 2018

Thanks for the speedy reply @RyanCavanaugh. Since this is the same as #11483, how is it different than the docs on control flow based type analysis? Does that performance hit come from checking if the property access/getter modifies the object?

I'd like to propose two actions:

  1. Can we add this to the docs and how do we get that going? This should be as accessible and well documented as the CFA docs for typeof x === 'string. No one else should have to waste their time with this.

  2. In #11483 you said:

    Since it should almost always be possible to write const j = list[i] instead, this shouldn't be too burdensome.

    That wouldn't pass code review in the large functional-inspired React/Redux repo I with. Is there any way to opt-in to this check? How do we get that going?

@RyanCavanaugh
Copy link
Member

how is it different than the docs on control flow based type analysis? Does that performance hit come from checking if the property access/getter modifies the object?

The difference is x.y vs x[expr].

Notice that CFA works perfectly fine when the keys are static, e.g. if (a.b.c.d) { a.b.c.d.toString() } is OK. The issue is when the key is computed (obj[key]) because now we go from a static lookup of variables + properties we already know about to an infinite matrix of obj vs key, plus the associated cost of determining whether or not key has been mutated between accesses.

  1. We'd take a doc PR to improve that (maybe a section about arrays / indexed access?) - https://github.com/Microsoft/TypeScript-Handbook

  2. obj[key]! will remove null / undefined from the type

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants