Function Expression Callback Analysis and deferred

#58729

• Story of adding /** @deferred */ to the Node.js .d.ts files node: Add deferred tag for ts5.6 DefinitelyTyped/DefinitelyTyped#70084
  • Mostly from @sandersn's perspective.
  • Asked: Is this only on parameters?
    • Yes.
  • Easy to add /** @deferred */ incorrectly to paramerters it doesn't really apply to. Silently does nothing.
    • Maybe fine?
  • Has to be exactly where the parameter is. Can't do it like @deferred @param {number} x */
  • 80% of the time, deferred needs to be added because a function is asynchronously called.
  • Had to ask: does "not guaranteed to be called" deferred?
    • Not necessarily.
    • Kind of a tripping point though.
  • deferred had to be sprinkled over so many signatures in Node.js.
    • Could use listener as a heuristic to figure out where to put it.
• Is assuming possibly-called and needing to annotated with deferred the right default?
  • So deferred vs. immediate.
  • In favor of possibly-called default.
    • Collection (array, map, set) callbacks never are deferred. Assuming possibly-called favors APIs like this.
    • Better to be more conservative on control flow analysis.
  • In favor of deferred by default.
    • Breaks existing code more.
    • All asynchronous API callbacks (event handlers, I/O, timers) are deferred functions.
    • immediate might actually be inferrable!
• If we started things from scratch, it feels like deferred would be the right choice.
  • Have to assume lots of people have .d.ts files outside of their control - the only workaround is to

    • update the d.ts or (very challenging for a lot of devs to be honest) or

    • define a function like

      function deferred<T extends (...args: any[]) => any>(deferred fn: T): T {
          return fn;
      }

      and pepper deferred(...) calls around each callback that has an issue.

      • But the callbacks don't error, subsequent references to a variable error. Users are likely to just cast instead.
  • If this was behind --strict it would be an easier sell for many many call-sites in an update.
  • Hard to get a sense without running on bigger codebases.
• With this PR, we are generally misunderstanding more code. Doesn't seem to catch as many bugs as we would think. It does error on idiomatic code like doing some async cleanup, but using to a variable afterwards
• Swapping the default behavior is quite surprising. It's probably a better default, but is it breaks for the sake of breaks?
• What are the conclusions and next steps?
  • We don't feel comfortable merging for 5.6.
  • We do want to review the breaks.
  • Ask for feedback from partner teams - can they adopt this? Are they catching bugs?