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/16/2018 #22011

Closed
DanielRosenwasser opened this issue Feb 17, 2018 · 9 comments
Closed

Design Meeting Notes, 2/16/2018 #22011

DanielRosenwasser opened this issue Feb 17, 2018 · 9 comments
Labels
Design Notes Notes from our design meetings

Comments

@DanielRosenwasser
Copy link
Member

Recursive Conditional Types

#21316 (comment)

  • @tycho01 already reiterated we have recursive conditional types with indexed access types on object types (Add Awaited<T> to core library #21613 (comment))
    • Well damn it, if you can already do it with object types, why make it ugly to do with regular conditional types?
  • Conditional types contain 4 types
    • The checked type, the extends type, the true type, and the false type
    • When the condition is definitely true or false, we do an immediate instantiation (/evaluation) of the respective branch
    • Those branches are already sort of deferral points, much like the object member types.
    • If an conditional type is instantiated over 100 times, we consider that to be too deep.
      • At that point, we try to find the respective alias type that contains that conditional type.
  • [[Demonstration of Peano-style arithmetic with multiplication]]
  • Challenges
    1. The "we've instantiated too deeply" error is issued on the declaration instead of the usage, which is not totally obvious.
    2. The act of checking whether a type satisfies a constraint in a conditional type (e.g. seeing if A is assignable to B in A extends B) requires evaluating A.
      • That means if A is a conditional type, either branch needs to be evaluated immediately to see if it satisfies B. That means you can end up exhausting your instantiation stack.
  • Question: Why not defer the type when comparing a conditional to the constraint type.
    • doesn't make sense, you need to check now
      • you could imagine that our compiler architecture was more deferred, but that would make it much harder to reason about.
  • Question: or, when you hit the limit, stop evaluating once you hit the limit and defer the evaluation?
  • Also, this instantiation stack limit is quite annoying: you can get errors on unrelated type aliases.
    • What if your stack is tied per type instantiation?
    • Or rather than per type, you use the global stack but check specific types in that stack.
  • Conclusion: give users an error in these situations, try to improve the message.

Ignore specific errors with @ts-ignore comments

Issue: #19139
PR: #21602

  • Given the error code, what are you actually suppressing?
    • Seems entirely unclear.
    • Error codes don't tell you what you're doing
      • Short-names are better than numbers.
  • So short names?
    • But this gives a bad UX; way more on the screen than you wanted.
    • Also, people are probably relying on the error codes even though they shouldn't be!!
    • What if we gave short codes in --pretty?
  • Could give quick info for the error code.
    • Great but how does that help you on a code review on GitHub?
      • Why, do you inherently distrust the code you're reviewing?
        • Sometimes yes?
  • A lot of these are to suppress lint rules.
    • Maybe the solution is just to get out of the linting business. :)
  • What if these comments caused errors when no error's present on the next line?
    • We might feel better about that.
      • Half jokingly: Can you suppress those errors?
        • Now the idea has become a half-joke.
  • Conclusion: no conclusion yet.
  • Post-design meeting realization: original motivation was mass suppression for working code.
    • Seems more reasonable with that in mind.
@DanielRosenwasser DanielRosenwasser added the Design Notes Notes from our design meetings label Feb 17, 2018
@KiaraGrouwstra
Copy link
Contributor

Well damn it, if you can already do it with object types, why make it ugly to do with regular conditional types?

well, the new conditional types solve type-level type checks.

@AriaMinaei
Copy link

Now that error suppression is being discussed, I'd like to propose also adding a compiler flag for reporting unused suppressions. #22021

@alexburner
Copy link

It seems like the discussion on @ts-ignore went something like:

  • Error codes are unclear
  • So, short names? But they have a host of other problems
  • And some other points too

I'd just like to toss out: at my place of work we would gladly use unclear error codes, if it gave us the ability to suppress errors. Heck, we'd use savage personal insults, as long as it allowed us this functionality.

We have a large JS application we're migrating to TS. We'd like to turn on the --strictNullChecks and --noImplicitAny flags, squelch the errors in old code, and be protected in new code. Currently, we're just turning on those flags, chipping away at the errors, then turning them back off (time permitting).

This will work eventually, but in the meanwhile there is nothing preventing us from introducing new null & implicit any errors. (I mean, there are diligent code reviews, but who trusts humans? That's why we use static type checking in the first place.)

TL;DR: we don't care if it's pretty, we just want the compiler to yell at us for bad null handling

@RyanCavanaugh
Copy link
Member

@alexburner what about the current ts-ignore functionality is blocking you from that approach?

One possible path would be to build once, write a quick script to insert @ts-ignores in the erroring lines, and then go back to try to remove them. I believe that's what Google does when they upgrade or turn on new flags.

@alexburner
Copy link

Just the specificity, we don't want to throw the baby out with the bathwater. All of our current TS code compiles fine otherwise, we only want to suppress the --strictNullChecks and --noImplicitAny errors.

@RyanCavanaugh
Copy link
Member

The real problem (I'm surprised this didn't make it into the notes) is that we generally don't consider a different error message being issued as a significant breaking change.

For example, we sometimes add a more-specific error message for certain cases to give a better developer experience. Every error message has its own distinct error code.

We don't want people to get into a situation where upgrading from TS X.Y to X.Y+1 yields hundreds of new errors simply because we changed an error message code, or end up doing a bunch of Rube Goldberg work in the checker to issue a "legacy" error message in some cases.

@alexburner
Copy link

In this case, we'd be down for that danger! It would still be an improvement over our current situation (and, ideally, we wouldn't be leaning on these "ignores" at some point in the future).

But, I do see how this makes things ugly, from a general public perspective. You don't want to be responsible for innocent bystanders receiving new errors during upgrading. And supporting legacy error messages sounds like a pain.

Is there any way this could be released as an experimental/volatile feature? Some sort of marketing/positioning that made it clear users are opting in to unpredictable behavior?

@RyanCavanaugh
Copy link
Member

The extent to which we've tried that is basically --experimentalDecorators, and now hundreds of millions of lines of Angular code depend on it and we can realistically never change it without adding a complex set of back-compat flags to keep supporting the current behavior.

The rando who shows up on HN or Twitter to trash TypeScript 3.7 because it put hundreds of new errors in his codebase isn't going to disclaim that he had --makeUpgradesImpossible enabled, and why would he? A different engineer turned that flag on without asking or telling anyone. Still not his fault, though - Why would TypeScript add such a dangerous flag in the first place? Bad decision making over at Microsoft.

Then he's here on the issue tracker along with everyone else who got opted into a janky build system requesting that we add an --ignoreIgnoreMessages flag to ignore the message part of the ignore.

Then we need another flag to specify when they don't don't want to not ignore a ts-ignore, then... 😢

@alexburner
Copy link

Then we need another flag to specify when they don't don't want to not ignore a ts-ignore, then... 😢

Okayyyy okay okay, I now viscerally understand the pains of supporting an open source project on this scale. Thank you for playing out this thought experiment with me, I will let you be 😅

@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design Notes Notes from our design meetings
Projects
None yet
Development

No branches or pull requests

5 participants