Function body is not checked against assertion signature #33743

Activity

1. andersk commented on Oct 2, 2019

   

   Anders Kaseorg (andersk)

   on Oct 2, 2019

   ContributorAuthor

   More actions

   FTR, the existing type guards feature suffers from the same soundness hole (which, as far as I can tell, isn’t called out at all in the documentation!):

   function isString(x: number | string): x is string {
       return true;
   }

   but I’m filing this against the new feature in the hope that there’s still time to give this more thought before it becomes stable. I can file another report for type guards if desired.

   Perhaps these unchecked features were justified with the rationale of allowing one to explain invariants that the checker wouldn’t derive by itself. But I would argue that they are also useful for encapsulating checks that the checker would derive if they weren’t encapsulated, and that these purposes should be separate: it should be possible to encapsulate checks without bypassing the type system.

   If one does need to bypass the type system, one can already do it explicitly with casts. Bypassing the type system should be explicit.

   const assertIsString: (x: number | string) => asserts x is string =
     ((x: number | string): void => {}) as
     (x: number | string) => asserts x is string;

   👍React with 👍4Reacted by arnold-graf, Linus Unnebäck, Greg Price and Boris Cherny
2. Ryan Cavanaugh (RyanCavanaugh)

   added

   Needs ProposalThis issue needs a plan that clarifies the finer details of how it could be implemented.This issue needs a plan that clarifies the finer details of how it could be implemented.

   SuggestionAn idea for TypeScriptAn idea for TypeScript

   on Oct 2, 2019
3. RyanCavanaugh commented on Oct 2, 2019

   

   Ryan Cavanaugh (RyanCavanaugh)

   on Oct 2, 2019

   Last edited by RyanCavanaugh

   Member

   More actions

   It's unclear how we'd do this in a way that would actually be desirable. There are a lot of ways you could cause an error and a lot of ways you could plausibly check for a type; at the point that you're writing an assertion function it's not like you could just plum forget to add a body. And if you did write a body that correctly asserted the value, but TypeScript didn't believe you for whatever reason, how would you convince it otherwise (if it was a function declaration)? You can't type-assert the control flow graph.

   e.g. a short and entirely plausible implementation of this function would be

   function assertIsString(x: number | string): asserts x is string {
     (x as any).substr()
   }

   which we have no hope of correctly analyzing.
4. andersk commented on Oct 2, 2019

   

   Anders Kaseorg (andersk)

   on Oct 2, 2019

   Last edited by andersk

   ContributorAuthor

   More actions

   There are a lot of ways you could cause an error and a lot of ways you could plausibly check for a type;

   TypeScript already understands most of them. I just want it to apply its existing knowledge instead of ignoring it. This, for example, should just work:

   function assertIsString(x: number | string): asserts x is string {
       if (typeof x === "number")
           throw new Error("x is a number, not a string");
   }

   since TypeScript already understands that x has type string in any execution path where this function returns normally.

   For the ones it doesn’t understand:

   And if you did write a body that correctly asserted the value, but TypeScript didn't believe you for whatever reason, how would you convince it otherwise (if it was a function declaration)?

   Like I suggested above:

   const assertIsString: (x: number | string) => asserts x is string =
     ((x: number | string): void => {
       (x as any).substr()
     }) as
     (x: number | string) => asserts x is string;

   It’s not pretty (which is perhaps a feature rather than a bug!), but it only needs to be done once. You can write this helper:

   const uncheckedTypeAssert: <T>(x: unknown) => asserts x is T =
     ((x: unknown): void => {}) as
     <T>(x: unknown) => asserts x is T;

   that can be reused wherever you need an unchecked type assertion:

   function assertIsString(x: number | string): asserts x is string {
     (x as any).substr();
     uncheckedTypeAssert<string>(x);
   }

   Or, if we wanted to add a prettier syntax, how about one that reuses the familiar principle that casts are unchecked:

   function assertIsString(x: number | string): asserts x is string {
     (x as any).substr();
     void 0 as asserts x is string;
   }

   or just

   function assertIsString(x: number | string): asserts x is string {
     (x as any).substr() as asserts x is string;
   }
5. gnprice commented on Oct 2, 2019

   

   Greg Price (gnprice)

   on Oct 2, 2019

   More actions

   at the point that you're writing an assertion function it's not like you could just plum forget to add a body.

   In practice, the bug that I'd expect to happen here is:

   • You write an assertion function that's correct.
   • A month later, someone extends one of the types involved in a way that makes the assertion function incorrect.

   From my experience with large codebases that lack type-checking, this doesn't seem unlikely at all -- on the contrary, it feels like exactly the kind of bug that untyped codebases tend to be full of.

   So I'd suggest reading Anders Kaseorg (@andersk)'s example with the empty body as a boiled-down stand-in for a lengthier example where the body looks plausible, but is missing a case.
6. gnprice commented on Oct 3, 2019

   

   Greg Price (gnprice)

   on Oct 3, 2019

   More actions

   A month later, someone extends one of the types involved in a way that makes the assertion function incorrect.

   Here's a fully-worked example of how that happens. Inevitably it's rather longer than the boiled-down example Anders Kaseorg (@andersk) gave at the top. I hope it's nevertheless helpful for priming one's intuition for how a bug of exactly this kind happens in a real codebase.

   The example starts from exactly the types the documentation uses as an example for type guards. I've just adapted the code slightly to use assertion functions instead.

   First, the types:

   interface Bird {
       fly();
       layEggs();
   }
   
   interface Fish {
       swim();
       layEggs();
   }
   
   type Pet = Bird | Fish
   

   These are verbatim from the docs' example, except I've added an alias for the union type that the example mentions several times.

   Now an assertion function, which is a direct translation of the type guard in the docs' example:

   function assertIsFish(pet: Pet): asserts pet is Fish {
       if ((pet as Fish).swim === undefined) {
           throw new Error("not a fish")
       }
   }
   

   Then here's an example of using the assertion function:

   function f() {
       // (... a bunch of app logic ...)
   
       // Because (... some invariant ...), this should in fact be a Fish.
       const fish = nextPet();
       // Assert that just to be sure.
       assertIsFish(fish);
   
       // Great, definitely a Fish.  We'll go do some delicate logic that
       // relies on the assumption this is a Fish.
       // (...)
       fish.swim();
       // (...)
       fish.layEggs();
       // (...)
   }
   

   At this stage, the assertion function is 100% correct, and all this code is quite reasonable.

   ---

   Now, someone comes along and adds another type of pet, extending the union:

   interface Amoeba {
       swim(): void;
       divide(): void;
   }
   
   type Pet = Bird | Fish | Amoeba
   

   This is a perfectly reasonable change. Naturally they go on to update all the code that needs to change to handle the new Amoeba case, and they lean on the type-checker to help them be sure they've covered all of it.

   They don't make any change to assertIsFish because they don't notice it -- it might be in a different file, or even a completely different project. And the type-checker doesn't give a peep about it.

   But, in fact, assertIsFish is now wrong! It'll accept a pet that's an Amoeba, as well as one that's a Fish.

   As a result, f can blow right past the assertIsFish call even though its fish isn't a fish at all, but an Amoeba. It'll run through the subsequent logic with that assumption violated, and e.g. blow up with a runtime type error at fish.layEggs().

   Here's fully stitched-together versions of this example on the playground: before; after.

   ---

   Unlike the Amoeba author, the type-checker looks at the definition of assertIsFish. And it has all the information it needs to see that the assertion function is now wrong -- it's not fundamentally different from the type checking that it does everywhere else. So it should be able to spot this bug in a straightforward assertion function like this one.

   OTOH if I do write an assertion function in a way the type-checker doesn't understand, then as Anders Kaseorg (@andersk) says, the bypass should be something explicit like a cast.

   PS: One might notice in this example that things only actually go wrong once there's also a bug somewhere to break the invariant that the fish in f will actually be a Fish. But that's just the nature of assertions -- they only do anything when there's already a bug somewhere -- so it doesn't really mitigate this issue. And in fact an analogous example with a type guard instead (if (isFish(pet)) fish.layEggs();) will already go wrong once Amoeba is added, with no exogenous bug involved at all.