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

Control flow based type narrowing for assert(...) calls #8655

Closed
yortus opened this issue May 18, 2016 · 27 comments · Fixed by #32695
Closed

Control flow based type narrowing for assert(...) calls #8655

yortus opened this issue May 18, 2016 · 27 comments · Fixed by #32695
Assignees
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Fix Available A PR has been opened for this issue

Comments

@yortus
Copy link
Contributor

yortus commented May 18, 2016

Now that TypeScript does control flow based type analysis, and there is a never type in the works, is it possible to consider providing better type checking around assert(...) function calls that assert that a variable has a certain type at runtime?

TL;DR: some assert functions are really just type guards that signal via return/throw rather than true/false. Example:

assert(typeof s === "string"); // throws if s is not a string
s.length; // s is narrowed to string here...

Problem

Asserts are common in contract-based programming, but I've also been coming across this scenario regularly whilst traversing JavaScript ASTs based on the Parser API (I'm using babel to produce/traverse ASTs).

For example, consider the MemberExpression:

memberexpression

Note we can assume property is an Identifier if computed===false. This is what I'd like to write:

function foo(expr: MemberExprssion) {
    if (expr.computed) {
        // handle computed case
    }
    else {
        // since computed===false, we know property must be an Identifier
        assert(isIdentifier(expr.property));

        let name = expr.property.name;  // ERROR: name doesn't exist on Identifier|Expression
    }
}

Unfortunately that doesn't compile, because expr.property does not get narrowed after the assert(...) call.

To get the full benefit of control flow analysis currently, you have to expand the assert call inline:

...
else {
    if (!isIdentifier(expr.property)) {
        throw new AssertionError(`Expected property to be an Identifier`);
    }

    let name = expr.property.name;  // OK
}
...

While preparing the typings for babel-core, babel-types and friends, I noticed that using asserts this way is the norm. babel-types actually provides an assertXXX method for every isXXX method. These assertXXX functions are really just type guards that signal via return/throw rather than true/false.

Possible Solutions?

Not sure if it's feasible at all! But the new work on never in #8652 suggests a few possibilities.

Specific assertions: assertIsT(...)

// normal type guard
function isIdentifier(n: Node): n is Identifier {
    return n.type === 'Identifier';
}

// PROPOSED SYNTAX: assert type guard
function assertIdentifier(n: Node): n is Identifier | never {
    if (n.type !== 'Identifier') {
        throw new AssertionError(`Expected an Identifier`);
    }
}

The compiler would reason that if this assert call returns at all, then it can safely narrow the variable type in following code.

General assertions used with type guards: assert(isT(...))

The more general assert(cond: boolean) function would need a different approach and might not be feasible, but here's an idea:

    // General case
    declare function assert(cond: boolean): void;

    // PROPOSED SYNTAX: Special overload for expressions of the form assert(isT(x))
    declare function assert<T>(guard: guard is T): void | never;

For that second assert overload to work, the compiler on seeing assert(isT(x)) would have to somehow forward the x is T narrowing from the isT(x) expression to the assert(...) expression at compile-time.

Would be great if it also detected/handled things like assert(typeof x == 'string').

Not sure if any of this would meet the cost/benefit bar, but it's just an idea.

@chilloutman
Copy link

chilloutman commented Apr 24, 2017

I feel like this should work:

function assertIdentifier(n: Node): (n is Identifier) | never

But #5992 has made this impossible, by only allowing type predicates to be used as return types.

@yortus
Copy link
Contributor Author

yortus commented Apr 24, 2017

I like @isiahmeadows' idea in #12885. With that, assertIdentifier could be declared as two overloads:

declare function assertIdentifier(n: Node): n is Identifier;
declare function assertIdentifier(n: Node): never;

let x: Node = ...
assertIdentifier(x);
x; // CFA infers x is an Identifier here, since the other overload declares it never returns 

@dead-claudia
Copy link

@yortus That wouldn't work, because n is Identifier is just a boolean subtype that narrows within if-else when it's the raw operand. In fact, that'd work closer to @chilloutman's idea than you might expect.

Regarding the function overload idea, I've developed that idea further into something far more broadly useful: #13257

Here's how it'd apply here:

declare function assertIdentifier(n: Node): [
    case n is Identifier: void,
    default: throw,
];

(n.b. The default: throw is redundant, just provided here for clarity.)

@yortus
Copy link
Contributor Author

yortus commented Apr 25, 2017

@isiahmeadows right, my snippet above based on existing syntax (i.e. without constraint types) should be:

declare function assertIdentifier(n: Identifier): void;
declare function assertIdentifier(n: Node): never;

let x: Node = ...
assertIdentifier(x);
x; // CFA infers x is an Identifier here, since the other overload declares it never returns 

@johnemau
Copy link

Any updates on this? I would love to see the chai equivalent

interface myTypeA {
   typeGaurd: "myTypeA";
   myValue: Boolean;
}

interface myTypeB {
   typeGaurd: "myTypeB";
}

// ...

const myObj: myTypeA | myTypeB = getMyObj();

expect(myObj.typeGaurd).to.equal("myType");  // type guard assert
expect(myObj.myValue).to.be.true;            // No error :)

@dead-claudia
Copy link

@johnemau Chai's type definitions currently suck as-is, and they really need rewritten to not use any so much. Twice, now, I migrated a TS project's tests to clean-assert (shameless plug: I wrote it), which has sane type definitions, and it took a solid half a week to fix all the resulting type errors in it. (It had a few thousand tests, and some pretty complex types, too.)

So that's equally a failing of that library, and you wouldn't see results until that's fixed.

@dead-claudia
Copy link

As for this request, here's my thought of what a proposal could look like:

  1. This return-specific syntax would be better IMHO: assume n is Identifier, .... This allows multiple variables to be checked simultaneously.
  2. A function with an assume return type must not return a value.
  3. If a function is otherwise inferred as returning void and any argument is narrowed to only one type at all return points, infer the return type to assume the argument is that type, rather than void.
  4. The return type should be treated as an unconditional type narrowing in the body.

@dead-claudia
Copy link

Edit: assume types really should look like this, with a few clarifications/edits:

  • Syntax: assume T if n is Identifier, ...
    • T is any valid return type
    • Stuff like assume n is string if n is string and similar are equivalent to true.
  • Flagged: If a function has an inferred return type, then the inferred return type also assumes any arguments narrowed if they are narrowed at all return points.
  • Normal function return rules apply as if the enclosing assume didn't exist

@alangpierce
Copy link
Contributor

For me, the most important aspect here is for there to be some way to write a generic assert function and similar functions, not just type assertions specifically.

I'm starting to move my team's large JS codebase to TypeScript, and we have an assert function that we use regularly for things like null checks. We also have an assertAndContinue function that crashes in development and returns false and logs an error in production when the condition fails.

Concrete examples of where TypeScript could do better:

assert(this.props.synthesisRulesRun != null);
const violations = cutSiteViolations(this.props.synthesisRulesRun, enzyme);
if (!assertAndContinue(field.requiredLink, `Field with id=${field.id} has no requiredLink`)) {
  return [];
}
const linkType = field.requiredLink.sampleType;

Both of these could be solved with the ! non-null assertion operator, and most other examples I could find were just null checks like these, but it still would be nice if people on my team could feel like they aren't losing anything by using assert or assertAndContinue rather than writing extra code inline.

@Pauan
Copy link

Pauan commented Mar 16, 2018

@alangpierce In our big TypeScript project we use the following helper:

export function assertExists<A>(value: A | null | undefined): A {
    if (value != null) {
        return value;

    } else {
        throw new Error("Value doesn't exist");
    }
}

You can use it like this:

const synthesisRulesRun = assertExists(this.props.synthesisRulesRun);
const violations = cutSiteViolations(synthesisRulesRun, enzyme);

It would be ideal for TypeScript to have better support for generic assert, but the above is a good workaround in the meantime.

@csvn
Copy link

csvn commented Aug 13, 2018

It's a shame that return types from methods can't be used for performance reasons. Having throw expressions still require two separate methods if both validation and error message is not triial and needs to be repeated:

if (!isCircle(x)) throw incorrectTypeError(x);
assertIsCircle(x);

But I understand that the trade-off might not be worth it.

@nrkn
Copy link

nrkn commented Nov 20, 2018

This would be wonderful - at the moment I'm doing something like this with JSON imports:

import { Foo, Bar } from './types.ts'
import { assertFoo, assertBar } from './assert.ts'
import * as fooJson from './foo.json'
import * as barJson from './bar.json'

assertFoo( fooJson )
assertBar( barJson )

export const foo = <Foo>fooJson
export const bar = <Bar>barJson

But it would be great if I could just do this, and the exported types would be Foo and Bar because of the assert functions:

import { assertFoo, assertBar } from './assert.ts'
import * as foo from './foo.json'
import * as bar from './bar.json'

assertFoo( foo )
assertBar( bar )

export { foo, bar }

@ikokostya
Copy link
Contributor

ikokostya commented Nov 21, 2018

Response to #8655 (comment)

https://github.com/tc39/proposal-throw-expressions is actually an alternate solution here as well - once that proposal is through, code like (typeof x === "number") || throw "Wrong type");

This is not alternate solution. For example, in Node.js process.exit() call doesn't throw error, but terminates current execution. In addition, with throw expressions syntax still need to repeat conditional expression and error message in every place of usage.

If control flow analysis already understands user defined type guards, why do not add another special form for assert like functions?

@markspolakovs
Copy link

Slightly out-there suggestion from someone not experienced with TS internals, but if the problem is that the graph is built with syntax, how about adding an assert statement, similar to Python's? Something like

assert typeof s === "string";
console.log(s.length);

@kitsonk
Copy link
Contributor

kitsonk commented Jan 27, 2019

That is counter to the design goals of TypeScript:

  1. Avoid adding expression-level syntax.

@ziriax
Copy link

ziriax commented Jan 29, 2019

@kitsonk too bad about that number 8, otherwise just adding an inline keyword would solve this issue no? Then the compiler would just expand the type assertion as if it was written inline by the programmer, and everything would work.

@kuceb
Copy link

kuceb commented Mar 9, 2019

You could make an identity function with a built in assertion, if you're willing to re-assign the variable.

const foo = (a: number | null) => {
  a = shouldBe(_.isNumber, a)
  a  //  a is number
}

const shouldBe = <T>(fn: (t1) => t1 is T, t) => (fn(t) ? t : throwError(fn, t))

const throwError = (fn:Function, t) => {
  throw new Error(`not valid, ${fn.name} failed on ${t}`)
}

where _.isNumber has a type guard x is number

@ahejlsberg
Copy link
Member

Implementation now available in #32695.

@dblock
Copy link

dblock commented Aug 7, 2019

Being fairly new to TypeScript I explored this problem a bit from the expect syntax perspective. Thanks @orta for pointing me to this issue. Watching.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed Fix Available A PR has been opened for this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.