fix: remove .value field from type declarations for boolean literal types

[kirkwaiblinger]

PR Checklist

• Addresses an existing open issue: fixes 🐛 Bug: .value does not exist in BooleanLiteralType #528
• That issue was marked as status: accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Ostensibly, this is the code change, but it looks pretty intentional that it was written this way in the first place, so.... maybe needs some more investigation in order to understand why it was written this way.

I'm not really sure how to test for this, either. I've written a test that proves the bug's existence, but wouldn't prevent it from regressing. Open to suggestions 🤔 I've written a test that proves the bug's existence at runtime, and will cause a type-checking error if the value field is added back, due to its use of @ts-expect-error

[RebeccaStevens]

Note: This would be a breaking change.

[JoshuaKGoldberg]

👍 good timing with #531!

[RebeccaStevens]

@JoshuaKGoldberg Should we remove UnknownLiteralType or keep it as an alias of FreshableIntrinsicType seems we are no longer adding any properties to it?

[JoshuaKGoldberg]

Good call, I think we should go with the same strategy for the two interfaces.

Actually, come to think of it, maybe we don't need to get rid of them at all - maybe we can just switch from the value: ... property to using a literal for the existing intrinsicName property? As in:

Suggested change

	export interface UnknownLiteralType extends FreshableIntrinsicType {}
	export interface UnknownLiteralType extends FreshableIntrinsicType {
	intrinsicName: "unknown";
	}

https://typescript-eslint.io/play#ts=5.6.2&showAST=types&fileType=.tsx&code=CYUwxgNghgTiAEYD2A7AzgF3gNyhAriAEJJIQhQoBc8ARqeZQNwBQLoksCy6WuBIAKooA1iiQB3avHyjxU1kA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

WDYT?

[kirkwaiblinger]

@RebeccaStevens

Note: This would be a breaking change.

Coming back to this, I guess I want to gently push back on this. The current type BooleanLiteralType (and its subtypes),

export interface BooleanLiteralType extends UnknownLiteralType {
	intrinsicName: "false" | "true";
	value: boolean;
}

does not describe a real runtime object from which we're removing a runtime field. Instead, it is an inaccurate description of a third party runtime value generated by TS. Therefore, any code that currently relies on this field is already broken. Removing this field from the type definition would, as far as I can tell, be strictly beneficial to TS users, since it would reveal the existing runtime bug at type-checking time. The really nice thing about correcting a type is that we know we're not introducing any runtime regressions, since types have no effect at runtime. Thoughts?

---

@JoshuaKGoldberg

Actually, come to think of it, maybe we don't need to get rid of them at all - maybe we can just switch from the value: ... property to using a literal for the existing intrinsicName property? As in:

https://typescript-eslint.io/play#ts=5.6.2&showAST=types&fileType=.tsx&code=CYUwxgNghgTiAEYD2A7AzgF3gNyhAriAEJJIQhQoBc8ARqeZQNwBQLoksCy6WuBIAKooA1iiQB3avHyjxU1kA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

WDYT?

I'm wondering if there's a misunderstanding here? The type UnknownLiteralType is not describing the TS unknown type; it's a base type for actual literal types, such as BooleanLiteralType type (in fact maybe this is the only one??), to extend. Therefore, the intrinsic name is not the literal 'unknown'. In fact, that type no longer adds any information over its base type (which already includes intrinsicName: string), as both @RebeccaStevens and @typescript-eslint/no-empty-interface rightly point out.

So, in theory, based on that information alone, I would say it can be removed.... However, we run into a bit of an awkward situation, since both isUnknownLiteralType and isFreshableIntrinsicType are exported, and they have different runtime implementations.

ts-api-utils/src/types/typeGuards/literal.ts

Lines 169 to 191 in ad63aca

	/**
	* `LiteralType` from typescript except that it allows for it to work on arbitrary types.
	* @category Type Types
	*/
	export interface UnknownLiteralType extends FreshableIntrinsicType {}
	/**
	* Test if a type is a {@link UnknownLiteralType}.
	* @category Types - Type Guards
	* @example
	* ```ts
	* declare const type: ts.Type;
	*
	* if (isUnknownLiteralType(type)) {
	* // ...
	* }
	* ```
	*/
	export function isUnknownLiteralType(
	type: ts.Type,
	): type is UnknownLiteralType {
	return isTypeFlagSet(type, ts.TypeFlags.Literal);
	}

ts-api-utils/src/types/typeGuards/compound.ts

Lines 7 to 30 in 8c74729

	/**
	* A type that is both an {@link IntrinsicType} and a `FreshableType`
	* @category Type Types
	*/
	export interface FreshableIntrinsicType
	extends ts.FreshableType,
	IntrinsicType {}
	/**
	* Test if a type is a `FreshableIntrinsicType`.
	* @category Types - Type Guards
	* @example
	* ```ts
	* declare const type: ts.Type;
	*
	* if (isFreshableIntrinsicType(type)) {
	* // ...
	* }
	*/
	export function isFreshableIntrinsicType(
	type: ts.Type,
	): type is FreshableIntrinsicType {
	return isIntrinsicType(type) && isFreshableType(type);
	}

If we remove UnknownLiteralType, and replace its usages with FreshableIntrinsicType, then, we have two different type guards for the exact same type, and, well, deciding how to handle that (and avoiding breaking changes) involves some decision-making that's not really my place to make. Therefore, my approach was to just leave the now-"useless" UnknownLiteralType in place for this PR, and see if it came up in review 🤷. I don't know what the right thing to do here is, and/or whether it makes sense to punt to a followup?

[kirkwaiblinger]

@JoshuaKGoldberg what do you think about the UnknownLiteralType part?

[JoshuaKGoldberg]

Oh, right. I think it makes sense to punt.

[RebeccaStevens]

I am swayed and am 👍 on treating this as a bugfix rather than a breaking change.

My reasoning for this being a breaking change is simply that if someone was relying on value existing on UnknownLiteralType then things would break. If they were using BooleanLiteralType, then obviously they have a bug but if they aren't, then their would be nothing wrong with their code that all of a sudden it would no longer work.

This is on of those things though where it just depends on where you draw the line for what counts as a breaking change and what doesn't. Theoretically, all changes are breaking changes as a user may be relying on the thing that was changed.

R.E. Breaking Change: The CPU no longer overheats when you hold down spacebar

[kirkwaiblinger]

My reasoning for this being a breaking change is simply that if someone was relying on value existing on UnknownLiteralType then things would break. If they were using BooleanLiteralType, then obviously they have a bug but if they aren't, then their would be nothing wrong with their code that all of a sudden it would no longer work.

Oh, I missed that the break you were referring to was specifically UnknownLiteralType rather than the removal of value in general from BooleanLiteralType and its subtypes. Not necessarily sure what to do about that (or whether it matters??) but an alternative would be to have BooleanLiteralType extend FreshableIntrinsicType directly and cut out the UnknownLiteralType which adds the erroneous value field?

That being said, it still seems like it's a bug to have value: unknown on UnknownLiteralType since isUnknownLiteralType returns true for booleans, even though they don't have the value field. Maybe it can still be removed? Or maybe it should be changed to value?: unknown?

[RebeccaStevens]

I think value?: unknown should be fine to use for a bug fix and we can deprecate UnknownLiteralType.

In v2 we can remove UnknownLiteralType altogether.

[JoshuaKGoldberg]

Sorry thought I commented these :c

[JoshuaKGoldberg]

Good call, I think we should go with the same strategy for the two interfaces.

Actually, come to think of it, maybe we don't need to get rid of them at all - maybe we can just switch from the value: ... property to using a literal for the existing intrinsicName property? As in:

Suggested change

	export interface UnknownLiteralType extends FreshableIntrinsicType {}
	export interface UnknownLiteralType extends FreshableIntrinsicType {
	intrinsicName: "unknown";
	}

https://typescript-eslint.io/play#ts=5.6.2&showAST=types&fileType=.tsx&code=CYUwxgNghgTiAEYD2A7AzgF3gNyhAriAEJJIQhQoBc8ARqeZQNwBQLoksCy6WuBIAKooA1iiQB3avHyjxU1kA&eslintrc=N4KABGBEBOCuA2BTAzpAXGYBfEWg&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

WDYT?

[kirkwaiblinger]

Looks like the CI failures are occurring on main, not due to the changes

[RebeccaStevens]

@kirkwaiblinger @JoshuaKGoldberg I've made a couple of updates to the branch. I think it should be good to go now.

[JoshuaKGoldberg]

Cool, thanks for this all! It was a good back-and-forth 😄.

I'll go ahead and merge this now to get a release. Then I'll send a followup issue -> PR to remove the @deprecated bits in the next major alongside #531.