Widening and non-widening computed enum types #52542
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at a84ac81. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at a84ac81. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at a84ac81. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at a84ac81. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at a84ac81. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here are the results of running the user test suite comparing Everything looks good! |
|
@ahejlsberg Here they are:Comparison Report - main..52542
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Heya @ahejlsberg, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
| } | ||
| return type; | ||
| } | ||
|
|
||
| function getRegularTypeOfLiteralType(type: Type): Type { | ||
| return type.flags & TypeFlags.Literal ? (type as LiteralType).regularType : | ||
| return type.flags & TypeFlags.Freshable ? (type as FreshableType).regularType : | ||
| type.flags & TypeFlags.Union ? ((type as UnionType).regularType || ((type as UnionType).regularType = mapType(type, getRegularTypeOfLiteralType) as UnionType)) : | ||
| type; | ||
| } | ||
|
|
||
| function isFreshLiteralType(type: Type) { |
There was a problem hiding this comment.
perhaps this "family" of functions could be renamed now? they all (isFreshLiteralType, getRegularTypeOfLiteralType, getFreshTypeOfLiteralType, getRegularTypeOfLiteralType) mention "literal" type but now they will also handle enums and enums are not literal types
There was a problem hiding this comment.
Yeah, but isFreshFreshableType and getFreshTypeOfFreshableType are pretty awful. 😮
There was a problem hiding this comment.
Yeah, can't argue that 😅 OTOH, those names would be more correct - they sound a little bit weird at first but if we focus on their meaning, it makes sense.
There was a problem hiding this comment.
The fresh type of bel-air!
I suppose it could be argued that Enum.A is an “enum literal”, even if it’s not a literal in the grammatical sense. The compiler is treating it as one in a way by giving it its own nominal type (Enum)
There was a problem hiding this comment.
The main problem from my PoV, is that "literal" is already used in the codebase and it often doesn't include enums:
TypeScript/src/compiler/types.ts
Line 6042 in 3099385
I see now though that EnumLiteral also exist so maybe it isn't a big deal.
|
Tests and performance all look good. |
There was a problem hiding this comment.
I think since these are freshable now, we need a declaration emit test that validates we write out initializers for the fresh variant in declaration emit, eg,
declare function computed(x: number): number;
enum E {
A = computed(0),
B = computed(1),
C = computed(2),
D = computed(3),
}
export class A {
prop = E.A;
}needs to emit
export declare class A {
prop = E.A;
}rather than
export class A {
prop: E;
}(and that needs to not read back in as an error in a declaration file if it does right now, since it does serve a semantic purpose - preserving freshness)
Agreed, though your example wouldn't validate that since class properties are mutable locations and their inferred types are always widened. But we should have an example of a |
|
|
||
| class C { | ||
| p1 = E.B; | ||
| p2 = E.B as const; |
There was a problem hiding this comment.
A readonly p3 = E.B would be another good addition, since that one should be like a const declaration, iirc.
| @@ -6028,7 +6028,8 @@ export const enum TypeFlags { | |||
| /** @internal */ | |||
| Nullable = Undefined | Null, | |||
| Literal = StringLiteral | NumberLiteral | BigIntLiteral | BooleanLiteral, | |||
| Unit = Literal | UniqueESSymbol | Nullable, | |||
| Unit = Enum | Literal | UniqueESSymbol | Nullable, | |||
| Freshable = Enum | Literal, | |||
There was a problem hiding this comment.
| Freshable = Enum | Literal, | |
| /** @internal */ | |
| Freshable = Enum | Literal, |
There was a problem hiding this comment.
I intended for this to be public.
| /** @internal */ | ||
| export interface FreshableIntrinsicType extends IntrinsicType { | ||
| freshType: IntrinsicType; // Fresh version of type | ||
| regularType: IntrinsicType; // Regular version of type | ||
| export interface FreshableType extends Type { | ||
| freshType: FreshableType; // Fresh version of type | ||
| regularType: FreshableType; // Regular version of type |
There was a problem hiding this comment.
Was this intended to be public API now?
|
|
||
| // String literal types (TypeFlags.StringLiteral) | ||
| // Numeric literal types (TypeFlags.NumberLiteral) | ||
| // BigInt literal types (TypeFlags.BigIntLiteral) | ||
| export interface LiteralType extends Type { | ||
| export interface LiteralType extends FreshableType { |
There was a problem hiding this comment.
You could technically still keep FreshableType if you left the members in place.
export interface LiteralType extends FreshableType {}
export interface LiteralType {
freshType: LiteralType;
regularType: LiteralType;
|
I think all feedback has been addressed, can I get another review please? |
Fixes #52531.