GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Preserve the distributivity of inlined conditional types in declaration emit by weswigham · Pull Request #48592 · microsoft/TypeScript

Conversation

@weswigham

@weswigham Wesley Wigham (weswigham) commented Apr 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

By wrapping them in two more layers of conditionals - one to establish a local type parameter for distributivity, one to constrain said type parameter so it remains usable as the instantiated check type.

This means something like type PrivateFields<Keys> = Keys extends _${string} ? Keys : undefined; when used as PrivateFields<keyof Obj> will be inlined to keyof Obj extends infer T ? T extends keyof Obj ? T extends _${string} ? T : never, to retain the distributive behavior, rather than the erroneous keyof Obj extends _${string} ? keyof Obj : never.

Fixes #48140

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

dragomirtitian reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@weswigham
Preserve the distributivity of inlined conditional types in declarati…
9770a54
…on emit
@typescript-bot TypeScript Bot (typescript-bot) assigned Wesley Wigham (weswigham) Apr 6, 2022
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Apr 6, 2022
@weswigham
Wesley Wigham (weswigham) requested review from Ryan Cavanaugh (RyanCavanaugh), Andrew Branch (andrewbranch) and Nathan Shively-Sanders (sandersn) April 6, 2022 17:57
andrewbranch
Andrew Branch (andrewbranch) reviewed Apr 6, 2022
View reviewed changes
Comment thread tests/baselines/reference/declarationEmitInlinedDistributiveConditional.js
export {};
//// [api.d.ts]
export declare const dropPrivateProps1: <Obj>(obj: Obj) => { [K in import("./internal").PublicKeys1<keyof Obj>]: Obj[K]; };
export declare const dropPrivateProps2: <Obj>(obj: Obj) => { [K in keyof Obj extends infer T ? T extends keyof Obj ? T extends `_${string}` ? never : T : never : never]: Obj[K]; };

@andrewbranch Andrew Branch (andrewbranch) Apr 6, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Is it not a problem that the middle conditional type (the inner of the two synthesized ones) is distributive itself? It seems like it comes out the same, but in a way the semantics are slightly different here, right?—the T extends `_${string}` bit will never actually have anything to distribute over because the distribution is done one level up.
  2. Whether or not (1) actually matters, you could use Ron’s new infer constraints here instead, which would reduce a level of nesting. That seems preferable to me, unless we’re specifically trying to avoid emitting that for back compat reasons, but... I think that’s a job for downlevel-dts.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@weswigham Wesley Wigham (weswigham) Apr 6, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Extra layers of distributivity don't matter, so long as the right type variable is distributed over before the conditional logic occurs.
  2. Yeah, I mention that in a comment in the code - we could, but we'd start producing declaration files incompatible with older versions of TS and I don't know if we wanna do that just yet (considering they don't come from any input code positions). It's always something we could change down the line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

andrewbranch reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@andrewbranch Andrew Branch (andrewbranch) Apr 6, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other thing I was curious about is whether we could ever just copy the unreachable type alias declaration to a local one. If we’re already copying its contents, why not just drop the whole declaration nearby with a unique name?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@weswigham Wesley Wigham (weswigham) Apr 6, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wherever we can, we do (specifically if we can preserve it as-is, in it's existing location). We only do inlining like this when the type comes from somewhere unreachable or elided (like inside a different module's locals or an elided function body). We don't copy from those scopes into the current scope because then we'd have to do 1. pull in its whole dependency tree of types (which may, in turn, depend on things we just can't copy, like signature type parameters), and 2. ensure everything is renamed in such a way where there's no name conflicts. It's just not something we do much of right now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@andrewbranch Andrew Branch (andrewbranch) Apr 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pull in its whole dependency tree of types

This is what I thought at first, but don’t we end up having to do that if we inline it too? Like the only part of the type that we don’t copy is the declaration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
andrewbranch
Andrew Branch (andrewbranch) approved these changes Apr 7, 2022
View reviewed changes
@weswigham
Wesley Wigham (weswigham) merged commit 4d2fb54 into microsoft:main Apr 7, 2022
@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) mentioned this pull request May 11, 2022
Bloomberg feedback on TypeScript 4.7 Beta #49071
Closed
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andrewbranch Andrew Branch (andrewbranch) andrewbranch approved these changes
@RyanCavanaugh Ryan Cavanaugh (RyanCavanaugh) Awaiting requested review from RyanCavanaugh
@sandersn Nathan Shively-Sanders (sandersn) Awaiting requested review from sandersn

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@weswigham Wesley Wigham (weswigham)

Labels

Author: Team For Milestone Bug PRs that fix a bug with a specific milestone

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Declaration emit may change type semantics when inlining conditional types

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@weswigham @andrewbranch @typescript-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.