microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.63k stars 12.44k forks source link

(Almost) arbitrary functions are assignable to an assertion-function-typed variable #49262

Open jiftechnify opened 2 years ago

jiftechnify commented 2 years ago

Bug Report

🔎 Search Terms

assertion functions assignability inconsistent type guards

🕗 Version & Regression Information

⏯ Playground Link

Playground link with relevant code

💻 Code

const assertNumber = (x: unknown): asserts x is number => {
  if (typeof x !== 'number') {
    throw Error("not a number")
  }
}
// suspicious assignment...
const assertString: (x: unknown) => asserts x is string = assertNumber

const x: unknown = 100;
assertString(x); // Because `assertString` is actually `assertNumber`, no error is thrown
/*** x's type is now string ***/
x.substring(1); // no compile error, but runtime error!

const foo = (x: string | undefined): number => 100
// suspicious assignment...
const assertTruthy: (x: string | undefined) => asserts x = foo

const y: string | undefined = false ? 'a' : undefined;
assertTruthy(y);
y.substring(1); // no compile error, but runtime error!

🙁 Actual behavior

Both (suspicious) assignments succeed with no compile error. This behavior is not consistent with that of type guards.

🙂 Expected behavior

Both assignments should be rejected by the compiler.

I believe assignability rule for assertion functions should be consistent with the rule for type guards. At least, an assignment of a function which doesn't have assertion function signature to an assertion-function-typed variable should be rejected. Furthermore, for assertion function signatures with "result type" ((x: ...) => asserts x is T), it should be covariant with respect to the "result type".

RyanCavanaugh commented 2 years ago

At least, an assignment of a function which doesn't have assertion function signature to an assertion-function-typed variable should be rejected.

Original notes at #32695 don't mention this, but IIRC the motivating behavior for this logic was that we wanted a binding with an assertion function type to be intializable by a "normal" function, i.e.:

const assertNumber: (x: unknown) => asserts x is number = x => {
  if (typeof x !== 'number') {
    throw Error("not a number")
  }
}

here the return type of the function expression is just void, so the initialization is allegedly illegal under this rule unless we added new logic to e.g. contextually-modify the return type of the function expression.

Assigning something with two different asserted-types is super sketchy. We could potentially flag this, but I think the lived experience over the last three years is that aliasing assertion functions of any kind is extraordinarily rare since they don't have any meaningful substitutability or polymorphism.

bgourlie commented 2 years ago

aliasing assertion functions... don't have any meaningful substitutability or polymorphism.

I disagree and believe this is a legitimate soundness issue! Consider the following, which among few other things defines a generic array assertion function:

type AssertFn<T> = (val: unknown) => asserts val is T;

const num: AssertFn<number> = (val) => {
  if (!Number.isFinite(val)) {
    throw new Error('num assertion failed');
  }
}

const array: <T>(assert: AssertFn<T>) => AssertFn<T[]> =
  (assert) => (val) => {
    if (!Array.isArray(val)) {
      throw new Error('assertIsArray failed');
    }
    val.forEach(assert);
  };

const assertStringArray: AssertFn<string[]> = array(num);

Link to playground

Note that I explicitly type assertStringArray as AssertFn<string[]> but am actually supplying a AssertFn<number[]> via array(num), and the compiler is fine with this. If I remove the explicit type annotation, it correctly infers the type to AssertFn<number>.

acutmore commented 1 year ago

One potential reason why this could be particularly 'dangerous' is that it's required to specify the assertion types to ensure they impact CFA.

type TypePredicate<T> = (v: unknown) => v is T;
type TypeAssertion<T> = (v: unknown) => asserts v is T;

declare function makeAssertion<T>(predicate: TypePredicate<T>): TypeAssertion<T>;
declare function isString(v: unknown): v is string;
const assertIsString = makeAssertion(isString);
declare let x: unknown;
assertIsString(x); // TS error! Assertions require every name in the call target to be declared with an explicit type annotation
- const assertIsString = makeAssertion(isString);
+ const assertIsString: TypeAssertion<string> = makeAssertion(isString);
declare let x: unknown;
assertIsString(x);
x; // good, x is string

but if we put the wrong type...

- const assertIsString = makeAssertion(isString);
+ const assertIsString: TypeAssertion<number> = makeAssertion(isString);
declare let x: unknown;
assertIsString(x);
x; // bad, x is number

I was expecting const assertIsString: TypeAssertion<number> = makeAssertion(isString); to produce a type error.

One more example:

type TypeAssertion<T> = (v: unknown) => asserts v is T;

declare function sendRequest<ExpectedReponse>(
    path: string,
    assertion: TypeAssertion<ExpectedReponse>
): Promise<ExpectedReponse>;

interface Person { name: string };
type People = Array<Person>;

declare function assertPerson(v: unknown): asserts v is Person;

// @ts-expect-error
sendRequest<People>("/db", assertPerson);

playground

mwoenker commented 5 months ago

This behavior was very surprising to me. I've been working on a project that validated JSON passed to API endpoints using assertion functions. I was shocked to realize that the code actually had no type safety. No one else on the project realized it either, including those who wrote the original code. A very simplified version of the code in question looks like this:

type Validator<T> = (val: unknown) => asserts val is T
type Handler<T> = (request: T) => void;

function attachApiEndpoint<T>(path: string, handler: Handler<T>, assertValid: Validator<T>) {
  server.attach(path, (request) => { 
    assertValid(request.requestBody);
    handler(request.requestBody);
  })
}

const assertString: Validator<string> = (val: unknown): asserts val is string => {
  if (typeof val !== 'string') {
    throw new Error('not a string');
  };
}

attachApiEndpoint(
  '/path/to/endpoint',
  (body: number) => {
    // body will actually be a string here!
  },
  assertString // no type error prevents an assertion not matching the handler from being passed here
)