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, 9/20/2019 #34572

Closed
DanielRosenwasser opened this issue Oct 18, 2019 · 0 comments
Closed

Design Meeting Notes, 9/20/2019 #34572

DanielRosenwasser opened this issue Oct 18, 2019 · 0 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Control Flow Updates

Assertions in Control Flow

#32695

  • Now it correctly reasons about exhaustive switch statements.
    • We used to detect this only syntactically - if you have a switch as the last statement, we would remove undefined from the return type.
    • But that wasn't sufficient. People kept complaining that it didn't catch specialized cases.
    • Leveraging the general Control Flow Analysis (CFA) logic is really the best way to do this.
    • This also means we can do well on exhaustive if statements.
    • function f44(x: "a" | "b") {
          if (someCondition()) {
              switch (x) {
                  case "a": return 100;
                  case "b": return 200;
              }
              // now considered unreachable,
              // return type doesn't include 'undefined' anymore.
              x;
          }
          throw "oops"
      }
  • Caveat: by adding this "smarter" logic, we consider code that's meant to be defensive against unchecked JS code to be "unreachable".
    • function f46(x: "a" | "b") {
          switch (x) {
              case "a": return 100;
              case "b": return 200;
          }
          // now considered unreachable - but
          // a JS user could've passed in "c"!
          throw "oops"
      }
    • The way to fix this is to add a default to the switch/statement.
    •  function f46(x: "a" | "b") {
           switch (x) {
               case "a": return 100;
               case "b": return 200;
      +        default: // empty default
           }
           // now considered unreachable - but
           // a JS user could've passed in "c"!
           throw "oops"
       }
  • Does add about 2% overhead on the checker... are committed to optimizing it.

Improved Caching in CFA

#33510

  • Wins back some perf.
    • Reduces check time by 1-2% in larger project.
  • A while back, we added a class of caching for class of code that does thousands and thousands of assignments with arithmetic.
    • If you need to re-evaluate the types at each assignment site, then this blows up the control flow graph and you have to traverse up the graph each time.
    • So we added caching.
  • This change makes it so that we only cache when we hit re-entrancy because it's usually not necessary to begin with.
  • Does this mean that the lookup of every narrowed type gets invoked at least twice?
    • [thinks] No?
  • Does this mean that we only cache every other node?
    • Maybe. It sounds like that may or may not be the intended effect of the code.
  • Caching for control flow paths that precede loops.
    • Also find a way to cache when re-entrancy before a loop.

Public Class Field Semantics Update

#33509

  • Class fields might be changing to [[Define]] semantics.
  • This is different from what TypeScript does, and we're trying to mitigate issues for existing code.
  • In 3.6, we allowed ambient classes (classes with declare or in .d.ts files) to have accessors.
    • Allows us to recognize that a property came from a getter/setter.
  • In 3.7:
    • Emit accessors in .d.ts files.
    • New syntax to say a property declaration has no emit.
    • Provide a flag to emit [[Set]] semantics.
      • legacyClassFields to opt in to old behavior.
      • true by default in 3.7
    • Error on overrides that would be problems with [[Define]].
  • What breaks?
    • Disallow accessor/property overrides.
      • Properties can only override properties.
      • Accessors can only override accessors.
      • Only exception is when the base declaration is abstract or comes from an interface.
      • Should we provide a codefix for it?
        • But it's not semantics-preserving.
  • Property overrides accessors
    • Before
      class Base {
          _p: unknown;
          get p() {
              return _p;
          }
          set p(v: unknown) {
              this._p = v;
          }
      }
      class Derived extends Base {
          p = 10;
      }
    • Before
      class Base {
          _p: unknown;
          get p() {
              return _p;
          }
          set p(v: unknown) {
              this._p = v;
          }
      }
      class Derived extends Base {
          declare p: number;
          constructor() {
              this.p = 10;
          }
      }
  • Ran the changes on DefinitelyTyped, our test suites, asked Googlers to try it out.
    • Property overrides accessor
      • Angular 2 - looks like you can move to a constructor assignment.
    • Accessor overrides property
      • VS Code and Azure SDK ran into something here.
  • We could potentially remove some initialization code if we can determine there are no ways to "witness" the values.
  • Are we going to change our emit for methods to reflect enumerability?
    • This feels like the only point we can change class members.
  • The emit here does feel bad.
    • We have opinionated defaults for downlevel iteration.
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

1 participant