TypeScript types: add `asserts` and return `never` from relevant assertions

bitjson commented 4 years ago

As of TypeScript 3.7, it's now possible to type assertions using the new asserts syntax. It would be great if AVA's assertions used this syntax where possible.

For example:

  const result = { success: true, value: 2 } as
    | { success: true; value: number }
    | { success: false; error: string };

  t.true(result.success);

  const sum = result.value + 1;

This produces the error:

Property 'value' does not exist on type '{ success: true; value: number; } | { success: false; error: string; }'.
  Property 'value' does not exist on type '{ success: false; error: string; }'.ts(2339)

If the t.true method had the following signature:

(actual: any, message?: string): asserts actual is true;

The example would not produce an error. With the current types, I have to do something like:

  const result = { success: true, value: 2 } as
    | { success: true; value: number }
    | { success: false; error: string };

  if(!result.success){
    t.fail();
    return; // required to avoid type error
  }

  const sum = result.value + 1;

Note also the extra required return – this can also be solved with a never-returning function, as is now the case with process.exit().

bitjson commented 4 years ago

Ah, I missed these related issues: https://github.com/avajs/ava/issues/2411, https://github.com/avajs/ava/issues/2330

Both issues are focused on adding the asserts types, which is made more complicated by this error:

test('test title', (t) => {
  const result = { success: true, value: 2 } as
    | { success: true; value: number }
    | { success: false; error: string };

  t.true(result.success); // Assertions require every name in the call target to be declared with an explicit type annotation.ts(2775)
}

This can be fixed by declaring the type of t:

test('test title', (t: ExecutionContext) => { 
  const result = { success: true, value: 2 } as
    | { success: true; value: number }
    | { success: false; error: string };

  t.true(result.success);

  const sum = result.value + 1;

}

As I understand it, this is a fundamental limitation of the TypeScript compiler, and assertions with inferred types are not likely to be implemented any time soon.

Unfortunately, most AVA users probably would have trouble figure out how the resolve the error (adding the : ExecutionContext when using t.true, t.false, or t.fail).

So I have 2 recommendations:

1. `t.fail` can return `never`

The TS compiler doesn't produce an error even if it's not able to compute how a never-returning function affects the control flow graph. So if the user leaves out the : ExecutionContext, it simply works as if the function returned void. (So it degrades gracefully.) E.g.:

// current usage
test('test title', (t) => {
  ... 
  if(condition){
    t.fail();
    return;
  }
}

// opt-in, better type-checking
test('test title', (t: ExecutionContext) => {
  ... 
 if(condition){
    t.fail();
    return; // Unreachable code detected.ts(7027)
  }
}

2. Expose strongly-typed `t.true` and `t.false` alternatives

Because adding the asserts type information to the existing t.true and t.false will produce a hard-to-understand error, AVA could expose something like t.trueAssert and t.falseAssert for users who would prefer to add the ExecutionContext type. E.g.:

// current usage
test('test title', (t) => {
  ... 
  t.true(result.success);
  if(!result.success){
    return;
  }
  const sum = result.value + 1;
  ...
}

// opt-in, better type-checking
test('test title', (t: ExecutionContext) => {
  ... 
  t.trueAssert(result.success);
  const sum = result.value + 1;
  ...
}

Thoughts on this proposal? And any ideas on better naming for the strongly-typed alternatives to t.true and t.false? (I think it's best if they begin with true and false respectively to improve discoverability in IDEs.)

novemberborn commented 4 years ago

Whilst I share your desire to use AVA's assertions for control flow analysis, I'm not sure if it's worth the hoops we'd have to jump through.

If you use t.context for instance, you need to specify its type in ExecutionContext<My_T_Context_Type>.

I don't think we need to add more assertion methods, either, that will break unless you've explicitly typed t.

I'm not sure why this is useful, but maybe it's a contrived example:

// opt-in, better type-checking
test('test title', (t: ExecutionContext) => {
  ... 
 if(condition){
    t.fail();
    return; // Unreachable code detected.ts(7027)
  }
}

Also it's not unreachable… assertions can fail the test but they don't stop the test from executing. So you might very well want that return if you're doing something "expensive" after.

earthlyreason commented 4 years ago

@bitjson to emphasize that last point:

it's not unreachable… assertions can fail the test but they don't stop the test from executing

This is something I also didn't realize when filing #2411. The assert-like functions on t do not in fact assert in the way meant by TypeScript's asserts keyword.

(Edit: Unless you are using the "fail fast" option. But you cannot tell that from the code itself.)

novemberborn commented 4 years ago

Note that the assertion doesn't actually have to throw for TypeScript to infer a type. E.g. given the proper annotations, this should work just fine:

type Discriminated = { kind: 'foo', foo: string } | { kind: 'bar', bar: string }

const value: Discriminated = getDiscriminated()
t.true(value.kind === 'foo')
t.is(value.foo, 'foo')

Unfortunately the necessary syntax may just be too awkward to recommend.

bitjson commented 4 years ago

This is something I also didn't realize when filing #2411. The assert-like functions on t do not in fact assert in the way meant by TypeScript's asserts keyword.

(Edit: Unless you are using the "fail fast" option. But you cannot tell that from the code itself.)

Ah! I've always used the failFast option, so I didn't realize that – thanks. So returning never isn't really correct without the failFast behavior.

I'm not sure why this is useful, but maybe it's a contrived example

@novemberborn yep, it's a little contrived. Here's a real example of the sort of discriminated type I'd love to be able to test without writing if statements. If your library returns a lot of discriminated types, your tests end up littered with if blocks just to fail and stop the test if the "success" type wasn't returned.

Anyway, now I understand that these types would rely on the failFast behavior. So I'm afraid any solution to this would require some meaningful API changes, rather than just updating types.

I suppose it might be possible for AVA to export a named export testStrict or something, where stricter type-checking is used (and failFast is required), but that's probably a lot of work to implement/document/maintain vs. the reward.

Since this doesn't seem actionable any time soon, this can probably be closed. Hopefully future TypeScript releases will make this easier.

novemberborn commented 4 years ago

This is something I also didn't realize when filing #2411. The assert-like functions on t do not in fact assert in the way meant by TypeScript's asserts keyword. (Edit: Unless you are using the "fail fast" option. But you cannot tell that from the code itself.)

Ah! I've always used the failFast option, so I didn't realize that – thanks. So returning never isn't really correct without the failFast behavior.

That's not quite right… --fail-fast just stops additional tests from starting, so that your ava invocation exits more quickly. Any test that is already running will complete, and failing assertions don't stop their test from completing either.

I appreciate this may be surprising, but so is interrupting the worker processes upon an error. It's a tough balance.

I wonder if we're going about this the wrong way. AVA calls these functions assertions, and TypeScript now has an asserts keyword… so they must be the same, right?! 😄

Here's an attempt at a discriminate assertion which would return a boolean. You need to provide generics, but then it does seem usable:

interface Assertions {
    discriminate <T, R extends T = T> (value: T, key: keyof T, comparator: R[typeof key]): value is R
}

Playground link

What do you think?

novemberborn commented 4 years ago

Building on this, if we were to return booleans for whether an assertion passed, we could turn some of them into type guards.

bitjson commented 4 years ago

@novemberborn that looks very promising! And also agreed on assertions returning booleans, that would make some of my tests much more fluent 💯

novemberborn commented 4 years ago

OK see these three issues:

osdiab commented 2 years ago

Revisiting this, I ended up working around it by defining my own wrappers on ava assertions like so:

/**
 * Wrapper on ava `t.is()` that applies a TypeScript type assertion, to allow for narrowing
 */
export function assertIs<Ctx, A, B extends A>(
  t: ExecutionContext<Ctx>,
  a: A,
  b: B,
  message?: string
): asserts a is B {
  t.is(a, b, message);
}

/**
 * Wrapper on ava `t.not()` that applies a TypeScript type assertion, to allow for narrowing
 */
export function assertNot<Ctx, A, B extends A>(
  t: ExecutionContext<Ctx>,
  a: A,
  b: B,
  message?: string
): asserts a is Exclude<A, B> {
  t.not(a, b, message);
}

If that helps anyone :)

avajs / ava