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

Void type narrowing stops working in callbacks #8541

Closed
blakeembrey opened this issue May 10, 2016 · 11 comments
Closed

Void type narrowing stops working in callbacks #8541

blakeembrey opened this issue May 10, 2016 · 11 comments
Labels
Duplicate An existing issue was already created

Comments

@blakeembrey
Copy link
Contributor

blakeembrey commented May 10, 2016

Seems like this would have been reported, but I had trouble finding a duplicate issue. Feel free to close if one does exist.

TypeScript Version:

nightly (1.9.0-dev.20160217)

Code

let foo: string | void
let cbr: (cb: (err: Error) => any) => any

if (foo) {
  cbr(function () {
    foo.substr(0, 10)
  })
}

Expected behavior: No error

Actual behavior:

index.ts(6,9): error TS2339: Property 'substr' does not exist on type 'string | void'.

Edit: Only visible on nightly - notice that in nightly foo.substr within the if (foo) is valid, but not within the callback function which is also inside if (foo).

@tinganho
Copy link
Contributor

tinganho commented May 10, 2016

Related to #8353 and #7770.

Statically there is nothing telling the compiler that the callback is executed immediately. Thus foo can be void inside the callback.

let foo: string | void
let cbr: (cb: (err: Error) => any) => any

if (foo) {
  cbr(function () { // compiler doesn't know when the callback is executed.
    foo.substr(0, 10)
  })
}
foo = undefined

@malibuzios
Copy link

I'm not sure if it's related to #8353, it seems closer to #8472.

They're somewhat like two sides of the same thing:

let x: number | string = "hi";

x; // 'x' is analyzed as 'string'
func();
x; // 'x' is analyzed as 'number'

function func() {
   x = 1;
}
let x: number | string = "hi";

// 'x' is analyzed as 'string' for execution of this particular call,
// so this may compile:
func();

function func() {
   // However, in general there is no way to know whether the type of 'x' would be
   // 'number' or 'string' here for an arbitrary caller context, so it technically 
   // may still error during compilation, unless perhaps more complex analysis is done:
   return x.charCodeAt(0); 
}

@blakeembrey
Copy link
Contributor Author

Thanks for the issues, they are all almost the same issues, but the solutions are probably much different. I understand the other issues stated are a bit more complicated - one requires understanding the call locations and another is around mutating variables outside the scope.

I understand that foo could be re-assigned, but in this case it's easy to analyse that because there's no future assignment to foo and it only got into the callback because it was truthy to start with. Just imagine that block is all wrapped in a closure, for example.

Anyway, if it sounds similar enough I'm happy for it to be closed - as long as the issue is being tracked somewhere 😄

@mhegazy
Copy link
Contributor

mhegazy commented May 10, 2016

this is a duplicate of #7662. you can see more discussion in #7719 (comment)

@mhegazy mhegazy closed this as completed May 10, 2016
@mhegazy mhegazy added the Duplicate An existing issue was already created label May 10, 2016
@blakeembrey
Copy link
Contributor Author

blakeembrey commented Jun 3, 2016

Actually, @mhegazy, I'd like to re-open this issue. I don't think it's entirely correct. For example, if it's a const, you can't re-assign the variable so even in a function it should stay narrowed.

@blakeembrey
Copy link
Contributor Author

All the referenced issues mention specifically var or let, but if you do something like:

const x: number | number[] = []

function cb () {
  x.push(1)
}

It should work. The const can not be re-assigned to another type.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 4, 2016

Const should work with latest in master.

@blakeembrey
Copy link
Contributor Author

Thanks. Is there a particular option to make it work? I just checked out master and am stilling getting:

index.ts(4,5): error TS2339: Property 'push' does not exist on type 'string | string[]'.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2016

Sorry about that, should have been more specific. it only narrows in function expressions and lambdas.. so this should work:

const x: number | number[] = []

var cb = function () {
    x.push(1)
}

now that i think about it, i do not see why this should not work for function declarations as well.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2016

created #8976 to track that.

@blakeembrey
Copy link
Contributor Author

Thanks @mhegazy

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

No branches or pull requests

4 participants