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

[ID-Prep] PR1 - Simplify declaration emit for literal types. #57444

Closed
wants to merge 4 commits into from

Conversation

dragomirtitian
Copy link
Contributor

@dragomirtitian dragomirtitian commented Feb 19, 2024

Fixes #57443, #57441, #57442

: errorType;
if(!isReadonlySymbol(symbol)) {
Copy link
Contributor Author

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

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/transformers/declarations.ts Outdated Show resolved Hide resolved
Copy link
Member

@weswigham weswigham left a 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.

@dragomirtitian dragomirtitian changed the title Simplify declaration emit for literal types. [ID-Prep] PR1 - Simplify declaration emit for literal types. Mar 7, 2024
@dragomirtitian dragomirtitian force-pushed the verbatim-literals branch 2 times, most recently from 3ee926e to a6c7559 Compare March 7, 2024 22:54
@dragomirtitian
Copy link
Contributor Author

@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.

@sandersn sandersn added the For Backlog Bug PRs that fix a backlog bug label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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