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

Design Meeting Notes, 2/27/2024 #57568

Closed
DanielRosenwasser opened this issue Feb 27, 2024 · 4 comments
Closed

Design Meeting Notes, 2/27/2024 #57568

DanielRosenwasser opened this issue Feb 27, 2024 · 4 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 27, 2024

Triple-Slash Reference Emit/Preservation (motivated by --isolatedDeclarations)

#57440

  • One stance we had with --isolatedDeclarations is that the flag should not affect emit itself.
  • One question around this: what do we do with /// <reference > directives?
  • When doing declaration file emit, TypeScript will try to insert
  • Let's say you have a statement like export * as fs from "fs".
    • TypeScript will emit a triple-slash reference to say /// <reference types="node" />
  • One idea for --isolatedDeclarations is to say that when TypeScript would do this, it should emit an error. In other words, the user has to write it out explicitly.
  • Problem with this is that TypeScript occasionally removes these directives as well when they're not needed.
  • This all feels pretty complicated.
  • Suggestion: preserve triple-slash references when they're written, never generate them.
    • Thinking we could do this in 6.0.
  • The other option would be an attribute on /// references that allows people to control whether the reference is preserved, elided, or whatever.
  • Do we know how often we generate these in the top200 repos?
    • No
  • Until recently, preservation wasn't perfect anyway - so maybe people aren't relying on it?
  • One strong preference: what you see is what you get.
  • If you inserted one of these references, you had to make sure you had the right types installed. The difference is a UX issue of what makes the error more obvious.
  • Can view /// refs as a holdover from a time long-past. Don't ascribe them more value than they deserve!
  • What about the non-types references?
    • /// <reference types="..." />
    • /// <reference paths="..." />
  • Do we synthesize?
    • types: currently sometimes yes.
    • paths (aka file references): hopefully no?
  • Do we elide?
    • types: currently also sometimes yes.
    • paths (aka file references): hopefully no?
  • Do we generate /// <reference paths="..." />
  • So we want to make these changes. But when?
    • One idea floated was to make sure that this only kicks in within --isolatedDeclarations, and gets turned on for everyone in 6.0
  • If we're not going to synthesize references, it might be appropriate to tell library authors that they need to preserve that?
  • A little gun-shy on including this in 5.5.
    • --verbatimReferenceDirectives, require this in --isolatedDeclarations, change it in 6.0...
      • And then deprecate --verbatimReferenceDirectives in 6.0? 😂
    • We'd like to see how common this is.
  • Nightly?
    • Nobody is going to publish on nightly!
  • Conclusion: Let's try to create an error when synthesizing, see what breaks on the top 400 repos, and then decide if we need a new flag.

Inferring Type Predicate Signatures from Function Bodies

#57465

  • Motivating example.

    const arr = [1, 2, "a", "b", undefined];
    
    const numbers = arr.filter(x => typeof x === "number");
    const el1: number = numbers[0]
  • Today that errors because the filter doesn't work.

  • Solution is to explicitly make the signature a type predicate

    - const numbers = arr.filter(x => typeof x === "number");
    + const numbers = arr.filter((x): x is number => typeof x === "number");
  • PR makes it so these signatures can be automatically infer.

    • Functions
      • with no explicit return type
      • that return a boolean
      • with a single return expression.
  • Analysis cost?

    • Most repos had no perf cost.
    • But occasionally there was a cost.
      • Cost to detect the guards.
      • Cost to deal with the usage of type guards.
    • In experiments, most of the cost comes from detecting if functions are type guards.
    • More from the usage in type guards.
  • This has been a long-standing request - people really don't like that filter doesn't work.

  • What happens when multiple variables get narrowed?

    • Seems like it's a bail-out.
    • Good, that's what we want.
      • Also, the false branch of a (x is number AND y is number) can't guarantee that the variables aren't both number. Kind of hard to reason about.
  • What broke?

    • Inability to express comparability bounds:

      declare const cat: Cat;
      declare const arr: Animal[];
      
      const dogs = arr.filter(a => isDog(a));
      
      dogs.indexOf(cat);
    • Inlining in Pyright.

      • Notably, not a filter!
  • Could argue that so many libraries that have their own isNotNull etc. exist only because function expressions can't narrow.

  • In some ways this is way better because type predicates are not checked based on how you've narrowed! But inferring is actually way better!

    • ...should we make sure that they are?
    • We would like to. Gotta find a checking opt-out.
  • What are the downsides?

    • Lots of expressions simply won't get a type guard inferred.
      • "What about !!x?"
        • No helper for Truthy.
@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Feb 27, 2024
@danvk
Copy link
Contributor

danvk commented Feb 27, 2024

Lots of expressions simply won't get a type guard inferred.

  • "What about !!x?"

    • No helper for Truthy.

You can't safely make !!x a type predicate without #15048. If a type predicate could have an explicit else clause then we could infer lots of type predicates! Something like const isTruthy = (x: number|null): x is number else (number|null) => !!x.

@fatcerberus
Copy link

  • ...should we make sure that [type predicates] are [checked based on how you’ve narrowed]?
  • We would like to. Gotta fine a checking opt-out.

As was discussed in #57436, isn’t this impractical? If you’re writing an explicitly annotated type guard (as opposed to just letting it be inferred for simple cases e.g. in a .filter()), it feels like in 90% of cases it’s going to be doing a kind of narrowing that the compiler wouldn’t be able to verify anyway, so you’d just produce false positives

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Feb 28, 2024

The user journey we're concerned about is

  1. You write a function that's inferred to be a type guard
  2. Just to be explicit, or perhaps for isolatedDeclarations you add a type guard annotation.
  3. For some reason someone decides to perform an invalid internal refactoring that would have invalidated an inferred type guard, but not an explicitly written one.

@danvk
Copy link
Contributor

danvk commented Feb 28, 2024

The user journey we're concerned about is

  1. You write a function that's inferred to be a type guard
  2. Just to be explicit, or perhaps for isolatedDeclarations you add a type guard annotation.
  3. For some reason someone decides to perform an invalid internal refactoring that would have invalidated an inferred type guard, but not an explicitly written one.

That's definitely a failure mode, though to be fair it's also a failure mode with type guards today (minus step 1).

It's the same story as for annotating return types, except that annotated return types are checked whereas type predicates are not. This user journey would argue that maybe they should be. Except that would be a much bigger, breakier change. I'd like to at least quantify how many existing explicit type predicates wouldn't check, see #57465 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

4 participants