Uh oh!
There was an error while loading. Please reload this page.
isolatedModules errors for non-literal enum initializers - #56736
Conversation
Jake Bailey (jakebailey)
commented
Dec 11, 2023
TypeScript Bot (@typescript-bot) test top200 TypeScript Bot (@typescript-bot) perf test this |
Heya Jake Bailey (@jakebailey), I've started to run the regular perf test suite on this PR at c51645a. You can monitor the build here. |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based top-repos suite on this PR at c51645a. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the parallelized Definitely Typed test suite on this PR at c51645a. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the diff-based user code test suite on this PR at c51645a. You can monitor the build here. Update: The results are in! |
Heya Jake Bailey (@jakebailey), I've started to run the tarball bundle task on this PR at c51645a. You can monitor the build here. |
Hey Jake Bailey (@jakebailey), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
TypeScript Bot (typescript-bot)
commented
Dec 11, 2023
Jake Bailey (@jakebailey) Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
TypeScript Bot (typescript-bot)
commented
Dec 11, 2023
Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready. |
TypeScript Bot (typescript-bot)
commented
Dec 11, 2023
Jake Bailey (@jakebailey) Here are the results of running the top-repos suite comparing Everything looks good! |
Jake Bailey (jakebailey)
commented
Dec 11, 2023
TypeScript Bot (@typescript-bot) perf test this |
Heya Jake Bailey (@jakebailey), I've started to run the regular perf test suite on this PR at c51645a. You can monitor the build here. Update: The results are in! |
TypeScript Bot (typescript-bot)
commented
Dec 11, 2023
Jake Bailey (@jakebailey) Here they are:CompilerComparison Report - baseline..pr
System info unknown Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown Hosts
Scenarios
Developer Information: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
Thanks Jan Kühle (@frigus02)!
Whatever we do in evaluatesToStringLiteral and evaluatesToNumericLiteral here will become the minimum requirement for all third-party compilers to implement to guide their own emit. I think what you have is pretty reasonable, but we should get the team, and other compiler maintainers, to weigh in. Nicolò Ribaudo (@nicolo-ribaudo)Evan Wallace (@evanw)Donny/강동윤 (@kdy1)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Jake Bailey (jakebailey)
commented
Jan 4, 2024
At least for The new test cases don't fail currently in |
Jan Kühle (frigus02)
left a comment
There was a problem hiding this comment.
Thanks for the review, Andrew and Jake. I addressed the comments.
This implementation is quite thorough. I wanted to avoid too many new errors in our code base when turning on isolatedModules, so I tried to support as much as possible. I'm happy to simplify it if you like, though.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Jan Kühle (frigus02)
commented
Jan 24, 2024
It's probably too late to get this into 5.4 now, is it? That's alright. Rather than introducing 2 new functions, which duplicate some of the `evaluate` logic, we could also extend `evaluate` to return whether the evaluation was purely syntactic, e.g.:diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index b0ea7ea9ff..78cab2d428 100644
--- a/src/compiler/checker.ts+++ b/src/compiler/checker.ts@@ -38687,7 +38687,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (isConstContext(node) || isTemplateLiteralContext(node) || someType(getContextualType(node, /*contextFlags*/ undefined) || unknownType, isTemplateLiteralContextualType)) {
return getTemplateLiteralType(texts, types);
}
- const evaluated = node.parent.kind !== SyntaxKind.TaggedTemplateExpression && evaluateTemplateExpression(node);+ const evaluated = node.parent.kind !== SyntaxKind.TaggedTemplateExpression && evaluateTemplateExpression(node).value;
return evaluated ? getFreshTypeOfLiteralType(getStringLiteralType(evaluated)) : stringType;
}
@@ -45010,15 +45010,17 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (!(nodeLinks.flags & NodeCheckFlags.EnumValuesComputed)) {
nodeLinks.flags |= NodeCheckFlags.EnumValuesComputed;
let autoValue: number | undefined = 0;
+ let previous: EnumMember | undefined;
for (const member of node.members) {
- const value = computeMemberValue(member, autoValue);+ const value = computeMemberValue(member, autoValue, previous);
getNodeLinks(member).enumMemberValue = value;
autoValue = typeof value === "number" ? value + 1 : undefined;
+ previous = member;
}
}
}
- function computeMemberValue(member: EnumMember, autoValue: number | undefined) {+ function computeMemberValue(member: EnumMember, autoValue: number | undefined, previous: EnumMember | undefined) {
if (isComputedNonLiteralName(member.name)) {
error(member.name, Diagnostics.Computed_property_names_are_not_allowed_in_enums);
}
@@ -45040,26 +45042,41 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// If the member is the first member in the enum declaration, it is assigned the value zero.
// Otherwise, it is assigned the value of the immediately preceding member plus one, and an error
// occurs if the immediately preceding member is not a constant enum member.
- if (autoValue !== undefined) {- return autoValue;- }+ if (autoValue === undefined) {
error(member.name, Diagnostics.Enum_member_must_have_initializer);
return undefined;
}
+ if (getIsolatedModules(compilerOptions) && previous?.initializer) {+ const previousResult = evaluate(previous.initializer);+ if (typeof previousResult.value !== "number" || !previousResult.syntactic) {+ error(+ member.name,+ Diagnostics.Enum_member_following_a_non_literal_numeric_member_must_have_an_initializer_when_isolatedModules_is_enabled,+ );+ }+ }+ return autoValue;+ }
function computeConstantValue(member: EnumMember): string | number | undefined {
const isConstEnum = isEnumConst(member.parent);
const initializer = member.initializer!;
- const value = evaluate(initializer, member);- if (value !== undefined) {- if (isConstEnum && typeof value === "number" && !isFinite(value)) {+ const result = evaluate(initializer, member);+ if (result.value !== undefined) {+ if (isConstEnum && typeof result.value === "number" && !isFinite(result.value)) {
error(
initializer,
- isNaN(value) ?+ isNaN(result.value) ?
Diagnostics.const_enum_member_initializer_was_evaluated_to_disallowed_value_NaN :
Diagnostics.const_enum_member_initializer_was_evaluated_to_a_non_finite_value,
);
}
+ else if (getIsolatedModules(compilerOptions) && typeof result.value === "string" && !result.syntactic) {+ error(+ initializer,+ Diagnostics.A_string_member_initializer_in_a_enum_declaration_can_only_use_constant_expressions_when_isolatedModules_is_enabled,+ );+ }
}
else if (isConstEnum) {
error(initializer, Diagnostics.const_enum_member_initializers_must_be_constant_expressions);
@@ -45070,13 +45087,23 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
else {
checkTypeAssignableTo(checkExpression(initializer), numberType, initializer, Diagnostics.Type_0_is_not_assignable_to_type_1_as_required_for_computed_enum_member_values);
}
- return value;+ return result.value;+ }++ interface EvaluationResult<T> {+ value: T;+ syntactic: boolean;
}
- function evaluate(expr: Expression, location?: Declaration): string | number | undefined {+ function evaluate(expr: Expression, location?: Declaration): EvaluationResult<string | number | undefined> {+ let syntactic = true;+ let value = evaluateInternal(expr, location);+ return {value, syntactic};++ function evaluateInternal(expr: Expression, location?: Declaration): string | number | undefined {
switch (expr.kind) {
case SyntaxKind.PrefixUnaryExpression:
- const value = evaluate((expr as PrefixUnaryExpression).operand, location);+ const value = evaluateInternal((expr as PrefixUnaryExpression).operand, location);
if (typeof value === "number") {
switch ((expr as PrefixUnaryExpression).operator) {
case SyntaxKind.PlusToken:
@@ -45089,8 +45116,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
break;
case SyntaxKind.BinaryExpression:
- const left = evaluate((expr as BinaryExpression).left, location);- const right = evaluate((expr as BinaryExpression).right, location);+ const left = evaluateInternal((expr as BinaryExpression).left, location);+ const right = evaluateInternal((expr as BinaryExpression).right, location);
if (typeof left === "number" && typeof right === "number") {
switch ((expr as BinaryExpression).operatorToken.kind) {
case SyntaxKind.BarToken:
@@ -45131,13 +45158,16 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
case SyntaxKind.NoSubstitutionTemplateLiteral:
return (expr as StringLiteralLike).text;
case SyntaxKind.TemplateExpression:
- return evaluateTemplateExpression(expr as TemplateExpression, location);+ const templateResult = evaluateTemplateExpression(expr as TemplateExpression, location);+ syntactic &&= templateResult.syntactic;+ return templateResult.value;
case SyntaxKind.NumericLiteral:
checkGrammarNumericLiteral(expr as NumericLiteral);
return +(expr as NumericLiteral).text;
case SyntaxKind.ParenthesizedExpression:
- return evaluate((expr as ParenthesizedExpression).expression, location);+ return evaluateInternal((expr as ParenthesizedExpression).expression, location);
case SyntaxKind.Identifier: {
+ syntactic = false;
const identifier = expr as Identifier;
if (isInfinityOrNaNString(identifier.escapedText) && (resolveEntityName(identifier, SymbolFlags.Value, /*ignoreErrors*/ true) === getGlobalSymbol(identifier.escapedText, SymbolFlags.Value, /*diagnostic*/ undefined))) {
return +(identifier.escapedText);
@@ -45145,6 +45175,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
// falls through
}
case SyntaxKind.PropertyAccessExpression:
+ syntactic = false;
if (isEntityNameExpression(expr)) {
const symbol = resolveEntityName(expr, SymbolFlags.Value, /*ignoreErrors*/ true);
if (symbol) {
@@ -45154,13 +45185,14 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
if (isConstantVariable(symbol)) {
const declaration = symbol.valueDeclaration;
if (declaration && isVariableDeclaration(declaration) && !declaration.type && declaration.initializer && (!location || declaration !== location && isBlockScopedNameDeclaredBeforeUse(declaration, location))) {
- return evaluate(declaration.initializer, declaration);+ return evaluateInternal(declaration.initializer, declaration);
}
}
}
}
break;
case SyntaxKind.ElementAccessExpression:
+ syntactic = false;
const root = (expr as ElementAccessExpression).expression;
if (isEntityNameExpression(root) && isStringLiteralLike((expr as ElementAccessExpression).argumentExpression)) {
const rootSymbol = resolveEntityName(root, SymbolFlags.Value, /*ignoreErrors*/ true);
@@ -45176,6 +45208,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
return undefined;
}
+ }
function evaluateEnumMember(expr: Expression, symbol: Symbol, location: Declaration) {
const declaration = symbol.valueDeclaration;
@@ -45190,17 +45223,19 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return getEnumMemberValue(declaration as EnumMember);
}
- function evaluateTemplateExpression(expr: TemplateExpression, location?: Declaration) {- let result = expr.head.text;+ function evaluateTemplateExpression(expr: TemplateExpression, location?: Declaration): EvaluationResult<string | undefined> {+ let value = expr.head.text;+ let syntactic = true;
for (const span of expr.templateSpans) {
- const value = evaluate(span.expression, location);- if (value === undefined) {- return undefined;+ const evaluateResult = evaluate(span.expression, location);+ syntactic &&= evaluateResult.syntactic;+ if (evaluateResult.value === undefined) {+ return {value: undefined, syntactic};
}
- result += value;- result += span.literal.text;+ value += evaluateResult.value;+ value += span.literal.text;
}
- return result;+ return {value, syntactic};
}
function checkEnumDeclaration(node: EnumDeclaration) { |
Andrew Branch (andrewbranch)
commented
Jan 24, 2024
Yeah, sorry for the delay. I was holding off close to the 5.4 beta when I noticed that there would be significant merge conflicts with #53463, but it turned out that didn’t go into the beta either. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Jan Kühle (frigus02)
commented
Mar 7, 2024
Hi Andrew Branch (@andrewbranch). Congrats to the TS 5.4 release! Do you think we can merge some version of this PR in time for 5.5? 😊 |
Jan Kühle (frigus02)
commented
Mar 15, 2024
Thank you for #57686, Andrew. I update the PR and it's much smaller now. |
Uh oh!
There was an error while loading. Please reload this page.
| "category": "Error", | ||
| "code": 18054 | ||
| }, | ||
| "A string member initializer in a enum declaration can only use constant expressions when 'isolatedModules' is enabled.": { |
There was a problem hiding this comment.
Paraphrasing a suggestion from Daniel Rosenwasser (@DanielRosenwasser):
| "A string member initializer in a enum declaration can only use constant expressions when 'isolatedModules' is enabled.": { | |
| "'{0}' has a string type, but must have syntactically recognizable string syntax when 'isolatedModules' is enabled.": { |
We could totally do a quick fix to wrap it in a template literal but I think this error is going to be so niche that I’m not sure it’s worth it.
There was a problem hiding this comment.
I like the suggested error message. But what do I substitute {0} with? The initializer can be a relatively complex expression. Is there a utility function to stringify that? Or should I use the enum member name or even the computed value?
There was a problem hiding this comment.
I forgot to include that; Daniel’s suggestion was 'EnumName.MemberName'
There was a problem hiding this comment.
Sounds good. Do we want to handle member names that are not valid identifier? E.g.:
import{bar}from'./bar';enumFoo{['not an identifier']=bar}// 'Foo.not an identifier' has a string type, ...// 'Foo["not an identifier"]' has a string type, ...There was a problem hiding this comment.
I updated the error message and ignored the non-identifier case for now, because it seems like an extremely rare case. That means it would print 'Foo.not an identifier' in the error message.
Andrew Branch (andrewbranch)
commented
Mar 19, 2024
TypeScript Bot (@typescript-bot) test top200 |
TypeScript Bot (typescript-bot)
commented
Mar 19, 2024
Andrew Branch (@andrewbranch) Here are the results of running the top-repos suite comparing Everything looks good! |
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
Thank you for your patience Jan Kühle (@frigus02)!
Fixes#56153
Adds 2 new errors under
isolatedModulesonly:Ran the new checks across our code base. I wanted to check if we need to support computed values based on local identifiers. But we have so few errors, I don't think this is necessary: