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

T | U var with initial type T, set to a U inside lambda always deduced as T #10613

Closed
zenmumbler opened this issue Aug 30, 2016 · 6 comments
Closed
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@zenmumbler
Copy link

zenmumbler commented Aug 30, 2016

TypeScript Version: 2.0.2 (RC)

Code

function test(lines: string[]) {
    var curThing: { x: number } | null = null;

    lines.forEach(line => {
        curThing = { x: 1 };
    });

    if (curThing) {
        console.info(curThing.x);
    }
}

Expected behavior:

No errors.

Actual behavior:

When strict null checks are on, TS2 RC deduces that curThing in the if at the end is of type null and inside the block is of type never. The error I get then is that never has no member named x. Fair enough.

In this case I can sidestep the issue by using for (var line of lines) { … } instead of the forEach method, but that may not always be the case. BTW, this is a reduced case from some parser code.

This case also deals with T | null but this applies to any T | U type value. I tested this separately with a string | number initialised to a number and then set to a string, etc.

@RyanCavanaugh RyanCavanaugh added Working as Intended The behavior described is the intended behavior; this is not a bug Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Aug 30, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Aug 30, 2016

The issue is the compiler has no idea when the function will be called. in this case lines.forEach will call the lambda immediately. but if this was setTimeout it would not. so doing this for the general case is not possible. all it knows is that the last definite assignment for curThing is null, and the if statement filters that out, leaving the type never.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 30, 2016

i suppose this is a regression from #8548

@mhegazy mhegazy added this to the TypeScript 2.0.3 milestone Aug 30, 2016
@zenmumbler
Copy link
Author

zenmumbler commented Sep 8, 2016

Not sure if this could be applied to TS, but Swift supports the @noescape modifier on functions passed as parameters that indicates that the passed function will execute before the called function exits. E.g. (Swift code follows)

func example(@noescape lamb: () -> ()) {
    lamb()
}

The compiler then checks that noescape function parameters are not passed to non-noescape function parameters, etc. In Swift this is mainly for some optimisations (and perhaps static analysis) but in TS this could provide static checking and thus accurate determination if a function param will or may not execute within the call context of the owning function.

Downsides: all of the standard library methods would have to be annotated and for it to work for me as well I would have to annotate it as well, but the only other option is cross-module SA (which is also impossible for any native functions)

@zenmumbler
Copy link
Author

To follow up on mhegazy's first comment: Indeed, the compiler right now has no way of knowing what will happen, but it pretends it knows anyway by assuming the lambda will not be called directly. If it truly cannot know, it has to leave the type alone and have the programmer do any manual casts, etc.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 13, 2016

After looking into it more, and catching up on changes in the last month or so, the reversal of the behavior in #8548 is intentional. see #10118 for more details.

My comment earlier still holds. the compiler does not check lambdas, or any calls for possible side effects. it can not make an assumption about the lambda being called immediately or not. Consider the same example with setTimeout in place for lines.forEach. Adding an annotation on the function parameter declaration seems like a right direction, but it does not provide the full solution either. consider:

var curThing: { x: number } | null = null;

lines.forEach(changeValueOfcurThing)

if (curThing) {
    console.info(curThing.x);
}

function changeValueOfcurThing {
    curThing = {x:1}
}

To get this right, first every call site has to be investigated for possible side effects, the annotation on the function declaration should be consulted, and the control flow analysis should be done on the function body as if it was inlined at the call site, then conclude that curThing would actually be non-nullable.

All of this is theoretically doable; but doing this would drastically increase the compilation time and the complexity of the compiler.

The workarounds available now, is using the bang ! operator to assert that the value is not empty, e.g.:

    if (curThing) {
        console.info(curThing!.x); // trust me curThing is not null
    }

use a cast for null:

function test(lines: string[]) {
    var curThing: { x: number }  = <any> null;

    // i am sure it will be set to a non-null value later on    
    lines.forEach(line => {
        curThing = { x: 1 };
    });

    if (curThing) {
        console.info(curThing.x);
    }
}

@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Sep 13, 2016
@mhegazy mhegazy closed this as completed Sep 13, 2016
@zenmumbler
Copy link
Author

Fair enough. I realised this would be a tricky one but figured I'd put it up. Thanks for giving it consideration.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants