Skip to content
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

Strict null checks and flow control of returned functions #9263

Closed
blakeembrey opened this issue Jun 20, 2016 · 7 comments
Closed

Strict null checks and flow control of returned functions #9263

blakeembrey opened this issue Jun 20, 2016 · 7 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@blakeembrey
Copy link
Contributor

TypeScript Version:

Version 1.9.0-dev.20160619-1.0

Code

function doThing (x?: { prop: boolean }) {
  if (x == null) {
    return function () {
      throw new TypeError('Not correctly set up')
    }
  }

  return function () {
    return x.prop
  }
}
{
  "compilerOptions": {
    "strictNullChecks": true
  }
}

Expected behavior: Successful compilation.

Actual behavior:

index.ts(9,12): error TS2532: Object is possibly 'undefined'.

I honestly wasn't sure what this exact feature would be called to find a duplicate, and I know I've logged a similar issues issues in the past. Let me know if a duplicate does exist. In any case, I would expect flow control to have understood the undefined case was already handled. I suppose this could be related to the fact the arguments are mutable? Is it possible to use flow control analysis to understand nothing re-assigns the value (in fact, nothing else even executes) after that return?

@kitsonk
Copy link
Contributor

kitsonk commented Jun 20, 2016

Flow control is not preserved across function boundaries, because TypeScript cannot be sure when the function is going to be invoked. It is discussed here.

@blakeembrey
Copy link
Contributor Author

blakeembrey commented Jun 20, 2016

@kitsonk Thanks, I have read that issue before. However, I disagree with the generality. I believe there are multiple cases that can be handled here. For instance, const x is handled across function boundaries because it can not be re-assigned.

Function Expression (Handled Today):

const y: { prop: string } | undefined = { prop: 'tets' }

if (y) {
  let test = () => {
    return y.prop
  }
}

Conditional (Handled Today):

let y: string | number = 'test'

if (typeof y === 'string') {
  y.charAt(0)
  // Using `y = 10` within this block would cause `y.charAt` to become an error.
}

y = 10

So, given these are handled correctly today, what is the difference between this and other variables that are already block scoped and never modified? For instance:

function test (x: string | number) {
  if (typeof x === 'string') {
    setTimeout(function () {
      console.log(x.length)
    })
    // Note the **return** here, just as in the original example, which means nothing else in this block can modify `x`.
    return
  }
}

However, even given the above fact, if return were not called but x never modified within the block of the function (which is what x is scoped to) you can also infer x never changes.

@RyanCavanaugh
Copy link
Member

The problem is that we don't realize that there are no assignments to x in the body, so this code is effectively indistinguishable from the unsafe version:

function doThing (x?: { prop: boolean }) {
  if (x == null) {
    return function () {
      throw new TypeError('Not correctly set up')
    }
  }
  let danger = function () {
    return x.prop;
  }
  x = null;
  return danger;
}

One workaround is to write const y = x at the top of your function. A proposed fix is to allow a const modifier on function parameters to avoid having to do that.

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jun 20, 2016
@blakeembrey
Copy link
Contributor Author

blakeembrey commented Jun 20, 2016

@RyanCavanaugh Thanks. Is it possible to include it as part of the flow control logic that currently exists? If it can error when y is re-assigned (becoming number | string) within an if block (earlier comment), I imagine it makes sense for this behaviour to be expanded for functions within the same block.

Edit: Sorry, to clarify, this was definitely intended as a feature request and not a bug report - I just filled out what it asked of me in the OP.

@yortus
Copy link
Contributor

yortus commented Jun 21, 2016

A proposed fix is to allow a const modifier on function parameters to avoid having to do that.

👍 to this, there are many places in our code that would benefit from this. It would satisfy tsc's control flow analysis and we could get rid of a bunch of dummy local vars that are there to help tsc but are making the code less readable.

@jasonswearingen
Copy link

a workaround: explcitly cast to a non-undefined union type, as per:

function doThing (_x?: { prop: boolean }) {
  let x = _x as { prop: boolean };
  if (x == null) {
    return function () {
      throw new TypeError('Not correctly set up')
    }
  }

  return function () {
    return x.prop
  }
}

@mhegazy
Copy link
Contributor

mhegazy commented Sep 26, 2016

This should be fixed by #10357. If a function parameter is never assigned to, it is treated as a const. so the sample in the OP compiles fine under TS 2.0.2 or later.

@mhegazy mhegazy closed this as completed Sep 26, 2016
@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Sep 26, 2016
@mhegazy mhegazy added this to the TypeScript 2.0.2 milestone Sep 26, 2016
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants