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

5.1.3 new "feature"? Branded types are preserved in const template literal #54648

Closed
conorbrandon opened this issue Jun 14, 2023 · 20 comments
Closed
Assignees
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@conorbrandon
Copy link

Bug Report

πŸ”Ž Search Terms

branded, interpolation, 5.1.3

πŸ•— Version & Regression Information

This changed between versions 5.0.4 and 5.1.3

⏯ Playground Link

Playground link with relevant code

πŸ’» Code

type S = `id_${string & {_brand: "ID"}}`;
//   ^? type S = `id_${string & { _brand: "ID"; }}`
declare const id: string & {_brand: "ID"};
const s = `id_${id}` as const;
//    ^? const s: `id_${string & { _brand: "ID"; }}`

πŸ™ Actual behavior

The intersected brand is preserved during string interpolation for variables and types!

πŸ™‚ Expected behavior

Pre 5.1.3, the intersected brand always got removed during string interpolation for variables and types.

This is part bug report and question. My question being in future versions, will this behavior be preserved? I wrote a clunky generic function and type to do this exact thing because I found it valuable, but I'd much rather rely on native behavior if this is going to stick around πŸ‘€

@RyanCavanaugh
Copy link
Member

I think this was (unintentional?) fallout from #48034 / #52345

@jakebailey
Copy link
Member

I think this was intentional? (I at least explicitly added it and tests for it, though in the course of fixing other stuff.)

Also @weswigham

@jakebailey
Copy link
Member

Though, I'm not sure how this should interact with as const... I guess it's a noop and that's normal?

@jakebailey
Copy link
Member

The relevant PR is #52836, note that the tests contain effectively exactly what the original post contains, a branded type inside a template.

@conorbrandon
Copy link
Author

Ah, template literals, that's the word I was looking for (instead of string interpolation). Had a brain fart, my apologies.

@conorbrandon conorbrandon changed the title 5.1.3 new "feature"? Branded types are preserved in const string interpolation 5.1.3 new "feature"? Branded types are preserved in const template literal Jun 15, 2023
@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Jun 19, 2023
@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 22, 2023
@ahejlsberg ahejlsberg self-assigned this Nov 15, 2023
@ahejlsberg ahejlsberg added Bug A bug in TypeScript and removed Working as Intended The behavior described is the intended behavior; this is not a bug labels Nov 15, 2023
@ahejlsberg
Copy link
Member

I'm reopening this one. The original issue, #48034, was that we're not "untagging" literal types when they occur in template literal placeholders. In other words, the expectation was that

type S1 = `xyz${"a"}`;
type S2 = `xyz${"a" & { tag: any }}`;

both resolve to "xyza". Originally, S2 would resolve to string. Currently, S2 resolves to `xyz${"a" & { tag: any }`. Neither of those are right, S2 should just resolve to "xyza".

@ahejlsberg ahejlsberg added this to the TypeScript 5.3.1 milestone Nov 15, 2023
@ahejlsberg ahejlsberg reopened this Nov 15, 2023
@jakebailey
Copy link
Member

I guess that implies that #52836 is to be reverted? πŸ™

There are some performance considerations (see #52345), so hopefully we don't regress that back again. That and the branded use case, and #53427, etc.

@ahejlsberg
Copy link
Member

I guess that implies that #52836 is to be reverted? πŸ™

I think so. But we still want to revert #48044 also. I think all we need is support for "popping tags" on literal types (such that "a" & { tag: void } behaves the same as just "a").

I really don't want to mess with supporting generalized intersections in placeholders. It doesn't add much in terms of expressiveness, but it adds a lot in terms of complexity and possible performance issues related to normalization.

@ahejlsberg
Copy link
Member

There's also #54188 which I really don't like. We decided to allow string & {} | "a" | "b" to not reduce to just string because the pattern was in use in some popular DT packages. But it's not at all clear we want to broaden this support to template literals--and even if we did, I would think it's the template literal itself that should be intersected with {}, not one of the placeholders.

@jakebailey
Copy link
Member

I guess we'll talk about it in the design meeting, but I can't help but feel a little sad given this issue was about the construct being useful, and I found the branded case to be a valuable idea: https://github.com/microsoft/TypeScript/blob/e170bc59d4ba0d335ce86f66296bec71c5018317/tests/cases/compiler/templateLiteralIntersection2.ts

I'll have to resurrect the perf test I had that motivated my follow-up.

@ahejlsberg
Copy link
Member

@jakebailey I saw that test, but I'm not sure I've seen an actual user request for the feature. Other than just stripping tags. I'm just not convinced anything beyond that is worth the complexity.

@jakebailey
Copy link
Member

Just to link it (since it's in the middle of a thread), #52345 (comment) is the perf case I had been looking into.

@ahejlsberg
Copy link
Member

ahejlsberg commented Nov 16, 2023

My inclination was otherwise, but since the consensus in the design meeting trended towards preserving tagged literals in placeholders, I'm going to go along. We still need a few fixes however, such as #53427 and not descending into template literals in getGenericObjectFlags. Plus the issue I mentioned here. I will put up a PR to that effect.

@jakebailey
Copy link
Member

Funny, I had the opposite takeaway from the meeting given the potential perf benefit πŸ˜…

@ahejlsberg
Copy link
Member

My chief performance concern was the overhead of descending into template literal types in getGenericObjectFlags and adding the objectFlags property to it. I'm backing that out in my PR as it isn't necessary. Beyond that, there is of course some overhead associated with preserving tagged literal types in placeholders, but you only incur that overhead if you use tagged literal types. It seemed like most folks liked the preservation and I'm willing to go with it.

@ahejlsberg ahejlsberg added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Nov 16, 2023
@unional
Copy link
Contributor

unional commented Mar 12, 2024

This behavior is causing a few types in type-plus to fail (e.g. IsTemplateLiteral, IsStringLiteral, Omit, IsNegative, etc) unional/type-plus#429.

In term of soundness, IMO it does make sense that ${string & { a: 1 }} to be reduced to ${string}.

in JS, it would be:

const extendedStr = Object.assign('abc', { a: 1 })
console.log(`${extendedStr}`) // 'abc'

the reasoning being the toString(): string remains unchanged thus the resulting type should be safe to reduce.

@conorbrandon
Copy link
Author

@unional That is a valid point, indeed.

@conorbrandon
Copy link
Author

It didn’t come up in the original discussion, but I might as well add it now. The specific use case for this relates to DynamoDB keys. A common pattern for such keys is, for example, USER#${UserID}. In these template literals, it has proven valuable to have a stricter type that includes the brand to avoid mistakes, especially during refactors.

i.e., this:

type UserID = string & { [BRAND]: "UserID" };
type Key = `USER#${UserID}`;

vs. this:

type KeyWide = `USER#${string}`;

Of course, there are workarounds, such as a helper function to enforce a KeyWide actually includes a UserID,

const formatUserKey = (userID: UserID): KeyWide => `USER#${userID}`;

but it's convenient to be able to write them literally as well, and have stronger assurances:

const foo = "foo";
const key: Key = `USER#${foo}`; // oops!

So I would like to see this stick around (as I mentioned, it has proven valuable), however, I do understand that "it's convenient" is not the strongest of arguments, especially if this is causing issues elsewhere.

@unional
Copy link
Contributor

unional commented Mar 17, 2024

Connecting related issues:

#49839
#54648
#57776
#57807

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants