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, 1/18/2023 #52296

Closed
DanielRosenwasser opened this issue Jan 18, 2023 · 12 comments
Closed

Design Meeting Notes, 1/18/2023 #52296

DanielRosenwasser opened this issue Jan 18, 2023 · 12 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

newLine and forceConsistentFileNames Defaults

#51909

  • newLine to lf?
    • Didn't get to discuss it thoroughly.
  • Change default of forceConsistentFileNames to true?
    • Catch issues between Windows/Mac/Linux ahead of pushing out to CI.
    • We think it's good to turn on.
  • We all feel okay about the fact that it doesn't "do what people think", right?
    • "What?"
    • It doesn't check whether the paths match what's on disk, it just checks whether the paths are consistent across the project.
  • Deprecate it?
    • Prefer not to.
  • Conclusion
    • --newLine lf by default.
    • --forceConsistentFileNames true by default.

catch Clause Variables

#52240

// @useUnknownInCatchVariables: false

try {} catch ({ e }) {}
//              ^?

try {} catch ({ e }: any) {}
//              ^?

try {} catch ({ e }: unknown) {}
//              ^?


try {} catch ({ x: { e } }) {}
//                   ^?

try {} catch ({ x: { e } }: any) {}
//                   ^?

try {} catch ({ x: { e } }: unknown) {}
//                   ^?

Comparison Operators

#52036
#52048

  • Some obvious but questionable things that popped up.
    • Number < number now disallowed
    • string | number < number now disallowed
    • unknown < number now disallowed
  • Some stuff that fails due to control flow limitations across functions.
  • What about objects that coerce to number (e.g. Date)?
    • Why don't we look at valueOf?
      • Or [Symbol.toPrimitive]?
      • Here we go...
  • Conclusion: Feels like this PR is a good start. Want to bring this in for 5.0.
    • Watch for 5.0 Beta feedback.

JSON.stringify

#51897
#52250

  • This thing broke some code.
  • Technically stringify can return undefined.
    • But technically also wrong because a Function can have a toJSON method.
  • There's a world where we could possibly keep this as-is if we reordered the signatures so that never works.
    • Overload resolution goes through a subtype pass.
  • But why are we adding this overload? Who ends up in a situation like this?
    • Doesn't even catch mistakes like (() => string) | string today.
    • Only contrived cases are caught.
  • The idea that everyone has to suffer for stringify because of these contrived-but-uncommon cases feels unreasonable.
  • Could ship an open-ended union?
  • Could also just tell people to interface-merge.
  • Conclusion:
    • Revert this, tell people to interface-merge on JSON with something that returns undefined.

Picking Between Types from Type Predicates and Original Types

#50916

  • Made a change a while back to pick the types from type predicates/type assertions.
  • Similar behavior with instanceof too.
  • Means that the original type does not survive at joining points of CFA.
  • Idea
    • In the true branch, we will still prefer the type predicate type.
    • In the false branch, we will now preserve the original type so that they join back at a CFA merge node.
  • So what do we now do about mutual subtypes?
    • For example, unknown and any are mutual subtypes.
    • Today we choose the type with the lowest ID.
    • Pretty unfortunate - these have measurable effects.
    • So we are making any a strict supertype of unknown.
      • Behavior directly in unions is the same.
      • Where else does this appear?
        • When the type IDs are not compared directly.

        • So for e.g. Array<any> vs. Array<unknown> - but Array is not special.

          type Box<T> = { readonly value: T };
          
          declare let anyBox: Box<any>;
          declare let unknownBox: Box<unknown>;
          
          let boxes1 = [anyBox, unknownBox];
          //  ^?
          let boxes2 = [unknownBox, anyBox];
          //  ^?
        • Now you always end up with Box<any>[].

    • Similar constraints with {} and any non-empty object type.
  • What other effects does this have?
    • The type of an assignment expression is always the right side:

      function f(obj: { a?: string }) {
          (obj = {}).a; // allowed?
      }
    • Hmm...

      function f(a: { x?: number }, b: { x?: string}) {
          a = b = {}; // allowed?
          // Equivalent to `a = (b = {})` which naively looks like `b = {}; a = {}` from a type perspective.
      
          a.x = 42;
      
          b.x?.toUpperCase();
      }
      • Fresh {} object type has "inverted" subtype behavior.
        • It is, but on the assignment we widen.
        • Do we have to widen? Could choose not to.
        • Feels like for the b = {}, we have to propagate out some information, or further narrow based on the left side(?)
    • (...args: any[]) => any should be a super-type of all functions?

      • But what about (...args: never[]) => unknown?
        • Not specially understood today by TS internals.
@fatcerberus
Copy link

fatcerberus commented Jan 19, 2023

Isn’t the “any-like” function type (...args: any) => any?

@Andarist
Copy link
Contributor

Isn’t the “any-like” function type (...args: any) => any?

IIRC - yes. But the fact that it's not (...args: any[]) => any creates some issues, like this one. I was experimenting with changing the effective rest of this AnyFunction type any[] (and not any) here but IIRC that in turn created some issues with tuple types as any[] ain't assignable to all tuple types.

@Andarist
Copy link
Contributor

Revert this, tell people to interface-merge on JSON with something that returns undefined.
The idea that everyone has to suffer for stringify because of these contrived-but-uncommon cases feels unreasonable.

While some of those are contrived and uncommon - this isn't quite that (in my opinion):

function stringify(a?: object): string {
  return JSON.stringify(a)
}

stringify(undefined) // undefined, oops

I had a production issue recently cause I accidentally passed an optional param/property to a stringifying function and somewhere down the road I expected to have a guaranteed string

@fatcerberus
Copy link

Also IIRC (...args: never) => unknown is understood by TS as the function top type. never[] is problematic because then it’s legal to call it with no arguments.

@DanielRosenwasser
Copy link
Member Author

While some of those are contrived and uncommon - this isn't quite that (in my opinion):

function stringify(a?: object): string {
  return JSON.stringify(a)
}

stringify(undefined) // undefined, oops

@Andarist I tried to capture it in the notes, but the new overload doesn't model that type correctly anyway - object | undefined will fall into the any case and provide you with string.

@DanielRosenwasser
Copy link
Member Author

Also IIRC (...args: never) => unknown is understood by TS as the function top type.

@ahejlsberg

Also - TIL that never is a valid rest parameter type.

@RyanCavanaugh
Copy link
Member

The thing that got us super derailed on the JSON.stringify thing was that people have the entirely reasonable assumption that if they pass something that isn't undefined, especially something that is object, then it "definitely won't" return undefined. But this isn't true -- functions are object (there are instanceof Object, they have all of Object's properties, etc), so given any value in TypeScript, we don't have any way to guarantee that it isn't actually referencing something that is secretly a function (or, more degenerately, has a toJSON method that returns undefined). And passing a function to JSON.stringify will return undefined, unless that function has a custom toJSON method. This isn't even particularly theoretical, since class constructors are also functions, yet can easily have static methods that satisfy object types.

Thus while you could write a conditional type to distribute over the possible inputs and say that it only possibly returns undefined if one of the inputs is possibly undefined... that type would also be wrong, in exactly the same failure mode that the type today is wrong.

So we're basically stuck with two options:

  • Today's behavior, never return undefined, which is obviously wrong and extremely annoying when you break prod
  • The alternate behavior, always return string | undefined, which is also obviously wrong and extremely annoying when in practice you will, 99% of the time, be able to construct a sound argument that none of the things I'm warning you about actually happen in this code, as far as anyone can tell, probably

Regardless, if you do want the alternate behavior, it's very easy to add an interface merge to your project and get the "always might be undefined" behavior. No commandline flags / custom lib / etc needed. Just hope that you don't overly habituate on writing JSON.stringify(x)! and end up right back where you started 😉

@Andarist
Copy link
Contributor

Right, I see the reasoning and I agree that adding | undefined would be quite extremely annoying (but I also tend to think that the type system is here to "annoy" us 😉 ). The problem with the proposed solution (having to use interface merging) is that one has to opt into this - which for most projects would mean that they never opt into this or that they only opt into this after they break production.

I understand though that this is a very hard/impossible choice to make here and there is no way to satisfy everybody.

Just hope that you don't overly habituate on writing JSON.stringify(x)! and end up right back where you started 😉

Well, at least that surfaces the possibility to the writer and reader, it educates them about this problem. So this is a good thing - even if people start adding ! to each JSON.stringify call. It shifts the responsibility onto the dev.

@fatcerberus
Copy link

but I also tend to think that the type system is here to "annoy" us 😉

To a point. The design goals do include the caveat “strike a balance between correctness and productivity” for a reason. JS has no undefined behavior and soundness is explicitly not an end goal so it’s always a subjective judgment call what should be a compile-time error vs. not.

@cherryblossom000
Copy link
Contributor

never[] is problematic because then it’s legal to call it with no arguments.

@fatcerberus This code doesn’t cause any errors:

declare const f: (...args: never) => unknown
f()

Playground

@fatcerberus
Copy link

fatcerberus commented Jan 22, 2023

That looks like a bug. [] (the effective type of args in that call) is not assignable to never. Compare that to never[], for which an empty array is a legal value.

@cherryblossom000
Copy link
Contributor

Found the issue for that bug: #48840

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

5 participants