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 — [ID-Prep] PR1 - Simplify declaration emit for literal types. by dragomirtitian · Pull Request #57444 · microsoft/TypeScript

Conversation

@dragomirtitian

@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) commented Feb 19, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Fixes #57443, #57441, #57442

Sorry, something went wrong.

Uh oh!

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

tonivj5 reacted with heart emoji
All reactions
  • ❤️ 1 reaction
dragomirtitian
Titian Cernicova-Dragomir (dragomirtitian) commented Feb 19, 2024
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
? getWidenedLiteralType(getTypeOfSymbol(symbol))
? getTypeOfSymbol(symbol)
: errorType;
if(!isReadonlySymbol(symbol)) {

@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) Feb 19, 2024

Copy link
Copy Markdown
Contributor 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.

The lack of this test is the reason for #57441

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread src/compiler/checker.ts Resolved

Uh oh!

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

Comment thread src/compiler/transformers/declarations.ts Resolved

Uh oh!

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

@dragomirtitian
Titian Cernicova-Dragomir (dragomirtitian) force-pushed the verbatim-literals branch from a937132 to 7fd08c4 Compare February 19, 2024 14:37
weswigham
Wesley Wigham (weswigham) requested changes Feb 20, 2024
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

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.

This introduces bugs in incremental compilation - by not preserving the widening behavior of the literals (wherever possible), by creating an initializer whenever the type at the location is a widening type, you can create declaration files where, when you check against the declaration files, you witness the literal type, while when you check against the source file, you witness the widening type (and thus, the base type). We have open issues about other types where we don't correctly do this today (eg, for tagged templates), and this would make the problem hugely worse.

This cannot be a baseline change in the compiler. This absolutely has to be flagged off, if we take it at all. There is a meaningful difference between a = 1 and : 1 on a variable declaration - that some emit mode can't track the widening behavior of literals across declarations does not mean we should introduce buggy behavior for all users.

Sorry, something went wrong.

Uh oh!

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

All reactions
@sandersn
Nathan Shively-Sanders (sandersn) requested a review from Andrew Branch (andrewbranch) February 27, 2024 19:30
@sandersn Nathan Shively-Sanders (sandersn) assigned Wesley Wigham (weswigham) Feb 27, 2024
@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) changed the title Simplify declaration emit for literal types. [ID-Prep] PR1 - Simplify declaration emit for literal types. Mar 7, 2024
@dragomirtitian
Titian Cernicova-Dragomir (dragomirtitian) force-pushed the verbatim-literals branch 2 times, most recently from 3ee926e to a6c7559 Compare March 7, 2024 22:54
@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) mentioned this pull request Mar 7, 2024
[ID-Prep] Preserve constant initializer in declarations as written in source #57443
Closed
6 tasks done
@dragomirtitian

Titian Cernicova-Dragomir (dragomirtitian) commented Mar 7, 2024

Copy link
Copy Markdown
Contributor Author

Wesley Wigham (@weswigham) Sorry it took a while to get back to this.

I changed the goal to just preserve the initializer node if possible. This should not change the meaning of the declaration.

It should also be a safe change to make. Even older versions of TypeScript will understand all of the primitive literal syntax, even if it was previously not present in declarations.

All reactions

Sorry, something went wrong.

Uh oh!

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

Titian Cernicova-Dragomir (dragomirtitian) added 4 commits March 8, 2024 12:46
@dragomirtitian
Added test that will change with this PR.
b0f1437
@dragomirtitian
Fix widened type in declarations files.
0f53db0
@dragomirtitian
Do not print initializer if node already has a type annotation.
75507b9
@dragomirtitian
Preserve initalizer nodes if they are primitive literals.
da5b104
@dragomirtitian
Titian Cernicova-Dragomir (dragomirtitian) force-pushed the verbatim-literals branch from 14c1e62 to da5b104 Compare March 8, 2024 13:28
@sandersn Nathan Shively-Sanders (sandersn) added the For Backlog Bug PRs that fix a backlog bug label Mar 8, 2024
@robpalme Rob Palmer (robpalme) mentioned this pull request Mar 13, 2024
--isolatedDeclarations for standalone DTS emit #47947
Closed
5 tasks done
@dragomirtitian Titian Cernicova-Dragomir (dragomirtitian) closed this Mar 18, 2024
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham requested changes
@andrewbranch Andrew Branch (andrewbranch) Awaiting requested review from andrewbranch

Uh oh!

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

Assignees

@weswigham Wesley Wigham (weswigham)

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ID-Prep] Preserve constant initializer in declarations as written in source

Uh oh!

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

3 participants

@dragomirtitian @weswigham @sandersn
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.