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

Assertions in control flow analysis #32695

Merged
merged 44 commits into from Sep 23, 2019
Merged

Assertions in control flow analysis #32695

merged 44 commits into from Sep 23, 2019

Conversation

@ahejlsberg
Copy link
Member

ahejlsberg commented Aug 3, 2019

With this PR we reflect the effects of calls to assert(...) functions and never-returning functions in control flow analysis. We also improve analysis of the effects of exhaustive switch statements, and report unreachable code errors for statements that follow calls to never-returning functions or exhaustive switch statements that return or throw in all cases.

The PR introduces a new asserts modifier that can be used in type predicates:

declare function assert(value: unknown): asserts value;
declare function assertIsArrayOfStrings(obj: unknown): asserts obj is string[];
declare function assertNonNull<T>(obj: T): asserts obj is NonNullable<T>;

An asserts return type predicate indicates that the function returns only when the assertion holds and otherwise throws an exception. Specifically, the assert x form indicates that the function returns only when x is truthy, and the assert x is T form indicates that the function returns only when x is of type T. An asserts return type predicate implies that the returned value is of type void, and there is no provision for returning values of other types.

The effects of calls to functions with asserts type predicates are reflected in control flow analysis. For example:

function f1(x: unknown) {
    assert(typeof x === "string");
    return x.length;  // x has type string here
}

function f2(x: unknown) {
    assertIsArrayOfStrings(x);
    return x[0].length;  // x has type string[] here
}

function f3(x: string | undefined) {
    assertNonNull(x);
    return x.length;  // x has type string here
}

From a control flow analysis perspective, a call to a function with an asserts x return type is equivalent to an if statement that throws when x is falsy. For example, the control flow of f1 above is analyzed equivalently to

function f1(x: unknown) {
    if (!(typeof x === "string")) {
        throw ...;
    }
    return x.length;  // x has type string here
}

Similarly, a call to a function with an asserts x is T return type is equivalent to an if statement that throws when a call to a function with an x is T return type returns false. In other words, given

declare function isArrayOfStrings(obj: unknown): obj is string[];

the control flow of f2 above is analyzed equivalently to

function f2(x: unknown) {
    if (!isArrayOfStrings(x)) {
        throw ...;
    }
    return x[0].length;  // x has type string[] here
}

Effectively, assertIsArrayOfStrings(x) is just shorthand for assert(isArrayOfStrings(x)).

In addition to support for asserts, we now reflect effects of calls to never-returning functions in control flow analysis.

function fail(message?: string): never {
    throw new Error(message);
}

function f3(x: string | undefined) {
    if (x === undefined) fail("undefined argument");
    x.length;  // Type narrowed to string
}

function f4(x: number): number {
    if (x >= 0) return x;
    fail("negative number");
}

function f5(x: number): number {
    if (x >= 0) return x;
    fail("negative number");
    x;  // Unreachable code error
}

Note that f4 is considered to not have an implicit return that contributes undefined to the return value. Without the call to fail an error would have been reported.

A function call is analyzed as an assertion call or never-returning call when

  • the call occurs as a top-level expression statement, and
  • the call specifies a single identifier or a dotted sequence of identifiers for the function name, and
  • each identifier in the function name references an entity with an explicit type, and
  • the function name resolves to a function type with an asserts return type or an explicit never return type annotation.

An entity is considered to have an explicit type when it is declared as a function, method, class or namespace, or as a variable, parameter or property with an explicit type annotation. (This particular rule exists so that control flow analysis of potential assertion calls doesn't circularly trigger further analysis.)

EDIT: Updated to include effects of calls to never-returning functions.

Fixes #8655.
Fixes #11572.
Fixes #12668.
Fixes #13241.
Fixes #18362.
Fixes #20409.
Fixes #20823.
Fixes #22470.
Fixes #27909.
Fixes #27388.
Fixes #30000.

@acutmore

This comment has been minimized.

Copy link

acutmore commented Aug 3, 2019

Really like this!

Curious if instead of asserts x it was considered to special case asserts x is true? Might be easier for people to learn/read for the cost of more complexity in the compiler

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 3, 2019

Curious if instead of asserts x it was considered to special case asserts x is true?

No, because the two are not equivalent. asserts x reflects the full effects of a logical expression when x is truthy, similar to an equivalent if statement. assert x is true simply narrows the type of a variable passed for x, similar to the effects of passing x to an equivalent user defined type predicate function.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 3, 2019

@typescript-bot perf test this

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Aug 3, 2019

Heya @ahejlsberg, I've started to run the perf test suite on this PR at fe70a62. You can monitor the build here. It should now contribute to this PR's status checks.

Update: The results are in!

@dragomirtitian

This comment has been minimized.

Copy link
Contributor

dragomirtitian commented Aug 3, 2019

@ahejlsberg Just curios, the official position when multiple such issues were raised was that adding all potential call expressions will grow the CF graph to much and thus it was not really feasible to add this feature. My question is what changed ? Was the reasoning flawed, other performance improvements now make this less of a perf concern, or this is still experimental and could still be axed if performance does meet expectations ?

@acutmore

This comment has been minimized.

Copy link

acutmore commented Aug 3, 2019

Ah! So asserts x declares it’s checking ‘truthy' rather than ‘true’

declare function assert(x): asserts x;
declare const x: string | null;
assert(x);
x.length; // x narrowed to string
@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 3, 2019

Ah! So asserts x declares it’s checking ‘truthy' rather than ‘true’

It's not just that.

asserts x reflects the full effects of the logical expression passed as an argument. E.g. assert(typeof x === "string" || typeof x === "number") narrows x to string | number in the following statements.

assert x is true however only affects a variable passed as an argument, i.e. assertIsTrue(x) narrows x to type true in the following, but does not reflect the effects of a logical expression passed as an argument.

@typescript-bot

This comment has been minimized.

Copy link
Collaborator

typescript-bot commented Aug 3, 2019

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..32695

Metric master 32695 Delta Best Worst
Angular - node (v12.1.0, x64)
Memory used 325,416k (± 0.03%) 325,819k (± 0.02%) +403k (+ 0.12%) 325,735k 326,007k
Parse Time 1.44s (± 0.91%) 1.43s (± 0.74%) -0.01s (- 0.97%) 1.41s 1.45s
Bind Time 0.76s (± 0.78%) 0.77s (± 1.49%) +0.00s (+ 0.52%) 0.75s 0.81s
Check Time 4.22s (± 0.44%) 4.27s (± 0.35%) +0.04s (+ 1.07%) 4.24s 4.32s
Emit Time 5.21s (± 0.49%) 5.27s (± 0.82%) +0.06s (+ 1.07%) 5.21s 5.39s
Total Time 11.64s (± 0.33%) 11.73s (± 0.48%) +0.09s (+ 0.78%) 11.65s 11.90s
Monaco - node (v12.1.0, x64)
Memory used 345,830k (± 0.02%) 346,156k (± 0.02%) +326k (+ 0.09%) 346,045k 346,293k
Parse Time 1.19s (± 0.64%) 1.17s (± 0.48%) -0.02s (- 1.52%) 1.16s 1.18s
Bind Time 0.67s (± 0.33%) 0.68s (± 0.54%) +0.00s (+ 0.75%) 0.67s 0.68s
Check Time 4.28s (± 0.44%) 4.29s (± 0.35%) +0.01s (+ 0.28%) 4.26s 4.32s
Emit Time 2.84s (± 0.67%) 2.85s (± 0.97%) +0.00s (+ 0.18%) 2.81s 2.93s
Total Time 8.97s (± 0.38%) 8.98s (± 0.28%) +0.01s (+ 0.09%) 8.94s 9.05s
TFS - node (v12.1.0, x64)
Memory used 301,335k (± 0.02%) 301,644k (± 0.02%) +310k (+ 0.10%) 301,532k 301,724k
Parse Time 0.92s (± 0.84%) 0.91s (± 0.92%) -0.02s (- 1.74%) 0.89s 0.92s
Bind Time 0.62s (± 1.08%) 0.63s (± 0.80%) +0.01s (+ 1.30%) 0.61s 0.63s
Check Time 3.83s (± 0.46%) 3.86s (± 0.54%) +0.03s (+ 0.89%) 3.79s 3.89s
Emit Time 2.94s (± 0.63%) 2.96s (± 0.76%) +0.02s (+ 0.75%) 2.89s 3.01s
Total Time 8.30s (± 0.34%) 8.34s (± 0.26%) +0.05s (+ 0.57%) 8.30s 8.38s
Angular - node (v8.9.0, x64)
Memory used 343,931k (± 0.01%) 344,376k (± 0.01%) +445k (+ 0.13%) 344,294k 344,499k
Parse Time 1.93s (± 0.34%) 1.83s (± 0.36%) -0.10s (- 5.12%) 1.82s 1.85s
Bind Time 0.82s (± 0.57%) 0.82s (± 0.91%) -0.00s (- 0.37%) 0.81s 0.84s
Check Time 5.07s (± 0.33%) 5.10s (± 0.81%) +0.03s (+ 0.61%) 5.03s 5.19s
Emit Time 6.08s (± 0.57%) 5.97s (± 1.60%) -0.11s (- 1.88%) 5.71s 6.12s
Total Time 13.90s (± 0.37%) 13.72s (± 0.70%) -0.18s (- 1.32%) 13.48s 13.86s
Monaco - node (v8.9.0, x64)
Memory used 363,317k (± 0.01%) 363,607k (± 0.01%) +290k (+ 0.08%) 363,514k 363,675k
Parse Time 1.52s (± 0.45%) 1.43s (± 0.31%) -0.09s (- 6.10%) 1.42s 1.44s
Bind Time 0.88s (± 0.39%) 0.88s (± 1.68%) +0.01s (+ 0.68%) 0.86s 0.92s
Check Time 5.28s (± 0.35%) 5.20s (± 1.37%) -0.08s (- 1.46%) 5.03s 5.30s
Emit Time 2.93s (± 0.37%) 3.14s (± 6.26%) +0.21s (+ 7.23%) 2.91s 3.49s
Total Time 10.61s (± 0.14%) 10.67s (± 1.38%) +0.05s (+ 0.50%) 10.49s 10.96s
TFS - node (v8.9.0, x64)
Memory used 317,282k (± 0.02%) 317,510k (± 0.01%) +228k (+ 0.07%) 317,443k 317,563k
Parse Time 1.23s (± 0.61%) 1.13s (± 0.33%) -0.10s (- 7.95%) 1.13s 1.14s
Bind Time 0.66s (± 0.74%) 0.67s (± 0.67%) +0.00s (+ 0.60%) 0.66s 0.68s
Check Time 4.47s (± 0.57%) 4.49s (± 0.42%) +0.02s (+ 0.54%) 4.45s 4.53s
Emit Time 3.05s (± 0.66%) 3.19s (± 1.73%) +0.13s (+ 4.35%) 3.05s 3.26s
Total Time 9.42s (± 0.33%) 9.48s (± 0.55%) +0.06s (+ 0.66%) 9.36s 9.58s
Angular - node (v8.9.0, x86)
Memory used 194,854k (± 0.02%) 195,000k (± 0.02%) +146k (+ 0.08%) 194,928k 195,056k
Parse Time 1.87s (± 0.46%) 1.78s (± 0.47%) -0.09s (- 4.76%) 1.76s 1.80s
Bind Time 0.95s (± 0.87%) 0.96s (± 0.73%) +0.01s (+ 0.52%) 0.94s 0.97s
Check Time 4.59s (± 0.72%) 4.65s (± 0.56%) +0.07s (+ 1.46%) 4.61s 4.72s
Emit Time 5.85s (± 0.87%) 5.78s (± 0.83%) -0.07s (- 1.20%) 5.67s 5.87s
Total Time 13.26s (± 0.54%) 13.17s (± 0.48%) -0.09s (- 0.65%) 13.03s 13.33s
Monaco - node (v8.9.0, x86)
Memory used 202,921k (± 0.01%) 203,068k (± 0.03%) +147k (+ 0.07%) 202,941k 203,192k
Parse Time 1.59s (± 0.56%) 1.48s (± 0.56%) -0.10s (- 6.37%) 1.47s 1.50s
Bind Time 0.70s (± 0.52%) 0.71s (± 0.56%) +0.01s (+ 0.99%) 0.70s 0.72s
Check Time 4.90s (± 0.44%) 4.90s (± 0.74%) +0.00s (+ 0.02%) 4.79s 4.96s
Emit Time 3.19s (± 0.76%) 3.18s (± 0.97%) -0.01s (- 0.38%) 3.13s 3.25s
Total Time 10.38s (± 0.30%) 10.28s (± 0.45%) -0.11s (- 1.02%) 10.14s 10.37s
TFS - node (v8.9.0, x86)
Memory used 178,237k (± 0.02%) 178,315k (± 0.02%) +78k (+ 0.04%) 178,185k 178,389k
Parse Time 1.30s (± 0.65%) 1.19s (± 0.93%) -0.11s (- 8.22%) 1.17s 1.23s
Bind Time 0.62s (± 1.42%) 0.64s (± 1.20%) +0.01s (+ 1.93%) 0.63s 0.66s
Check Time 4.28s (± 0.70%) 4.34s (± 0.92%) +0.06s (+ 1.28%) 4.27s 4.41s
Emit Time 2.88s (± 0.45%) 2.84s (± 2.22%) -0.04s (- 1.35%) 2.64s 2.93s
Total Time 9.09s (± 0.41%) 9.01s (± 0.81%) -0.08s (- 0.90%) 8.84s 9.16s
Angular - node (v9.0.0, x64)
Memory used 343,623k (± 0.01%) 343,948k (± 0.02%) +325k (+ 0.09%) 343,706k 344,059k
Parse Time 1.68s (± 0.49%) 1.67s (± 0.44%) -0.01s (- 0.42%) 1.66s 1.69s
Bind Time 0.77s (± 0.84%) 0.77s (± 0.68%) +0.00s (+ 0.13%) 0.76s 0.78s
Check Time 4.78s (± 0.51%) 4.83s (± 0.57%) +0.05s (+ 0.98%) 4.78s 4.91s
Emit Time 5.69s (± 1.81%) 5.48s (± 0.98%) -0.22s (- 3.85%) 5.37s 5.61s
Total Time 12.92s (± 0.87%) 12.74s (± 0.30%) -0.18s (- 1.37%) 12.68s 12.84s
Monaco - node (v9.0.0, x64)
Memory used 363,060k (± 0.03%) 363,350k (± 0.03%) +290k (+ 0.08%) 363,188k 363,663k
Parse Time 1.29s (± 0.90%) 1.27s (± 0.52%) -0.02s (- 1.24%) 1.26s 1.28s
Bind Time 0.85s (± 0.58%) 0.86s (± 0.88%) +0.00s (+ 0.23%) 0.84s 0.88s
Check Time 4.91s (± 0.45%) 4.91s (± 0.50%) -0.00s (- 0.02%) 4.85s 4.94s
Emit Time 3.36s (± 0.43%) 3.37s (± 0.34%) +0.00s (+ 0.12%) 3.35s 3.40s
Total Time 10.41s (± 0.36%) 10.40s (± 0.37%) -0.01s (- 0.09%) 10.32s 10.50s
TFS - node (v9.0.0, x64)
Memory used 317,006k (± 0.02%) 317,300k (± 0.02%) +293k (+ 0.09%) 317,182k 317,415k
Parse Time 1.02s (± 0.51%) 1.01s (± 0.47%) -0.01s (- 1.08%) 1.00s 1.02s
Bind Time 0.61s (± 0.97%) 0.61s (± 0.97%) +0.00s (+ 0.16%) 0.60s 0.63s
Check Time 4.34s (± 0.47%) 4.39s (± 0.33%) +0.05s (+ 1.13%) 4.35s 4.42s
Emit Time 3.20s (± 0.62%) 3.18s (± 0.48%) -0.01s (- 0.44%) 3.15s 3.21s
Total Time 9.18s (± 0.33%) 9.20s (± 0.27%) +0.02s (+ 0.20%) 9.14s 9.25s
Angular - node (v9.0.0, x86)
Memory used 194,883k (± 0.02%) 195,048k (± 0.04%) +165k (+ 0.08%) 194,887k 195,228k
Parse Time 1.60s (± 0.55%) 1.59s (± 0.37%) -0.01s (- 0.88%) 1.58s 1.60s
Bind Time 0.88s (± 0.53%) 0.89s (± 1.07%) +0.01s (+ 0.68%) 0.87s 0.91s
Check Time 4.30s (± 0.69%) 4.34s (± 0.75%) +0.04s (+ 0.91%) 4.29s 4.45s
Emit Time 5.53s (± 0.99%) 5.54s (± 0.71%) +0.01s (+ 0.22%) 5.47s 5.65s
Total Time 12.31s (± 0.59%) 12.35s (± 0.46%) +0.04s (+ 0.37%) 12.26s 12.47s
Monaco - node (v9.0.0, x86)
Memory used 202,929k (± 0.02%) 203,055k (± 0.03%) +126k (+ 0.06%) 202,917k 203,166k
Parse Time 1.32s (± 0.85%) 1.31s (± 0.67%) -0.01s (- 0.45%) 1.30s 1.34s
Bind Time 0.64s (± 0.62%) 0.65s (± 0.58%) +0.00s (+ 0.62%) 0.64s 0.65s
Check Time 4.70s (± 0.56%) 4.73s (± 0.71%) +0.03s (+ 0.68%) 4.67s 4.83s
Emit Time 3.09s (± 0.28%) 3.09s (± 0.56%) -0.00s (- 0.03%) 3.04s 3.12s
Total Time 9.75s (± 0.32%) 9.78s (± 0.42%) +0.03s (+ 0.28%) 9.70s 9.88s
TFS - node (v9.0.0, x86)
Memory used 178,233k (± 0.01%) 178,312k (± 0.03%) +79k (+ 0.04%) 178,185k 178,407k
Parse Time 1.04s (± 0.72%) 1.03s (± 0.48%) -0.01s (- 1.06%) 1.01s 1.03s
Bind Time 0.57s (± 1.19%) 0.57s (± 1.19%) +0.00s (+ 0.17%) 0.56s 0.59s
Check Time 4.12s (± 0.51%) 4.14s (± 0.60%) +0.02s (+ 0.39%) 4.08s 4.19s
Emit Time 2.79s (± 1.18%) 2.78s (± 1.13%) -0.01s (- 0.43%) 2.73s 2.88s
Total Time 8.53s (± 0.53%) 8.52s (± 0.50%) -0.00s (- 0.06%) 8.44s 8.63s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-142-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
  • node (v9.0.0, x64)
  • node (v9.0.0, x86)
Scenarios
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Angular - node (v9.0.0, x64)
  • Angular - node (v9.0.0, x86)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • Monaco - node (v9.0.0, x64)
  • Monaco - node (v9.0.0, x86)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • TFS - node (v9.0.0, x64)
  • TFS - node (v9.0.0, x86)
Benchmark Name Iterations
Current 32695 10
Baseline master 10

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 3, 2019

@dragomirtitian What changed? First realizing that the CFA node to AST node ratio is pretty low (about 10% for the compiler itself, for example), and further that we can restrict ourselves to only including top-level expression statement call nodes in the CFA graph. Again, using the compiler itself as an example, this PR only increases the number of CFA nodes by 7.5%. So, overall we're talking less than 1% of additional memory overhead. And execution time overhead is very low when CFA call nodes turn out to not be assertions.

The perf test bot numbers appear to confirm this. Less that 0.1% memory overhead and zero execution time overhead. If anything, I would actually have expected more impact.

I guess it's sometimes good to question conventional wisdom. Even when it's your own!

@acutmore

This comment has been minimized.

Copy link

acutmore commented Aug 3, 2019

asserts x reflects the full effects of the logical expression passed as an argument.

This I understand :-). What I am doing a poor job of expressing was that in my mind the reflection is a detail of the call site, and theoretically a function that asserts x is true could be completely obviously to this. Though the more I think about this, the more I can see how that would involve a lot of complexity. As it would almost be similar to supporting something like this:

declare const x: string | number;
const isString = typeof x === 'string'; // isString: (false & x is number) | (true & x is string);

if (isString) {
    x; // x: string;
}

So I retract all I have said, and have fully joined the asserts x fanclub!

@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Aug 3, 2019

Love this as it would make input validation (even against something like a JSON schema) a lot less clunky!

What makes me think though: Have you thought about expressing this with return types instead?

assertString<T>(value: T): T extends string ? void : never

Assertion functions are really just functions that throw errors in certain cases. A function returning never means it is always throwing. If a function returns never exactly when the input is a string (i.e. always throws when the input is a string), we know that after that call the value must be a string.

This was also suggested and upvoted in the issue: #8655 (comment)
I think if this feature can be expressed with existing syntax and concepts, adding more keywords and concepts to the language should be avoided (or TS will eventually become too complex).

The only thing a conditional never types cannot express is a manual type checking boolean expression:

assert(typeof x === 'string')

but I think that is actually a good thing. People should use specialized assertion functions, because they would throw an assertion error like

Expected type of value to be string, got number

instead of

Expected false to be true

which is not helpful. Plain assert() should always be avoided.

It also seems like asserts would not work with the popular expect() assertion style (used in Jest):

expect(someValue).toBeString()


function expect<T>(value: T): Matcher<T>
interface Matcher<T> {
  toBeString(): asserts ??? is string; // can't reference value here anymore
}

while that would work great with never return types:

function expect<T>(value: T): Matcher<T>
interface Matcher<T> {
  toBeString(): T extends string ? void : never;
}
Copy link
Contributor

ajafff left a comment

technically it's a breaking change, because the following code no longer parses without error (but what are the odds such code really exists?)

declare function f(asserts: unknown): asserts is string;
@@ -1276,6 +1284,22 @@ namespace ts {
activeLabels!.pop();
}

function isDottedName(node: Expression): boolean {

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

what's the difference to isEntityNameExpression?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Aug 3, 2019

Author Member

Good catch! No difference, will change to use the existing function.

@@ -1276,6 +1284,22 @@ namespace ts {
activeLabels!.pop();
}

function isDottedName(node: Expression): boolean {
return node.kind === SyntaxKind.Identifier || node.kind === SyntaxKind.PropertyAccessExpression && isDottedName((<PropertyAccessExpression>node).expression);

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

Is there a reason not to include this and super in property access expressions?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Aug 3, 2019

Author Member

I think that would be okay, but I'll have to convince myself it can't trigger circularities in control flow analysis.

node.assertsModifier = parseExpectedToken(SyntaxKind.AssertsKeyword);
node.parameterName = parseIdentifier();
if (parseOptional(SyntaxKind.IsKeyword)) {
node.type = parseType();

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

This makes this type of object polymorphic. Could you instead always assign the property and use undefined if there is no type?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Aug 3, 2019

Author Member

Yup

@@ -3225,6 +3228,16 @@ namespace ts {
}
}

function parseAssertsTypePredicate(): TypeNode {
const node = <TypePredicateNode>createNode(SyntaxKind.TypePredicate);
node.assertsModifier = parseExpectedToken(SyntaxKind.AssertsKeyword);

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

adding this property here and not assigning it in parseTypeOrTypePredicate where regular TypePredicate nodes are constructed, create yet another hidden class that hinders optimization at runtime.
Either assign it last in this function or (even better) assign it in both functions in the same order

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Aug 3, 2019

Author Member

Agreed

@@ -3225,6 +3228,16 @@ namespace ts {
}
}

function parseAssertsTypePredicate(): TypeNode {
const node = <TypePredicateNode>createNode(SyntaxKind.TypePredicate);
node.assertsModifier = parseExpectedToken(SyntaxKind.AssertsKeyword);

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

Is there a possibility that there will be more modifiers in the future? If so, would it make sense to put this into Node#modifiers?

This comment has been minimized.

Copy link
@ahejlsberg

ahejlsberg Aug 3, 2019

Author Member

It's possible, but for now I'm going to keep it the way it is.

@@ -667,17 +667,18 @@ namespace ts {
return <KeywordTypeNode>createSynthesizedNode(kind);
}

export function createTypePredicateNode(parameterName: Identifier | ThisTypeNode | string, type: TypeNode) {
export function createTypePredicateNode(assertsModifier: AssertsToken | undefined, parameterName: Identifier | ThisTypeNode | string, type: TypeNode | undefined) {

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

this is a breaking API change.
typically there is a new overload added to maintain backwards compatibility. the old signature can be marked as deprecated right away and could be removed later.

@@ -16845,23 +16820,62 @@ namespace ts {
return isLengthPushOrUnshift || isElementAssignment;
}

function maybeTypePredicateCall(node: CallExpression) {
function isDeclarationWithExplicitTypeAnnotation(declaration: Declaration | undefined) {

This comment has been minimized.

Copy link
@ajafff

ajafff Aug 3, 2019

Contributor

should this handle JSDoc as well?

@treybrisbane

This comment has been minimized.

Copy link

treybrisbane commented Aug 4, 2019

It looks as though this can be used to track mutations, e.g.:

class Foo {
  constructor(public bar: boolean) {}

  setBar<T extends boolean>(newBar: T): asserts this is Foo & { bar: T } {
    this.bar = newBar;
  }
}

const Foo = new Foo(false);
// foo is Foo
foo.setBar(true);
// foo is Foo & { bar: true }

Or

type Foo = { bar: boolean };

function setBar<T extends boolean>(foo: Foo, newBar: T): asserts foo is Foo & { bar: T } {
  foo.bar = newBar;
}

const foo: Foo = { bar: false };
// foo is Foo
setBar(foo, true);
// foo is Foo & { bar: true }

Is this correct?

@j-f1

This comment has been minimized.

Copy link

j-f1 commented Aug 4, 2019

Another advantage to using the never type instead as suggested above is that it would also add support for calling e.g. process.exit in a conditional to narrow the type.

@kitsonk kitsonk mentioned this pull request Aug 4, 2019
@zenozen

This comment has been minimized.

Copy link

zenozen commented Aug 4, 2019

Really nice!

Maybe we could use “asserts false” to represent a function that does not return? (Throws exception) This could help a bunch of case like assertNever, or unimplemented? Or maybe just “assert x is never” works?

@krzkaczor

This comment has been minimized.

Copy link

krzkaczor commented Aug 4, 2019

@ahejlsberg I have a couple of questions:

  1. Does it work with async assert as well? Ie. a function that depending on a condition resolves/rejects a promise.
  2. Does it work with assertions on this? I wonder if this can help implement linear types (related: #16148). My dummy example:
class Socket {
    public async open() asserts this is CloseableSocket {
        console.log("Opening...")
    }

    public async close() asserts this is OpenableSocket {
        console.log("Closing...")
    }
}

interface CloseableSocket{
    close() asserts this is OpenableSocket;
}

interface OpenableSocket{
    open() asserts this is CloseableSocket;
}

Now it would be impossible to call open on the already opened socket and close the already closed socket. This would be really cool to see!

@goodmind

This comment has been minimized.

Copy link

goodmind commented Aug 4, 2019

How to write invariant with it?

@jack-williams

This comment has been minimized.

Copy link
Collaborator

jack-williams commented Aug 4, 2019

@treybrisbane Your second example works, but your first does not because this is not supported in an assert predicate (not sure if that is by-design). So you can track mutations, but this only really works for monotonic references.

@krzkaczor Pre-emptive apology for the pedantry, sorry. What you implement there is known as type-state. Linear (or affine) types are required to soundly implement type-state, but that code does not actually guarantee there is only one reference to a given object. That still looks like an interesting use of this PR though, and if you assume that the user is careful with their aliasing you might be able to add a lot of type-safety.

@felixfbecker

The syntax:

assertString(value: unknown): value extends string ? void : never

also relies on new concepts, specifically having an expression (identifier) appearing in the check-type of a conditional type. On the surface I think it looks familiar to existing ideas, but there may be a non-trivial amount of new concepts required to implement and explain that feature thoroughly.

I think if you want meaningful assertion messages (which is definitely desirable), it could be written like:

function assertString(value: unknown): asserts value is string {
    if (typeof value !== "string") {
        throw "Expected 'string', got ${typeof value}";
    }
}
@felixfbecker

This comment has been minimized.

Copy link

felixfbecker commented Aug 4, 2019

@jack-williams sorry, updated my comment, what I meant was:

assertString<T>(value: T): T extends string ? void : never

which does not require any new concepts. In fact, I would argue, it is almost a bit unexpected that this does not work already, because the semantics of never would lead to this conclusion. TypeScript already infers the never type for functions that always throw, and flags unreachable code after the throw statement. One would think that the fact that the function returns never would also make TS flag code after a call of such function (but doesn't atm). Then by using conditional types we can intuitively model assertions.

@alexreardon

This comment has been minimized.

Copy link

alexreardon commented Aug 4, 2019

This is needed to correctly move tiny-invariant to typescript: alexreardon/tiny-invariant#45. We have not been able to write a correct typescript invariant

We also use invariant heavily for react-beautiful-dnd, so having this style of guard would making moving rbd over to Typescript much easier atlassian/react-beautiful-dnd#982

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 5, 2019

@felixfbecker The difference between the two forms

assertString(value: unknown): asserts value extends string;
assertString<T>(value: T): T extends string ? void : never

is that that we cannot necessarily make conclusions about an argument passed for value from a type argument for T. For example, imagine a type argument was explicitly specified for T, or that multiple parameters reference T, or that T is only referenced in a composite type and not as a naked type parameter. In those cases it is not meaningful to make conclusions for value and we would need rules to exclude them. Which ultimately leads you to the current form.

@ahejlsberg

This comment has been minimized.

Copy link
Member Author

ahejlsberg commented Aug 5, 2019

@typescript-bot perf test this again to observe effects of including this.xxx(...) calls in control flow graph.

@justingrant justingrant mentioned this pull request Oct 4, 2019
cilice added a commit to cilice/tiny-invariant that referenced this pull request Oct 6, 2019
With the coming TypeScript 3.7 release in November, there will be a new feature supporting assertions. See microsoft/TypeScript#32695 for more.

Signed-off-by: Alexander Plavinski <hello@cilice.me>
@cilice cilice mentioned this pull request Oct 6, 2019
kdesysadmin pushed a commit to KDE/syntax-highlighting that referenced this pull request Oct 10, 2019
Summary:
TypeScript:
* Add asserts: microsoft/TypeScript#32695
* Highlight types after `as` expression.
* Add type helpers: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-5.html#the-omit-helper-type

TypeScript React (TSX):
* When `<T extends` is detected, highlight as a type assertion, not as a Tag.
  See: microsoft/TypeScript-TmLanguage@11b1a4f

JavaScript React (JSX):
* Use non-capture groups in RegExpr rules.

Reviewers: #framework_syntax_highlighting, dhaumann, cullmann

Reviewed By: #framework_syntax_highlighting, cullmann

Subscribers: kwrite-devel, kde-frameworks-devel

Tags: #kate, #frameworks

Differential Revision: https://phabricator.kde.org/D24355
joelpurra added a commit to joelpurra/is that referenced this pull request Nov 8, 2019
Uses the typescript v3.7 feature `asserts value is T` to create `assert` variants of the `is` type guards. The assertions are used to narrow types at compile time, and to throw `TypeError` at runtime for values which are not of the correct type.

```typescript
import {assert} from '@sindresorhus/is';

assert.string(foo);
```

- Each method in `is` is wrapped and mirrored in `assert`.
- Tests for `is` are duplicated for `assert`.

Notes

- The explicit typing in `interface Assert` is required for typescript to acknowledge the assertions.
- Due the assertions requiring explicit typing, using ` property for is.assert.string()` (and so on) would require using `namespace is`, which was removed in sindresorhus#78.
- Custom descriptions are used to enhance some assertion error messages.
- Could perhaps use Node.js' `AssertionError` on the server-side, but am avoiding the `import`.

Fixes sindresorhus#91.

See

- sindresorhus#91
- https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions
- microsoft/TypeScript#32695
joelpurra added a commit to joelpurra/is that referenced this pull request Nov 8, 2019
Uses the typescript v3.7 feature `asserts value is T` to create `assert` variants of the `is` type guards. The assertions are used to narrow types at compile time, and to throw `TypeError` at runtime for values which are not of the correct type.

```typescript
import {assert} from '@sindresorhus/is';

assert.string(foo);
```

- Each method in `is` is wrapped and mirrored in `assert`.
- Tests for `is` are duplicated for `assert`.

Notes

- The explicit typing in `interface Assert` is required for typescript to acknowledge the assertions.
- Due to the assertions requiring explicit typing, using ` property for is.assert.string()` (and so on) would require using `namespace is`, which was removed in sindresorhus#78. This also means that `assert` needs to be exported separately.
- Custom descriptions are used to enhance some assertion error messages. The value is not included in the error message.
- Could perhaps use Node.js' fitting `AssertionError` on the server-side, but it would require an `import`.

Fixes sindresorhus#91.

See

- sindresorhus#91
- https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions
- microsoft/TypeScript#32695
joelpurra added a commit to joelpurra/is that referenced this pull request Nov 8, 2019
Uses the typescript v3.7 feature `asserts value is T` to create `assert` variants of the `is` type guards. The assertions are used to narrow types at compile time, and to throw `TypeError` at runtime for values which are not of the correct type.

```typescript
import {assert} from '@sindresorhus/is';

assert.string(foo);
```

- Each method in `is` is wrapped and mirrored in `assert`.
- Tests for `is` are duplicated for `assert`.

Notes

- The explicit typing in `interface Assert` is required for typescript to acknowledge the assertions.
- Due to the assertions requiring explicit typing, using ` property for is.assert.string()` (and so on) would require using `namespace is`, which was removed in sindresorhus#78. This also means that `assert` needs to be exported separately.
- Custom descriptions are used to enhance some assertion error messages. The value is not included in the error message.
- Could perhaps use Node.js' fitting `AssertionError` on the server-side, but it would require an `import`.

Fixes sindresorhus#91.

See

- sindresorhus#91
- https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions
- microsoft/TypeScript#32695
joelpurra added a commit to joelpurra/is that referenced this pull request Nov 8, 2019
Uses the typescript v3.7 feature `asserts value is T` to create `assert` variants of the `is` type guards. The assertions are used to narrow types at compile time, and to throw `TypeError` at runtime for values which are not of the correct type.

```typescript
import {assert} from '@sindresorhus/is';

assert.string(foo);
```

- Each method in `is` is wrapped and mirrored in `assert`.
- Tests for `is` are duplicated for `assert`.

Notes

- The explicit typing in `interface Assert` is required for typescript to acknowledge the assertions.
- Due to the assertions requiring explicit typing, using ` property for is.assert.string()` (and so on) would require using `namespace is`, which was removed in sindresorhus#78. This also means that `assert` needs to be exported separately.
- Custom descriptions are used to enhance some assertion error messages. The value is not included in the error message.
- Could perhaps use Node.js' fitting `AssertionError` on the server-side, but it would require an `import`.

Fixes sindresorhus#91.

See

- sindresorhus#91
- https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#assertion-functions
- microsoft/TypeScript#32695
@drew-gross

This comment has been minimized.

Copy link

drew-gross commented Nov 11, 2019

I read about this feature in the release notes and was excited to adopt it into my codebase, but it doesn't actually seem to help with the most common pattern I have. Was this feature intended to prevent errors in this snippet?

let alwaysThrow = (): never => {
    throw "always throw"
}

let whyNotOk = (a: string | number): undefined => {
    if (typeof a == 'string') alwaysThrow();
    a + 1;
    return undefined;
}

let ok = (a: string | number): undefined => {
    if (typeof a == 'string') return alwaysThrow();
    a + 1;
    return undefined;
}   

plauground

@MicahZoltu

This comment has been minimized.

Copy link
Contributor

MicahZoltu commented Nov 11, 2019

@drew-gross It appears it doesn't work with functions in variables. If you change your alwaysThrow to:

function alwaysThrow(): never { throw new Error() }

Then it will check correctly.

@AlCalzone

This comment has been minimized.

Copy link

AlCalzone commented Nov 11, 2019

@drew-gross @MicahZoltu
It works if you add a type annotation to the variable holding the function: playground

If I remember correctly, this is also mentioned in the PR and/or release notes.

@thealjey

This comment has been minimized.

Copy link

thealjey commented Nov 11, 2019

export function assertArray<T>(
  actual: T,
  message: string = "expected value to be an array"
): asserts actual is any[] {
  assert<T>(actual, isArray, message);
}

A type predicate's type must be assignable to its parameter's type.
Type 'any[]' is not assignable to type 'T'.
'any[]' is assignable to the constraint of type 'T', but 'T' could be instantiated with a different subtype of constraint '{}'.

How can I assert that something is an array of any?

In fact, asserts is just straight up does not work when actual is a generic type, regardless of what's on the right side of is.

@acutmore

This comment has been minimized.

Copy link

acutmore commented Nov 11, 2019

@thealjey

export function assertArray(
  actual: any,
  message: string = "expected value to be an array"
): asserts actual is any[] {
  assert<any>(actual, isArray, message);
}
@thealjey

This comment has been minimized.

Copy link

thealjey commented Nov 12, 2019

@acutmore
yes, like I said in my comment, it is because of the generic type
it does compile like that
but, it's like covering your eyes and pretending the problem does not exist
I'm just saying that this new feature, unlike what the documentation leads one to believe, is not up to par with the rest of the language when it comes to type inference

@zakm

This comment has been minimized.

Copy link

zakm commented Dec 4, 2019

Is it possible to use these assertions when type checking plain javascript files?

@Bnaya

This comment has been minimized.

Copy link

Bnaya commented Dec 4, 2019

@zakm

/**	
 * @template T	
 * @param {T} v	
 * @return {asserts v is NonNullable<T>}	
 */	
export function assertNonNull(v) {	
  if (v === undefined || v === null) {	
    throw new Error("assertNonNull");	
  }	
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.