Skip to content

support control flow analysis of tagged template calls #51426

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

Open
5 tasks done
turadg opened this issue Nov 7, 2022 · 6 comments
Open
5 tasks done

support control flow analysis of tagged template calls #51426

turadg opened this issue Nov 7, 2022 · 6 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@turadg
Copy link

turadg commented Nov 7, 2022

Suggestion

πŸ” Search Terms

return never tagged template unreachable code

Related issues:
#32695
#50363

βœ… Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

Control flow analysis detecting unreachable code after a tagged template function call that throws.

πŸ“ƒ Motivating Example

It's a surprise that the same function call does not have control flow analysis when used as a tagged template:

function throws(opt?: any): never {
    throw new Error()
}

function testFn() {
    throws(`reason`);
    
    // @ts-expect-error unreachable
    console.log("is unreachable!")
}

function testTag() {
    throws`reason`;
    
    // ts does not detect that this is unreachable
    console.log("is unreachable!")
}

(playground)

πŸ’» Use Cases

Throwing an exception with a message

Normally this would be something like,

assert(condition, 'explanation');

However for performance reasons we eschew the function call:

condition || Fail`explanation`;

The asserts works but the Fail doesn't. We could work around this by Fail('explanation') but we prefer to be consistent in use of the Fail function. So for our current work-around is to explicitly throw the tagged template.

@fatcerberus
Copy link

fatcerberus commented Nov 7, 2022

This is actually two separate issues, one of which is by design. The pattern condition || fail("explanation") simply doesn't work at all, as only top-level function calls affect control flow analysis.

#32695

A function call is analyzed as an assertion call or never-returning call when ...
... the call occurs as a top-level expression statement

@turadg
Copy link
Author

turadg commented Nov 7, 2022

Thanks for pointing that out. Is there any issue to track a feature request for condition || fail? If not, would one be welcomed? EDIT: yes, #50739

Do switch case blocks count as a top-level expression statement? Because we also use this instead of break.

@MartinJohns
Copy link
Contributor

Do switch case blocks count as a top-level expression statement?

A switch is not an expression at all.

@mhofman
Copy link

mhofman commented Nov 7, 2022

The lack of narrowing based on logical expression was previously filed as issue #50739.

We can focus this issue on the lack of control flow analysis through tagged template literal as the following code would show:

function throws(opt?: any): never {
    throw new Error()
}

declare const a: string | null;

function testFn() {
    if (!a) throws(`reason`);
    
    a.charAt(0); // a is correctly narrowed to `string`
}

function testTag() {
    if (!a) throws`reason`;
    
    a.charAt(0); // Object is possibly 'null'. ts(2531)
}

function testFnInSwitch() {
    switch(a) {
        case null: // No fallthrough correctly infered
            throws(`reason`);
        default:
            a.charAt(0); // a is correctly narrowed to `string`
    }
}

function testTagInSwitch() {
    switch(a) {
        case null: // Fallthrough case in switch. ts(7029)
            throws`reason`;
        default:
            a.charAt(0);  // Object is possibly 'null'. ts(2531)
    }
}

Playground Link: Provided

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Nov 8, 2022
@71
Copy link

71 commented Mar 25, 2023

I have implemented this in 71@de96190, but can't submit a PR as this issue is not in the "Backlog" milestone. Is there any way this could be added to the backlog? The proposed change is fairly minimal, but fixes this issue.

@71
Copy link

71 commented Mar 30, 2023

Gentle ping (sorry) -- would love to submit a PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants