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

'Never' is not checked for unreachability #10973

Closed
tinganho opened this issue Sep 18, 2016 · 5 comments
Closed

'Never' is not checked for unreachability #10973

tinganho opened this issue Sep 18, 2016 · 5 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@tinganho
Copy link
Contributor

function fail(): never {
    throw new Error('oops');
}

function test1() {
    let str: string;
    fail();
    str = 'helloworld';  // No error. Should error with unreachable code.
}

function test2() {
    let str: string;
    if (Math.random() > 0.5) {
        str = 'helloworld';
    }
    else {
        fail();
    }
    let str2 = str + ''; // Error: str is used before being assigned. But should not error.
}
@yortus
Copy link
Contributor

yortus commented Sep 18, 2016

I raised this point in #9260 and @ahejlsberg responded in #9260 (comment) (see second half of comment).

@tinganho
Copy link
Contributor Author

@yortus your example which @ahejlsberg commented on is slightly different then this case. That example is for inferred 'never' typing for functions. In my example I use a declared 'never' function.

Just quoting @ahejlsberg :

We only report unreachable code errors based on the structure of the code, i.e. when we are 100% certain the code is unreachable in all circumstances.

Maybe inferred cases cannot reach 100%. I don't know. But for me, this example is 100%:

function test1() {
    let str: string;
    fail();
    str = 'helloworld';  // No error. Should error with unreachable code.
}

Because it is nearly exactly the same as this one:

function test1() {
    let str: string;
    throw new Error('oops');
    str = 'helloworld';  // Error unreachable code detected
}

This is also 100%:

function test2() {
    let str: string;
    if (Math.random() > 0.5) {
        str = 'helloworld';
    }
    else {
        fail();
    }
    let str2 = str + ''; // Error: str is used before being assigned. But should not error.
}

Because this one already are:

function test2() {
    let str: string;
    if (Math.random() > 0.5) {
        str = 'helloworld';
    }
    else {
        throw new Error('oops');
    }
    let str2 = str + ''; // No error
}

@yortus
Copy link
Contributor

yortus commented Sep 18, 2016

@tinganho First of all, I would prefer the behaviour you are suggesting here (i.e. detect the unreachable code and error).

That aside, I don't think the compiler currently does any cross-function analysis, so it doesn't assume the code after calling fail() is unreachable. So I suspect @ahejlsberg's comment does apply to your example as well. It would be great if he could clarify it himself.

@ahejlsberg
Copy link
Member

To clarify what I said here, reachability analysis considers only the grammatical structure of the code, i.e. it doesn't factor in types. So, we know for certain that control won't reach a statement following a throw or return, but we don't know for certain that a function that promises to never return won't actually return. The distinction is important when you for example consider that JavaScript code might violate type annotations by passing parameters or returning values that don't conform to the annotated types.

That said, I agree it would be nice if we could report reachability errors following calls to never-returning functions, be they implicitly or explicitly typed. However, it's not a trivial change given how the analysis is currently structured. Meanwhile, you can make it work by writing return fail() instead of just fail():

function fail(): never {
    throw new Error('oops');
}

function test1() {
    let str: string;
    return fail();
    str = 'helloworld';  // Error, unreachable code
}

function test2() {
    let str: string;
    if (Math.random() > 0.5) {
        str = 'helloworld';
    }
    else {
        return fail();
    }
    let str2 = str + '';  // Ok
}

@tinganho
Copy link
Contributor Author

tinganho commented Sep 19, 2016

I think I'm starting to see a pattern here: never functions must be called in places where they can never be reached. Called elsewhere is not their intentions.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Sep 29, 2016
@mhegazy mhegazy closed this as completed Sep 29, 2016
@mhegazy mhegazy added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Sep 29, 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
Design Limitation Constraints of the existing architecture prevent this from being fixed Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants