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

Switch is considered exhaustive or not based on the code after it #18882

Closed
kendfrey opened this issue Oct 2, 2017 · 7 comments
Closed

Switch is considered exhaustive or not based on the code after it #18882

kendfrey opened this issue Oct 2, 2017 · 7 comments
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@kendfrey
Copy link

kendfrey commented Oct 2, 2017

TypeScript Version: 2.5.3

Code

With --noImplicitReturns:

1:

function foo(bar: "a"): number
{
    switch (bar)
    {
        case "a":
            return 1;
    }
}

2:

function foo(bar: "a"): number
{
    switch (bar)
    {
        case "a":
            return 1;
    }

    let unreachable;
}

3:

function foo(bar: "a"): number
{
    switch (bar)
    {
        case "a":
            return 1;
    }

    function doNothing()
    {
    }
}

Expected behavior:

Either:

(1), (2), and (3):

error TS2366: Function lacks ending return statement and return type does not include 'undefined'.

Or:

(1) and (3) compile.
(2):

error TS7027: Unreachable code detected.

Actual behavior:

(1) compiles.
(2) and (3):

error TS2366: Function lacks ending return statement and return type does not include 'undefined'.

Additional notes:

I'm not sure which is the correct behaviour, but right now it's inconsistent.

Originally posted as a question here: https://stackoverflow.com/q/46518643/785745

@ahejlsberg
Copy link
Member

Currently, when an exhaustive switch statement is the last statement of a function body, we consider the end point unreachable. We could potentially ignore trailing declarations that contain no executable code in this analysis.

@ahejlsberg ahejlsberg added the Suggestion An idea for TypeScript label Oct 2, 2017
@RyanCavanaugh RyanCavanaugh added the In Discussion Not yet reached consensus label Oct 2, 2017
@RyanCavanaugh RyanCavanaugh added Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Oct 30, 2017
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Oct 30, 2017
@RyanCavanaugh
Copy link
Member

Accepting PRs - when detecting whether the switch is "final", skip over any declarations that are hoisted (functions but not classes), declarations that are type-only (interface, type, const enum), or declarations that have no side effects (var)

@charlespierce
Copy link
Contributor

@RyanCavanaugh To make sure I understand correctly: Should a var declaration only be considered side-effect free if it has no initializer? I.e. var a; should be skipped but var a = 1; should not be?

Also, is there any reason why we can't consider any top-level exhaustive switch to be "final"? Since an exhaustive switch will necessarily return along all code paths, any code after it should be unreachable and thus a function with a top-level exhaustive switch won't ever have an implicit return.

@DanielRosenwasser
Copy link
Member

If it's truly exhaustive (i.e. it has a default and all branches throw or return) then that does seem reasonable.

@charlespierce
Copy link
Contributor

charlespierce commented Nov 16, 2017

@DanielRosenwasser The current logic that determines a Switch is exhaustive requires that the switch:

  1. Has no default branch

  2. Expression is a literal type, enum, or union of literal types.

  3. All sub-types of the expression are covered by the case statements within.

  4. All branches throw or return.

That seems at first glance to be more restrictive than your conditions. I'll take a look at implementing it so that functions with exhaustive switches never have implicit returns.

@charlespierce
Copy link
Contributor

PR Opened with the above logic #20063

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Nov 16, 2017
@mhegazy mhegazy modified the milestones: Community, TypeScript 2.7 Nov 16, 2017
@mhegazy
Copy link
Contributor

mhegazy commented Nov 16, 2017

thanks @charlespierce !

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Effort: Moderate Requires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual". Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants