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.

Sign up for GitHub

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

Use evaluator for isolatedModules enum restrictions #57966

Merged
merged 6 commits into from
Apr 4, 2024
+400 −137
Merged

Use evaluator for isolatedModules enum restrictions #57966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b90b2d8
Use evaluator for isolatedModules enum restrictions
andrewbranch Mar 27, 2024
79b5ff1
Merge branch 'main' into bug/isolated-modules-enum-restrictions
andrewbranch Mar 27, 2024
392976d
Roll back outer expression skipping
andrewbranch Mar 27, 2024
ed0ea55
Merge branch 'main' into bug/isolated-modules-enum-restrictions
andrewbranch Mar 27, 2024
2035fc6
Update test
andrewbranch Mar 27, 2024
4f69ba7
Remove type-only ipmorts
andrewbranch Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 56 additions & 47 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ import {
equateValues,
escapeLeadingUnderscores,
escapeString,
EvaluatorResult,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit but we haven't really used type imports anywhere else quite yet (besides that protocol.ts blunder); I don't think we've set the settings in settings.json yet that would allow sorting to be inline either... Maybe we just avoid type imports until we can switch them all?

Copy link
Member Author

@andrewbranch andrewbranch Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine with me, but this is what auto-imports did automatically 😬 My bad, that’s my own setting; I guess I was testing it a while back. It works!

evaluatorResult,
every,
EvolvingArrayType,
ExclamationToken,
Expand Down Expand Up @@ -723,7 +725,6 @@ import {
isStringOrNumericLiteralLike,
isSuperCall,
isSuperProperty,
isSyntacticallyString,
isTaggedTemplateExpression,
isTemplateSpan,
isThisContainerOrFunctionBlock,
Expand Down Expand Up @@ -12764,7 +12765,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
for (const member of (declaration as EnumDeclaration).members) {
if (hasBindableName(member)) {
const memberSymbol = getSymbolOfDeclaration(member);
const value = getEnumMemberValue(member);
const value = getEnumMemberValue(member).value;
const memberType = getFreshTypeOfLiteralType(
value !== undefined ?
getEnumLiteralType(value, getSymbolId(symbol), memberSymbol) :
Expand Down Expand Up @@ -21185,8 +21186,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
return false;
}
const sourceValue = getEnumMemberValue(getDeclarationOfKind(sourceProperty, SyntaxKind.EnumMember)!);
const targetValue = getEnumMemberValue(getDeclarationOfKind(targetProperty, SyntaxKind.EnumMember)!);
const sourceValue = getEnumMemberValue(getDeclarationOfKind(sourceProperty, SyntaxKind.EnumMember)!).value;
const targetValue = getEnumMemberValue(getDeclarationOfKind(targetProperty, SyntaxKind.EnumMember)!).value;
if (sourceValue !== targetValue) {
const sourceIsString = typeof sourceValue === "string";
const targetIsString = typeof targetValue === "string";
Expand Down Expand Up @@ -39336,7 +39337,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 && evaluate(node);
const evaluated = node.parent.kind !== SyntaxKind.TaggedTemplateExpression && evaluate(node).value;
return evaluated ? getFreshTypeOfLiteralType(getStringLiteralType(evaluated)) : stringType;
}

Expand Down Expand Up @@ -45745,15 +45746,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let autoValue: number | undefined = 0;
let previous: EnumMember | undefined;
for (const member of node.members) {
const value = computeMemberValue(member, autoValue, previous);
getNodeLinks(member).enumMemberValue = value;
autoValue = typeof value === "number" ? value + 1 : undefined;
const result = computeEnumMemberValue(member, autoValue, previous);
getNodeLinks(member).enumMemberValue = result;
autoValue = typeof result.value === "number" ? result.value + 1 : undefined;
previous = member;
}
}
}

function computeMemberValue(member: EnumMember, autoValue: number | undefined, previous: EnumMember | undefined) {
function computeEnumMemberValue(member: EnumMember, autoValue: number | undefined, previous: EnumMember | undefined): EvaluatorResult {
if (isComputedNonLiteralName(member.name)) {
error(member.name, Diagnostics.Computed_property_names_are_not_allowed_in_enums);
}
Expand All @@ -45764,44 +45765,47 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}
}
if (member.initializer) {
return computeConstantValue(member);
return computeConstantEnumMemberValue(member);
}
// In ambient non-const numeric enum declarations, enum members without initializers are
// considered computed members (as opposed to having auto-incremented values).
if (member.parent.flags & NodeFlags.Ambient && !isEnumConst(member.parent)) {
return undefined;
return evaluatorResult(/*value*/ undefined);
}
// If the member declaration specifies no value, the member is considered a constant enum member.
// 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) {
error(member.name, Diagnostics.Enum_member_must_have_initializer);
return undefined;
return evaluatorResult(/*value*/ undefined);
}
if (getIsolatedModules(compilerOptions) && previous?.initializer && !isSyntacticallyNumericConstant(previous.initializer)) {
error(
member.name,
Diagnostics.Enum_member_following_a_non_literal_numeric_member_must_have_an_initializer_when_isolatedModules_is_enabled,
);
if (getIsolatedModules(compilerOptions) && previous?.initializer) {
const prevValue = getEnumMemberValue(previous);
if (!(typeof prevValue.value === "number" && !prevValue.resolvedOtherFiles)) {
error(
member.name,
Diagnostics.Enum_member_following_a_non_literal_numeric_member_must_have_an_initializer_when_isolatedModules_is_enabled,
);
}
}
return autoValue;
return evaluatorResult(autoValue);
}

function computeConstantValue(member: EnumMember): string | number | undefined {
function computeConstantEnumMemberValue(member: EnumMember): EvaluatorResult {
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 value === "string" && !isSyntacticallyString(initializer)) {
else if (getIsolatedModules(compilerOptions) && typeof result.value === "string" && !result.isSyntacticallyString) {
error(
initializer,
Diagnostics._0_has_a_string_type_but_must_have_syntactically_recognizable_string_syntax_when_isolatedModules_is_enabled,
Expand All @@ -45818,30 +45822,20 @@ 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;
}

function isSyntacticallyNumericConstant(expr: Expression): boolean {
expr = skipOuterExpressions(expr);
switch (expr.kind) {
case SyntaxKind.PrefixUnaryExpression:
return isSyntacticallyNumericConstant((expr as PrefixUnaryExpression).operand);
case SyntaxKind.BinaryExpression:
return isSyntacticallyNumericConstant((expr as BinaryExpression).left) && isSyntacticallyNumericConstant((expr as BinaryExpression).right);
case SyntaxKind.NumericLiteral:
return true;
}
return false;
return result;
}

function evaluateEntityNameExpression(expr: EntityNameExpression, location?: Declaration) {
const symbol = resolveEntityName(expr, SymbolFlags.Value, /*ignoreErrors*/ true);
if (!symbol) return undefined;
if (!symbol) return evaluatorResult(/*value*/ undefined);

if (expr.kind === SyntaxKind.Identifier) {
const identifier = expr;
if (isInfinityOrNaNString(identifier.escapedText) && (symbol === getGlobalSymbol(identifier.escapedText, SymbolFlags.Value, /*diagnostic*/ undefined))) {
return +(identifier.escapedText);
// Technically we resolved a global lib file here, but the decision to treat this as numeric
// is more predicated on the fact that the single-file resolution *didn't* resolve to a
// different meaning of `Infinity` or `NaN`. Transpilers handle this no problem.
return evaluatorResult(+(identifier.escapedText), /*isSyntacticallyString*/ false);
}
}

Expand All @@ -45851,9 +45845,18 @@ 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);
const result = evaluate(declaration.initializer, declaration);
if (location && getSourceFileOfNode(location) !== getSourceFileOfNode(declaration)) {
return evaluatorResult(
result.value,
/*isSyntacticallyString*/ false,
/*resolvedOtherFiles*/ true,
);
}
return result;
}
}
return evaluatorResult(/*value*/ undefined);
}

function evaluateElementAccessExpression(expr: ElementAccessExpression, location?: Declaration) {
Expand All @@ -45864,21 +45867,23 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const name = escapeLeadingUnderscores(expr.argumentExpression.text);
const member = rootSymbol.exports!.get(name);
if (member) {
Debug.assert(getSourceFileOfNode(member.valueDeclaration) === getSourceFileOfNode(rootSymbol.valueDeclaration));
return location ? evaluateEnumMember(expr, member, location) : getEnumMemberValue(member.valueDeclaration as EnumMember);
}
}
}
return evaluatorResult(/*value*/ undefined);
}

function evaluateEnumMember(expr: Expression, symbol: Symbol, location: Declaration) {
const declaration = symbol.valueDeclaration;
if (!declaration || declaration === location) {
error(expr, Diagnostics.Property_0_is_used_before_being_assigned, symbolToString(symbol));
return undefined;
return evaluatorResult(/*value*/ undefined);
}
if (!isBlockScopedNameDeclaredBeforeUse(declaration, location)) {
error(expr, Diagnostics.A_member_initializer_in_a_enum_declaration_cannot_reference_members_declared_after_it_including_members_defined_in_other_enums);
return 0;
return evaluatorResult(/*value*/ 0);
}
return getEnumMemberValue(declaration as EnumMember);
}
Expand Down Expand Up @@ -48510,9 +48515,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return nodeLinks[nodeId]?.flags || 0;
}

function getEnumMemberValue(node: EnumMember): string | number | undefined {
function getEnumMemberValue(node: EnumMember): EvaluatorResult {
computeEnumMemberValues(node.parent);
return getNodeLinks(node).enumMemberValue;
return getNodeLinks(node).enumMemberValue ?? evaluatorResult(/*value*/ undefined);
}

function canHaveConstantValue(node: Node): node is EnumMember | AccessExpression {
Expand All @@ -48527,15 +48532,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {

function getConstantValue(node: EnumMember | AccessExpression): string | number | undefined {
if (node.kind === SyntaxKind.EnumMember) {
return getEnumMemberValue(node);
return getEnumMemberValue(node).value;
}

const symbol = getNodeLinks(node).resolvedSymbol;
if (symbol && (symbol.flags & SymbolFlags.EnumMember)) {
// inline property\index accesses only for const enums
const member = symbol.valueDeclaration as EnumMember;
if (isEnumConst(member.parent)) {
return getEnumMemberValue(member);
return getEnumMemberValue(member).value;
}
}

Expand Down Expand Up @@ -48896,6 +48901,10 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const node = getParseTreeNode(nodeIn, canHaveConstantValue);
return node ? getConstantValue(node) : undefined;
},
getEnumMemberValue: nodeIn => {
const node = getParseTreeNode(nodeIn, isEnumMember);
return node ? getEnumMemberValue(node) : undefined;
},
collectLinkedAliases,
getReferencedValueDeclaration,
getReferencedValueDeclarations,
Expand Down
1 change: 1 addition & 0 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,6 +1103,7 @@ export const notImplementedResolver: EmitResolver = {
isEntityNameVisible: notImplemented,
// Returns the constant value this property access resolves to: notImplemented, or 'undefined' for a non-constant
getConstantValue: notImplemented,
getEnumMemberValue: notImplemented,
getReferencedValueDeclaration: notImplemented,
getReferencedValueDeclarations: notImplemented,
getTypeReferenceSerializationKind: notImplemented,
Expand Down
17 changes: 8 additions & 9 deletions src/compiler/transformers/ts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ import {
isSimpleInlineableExpression,
isSourceFile,
isStatement,
isSyntacticallyString,
isTemplateLiteral,
isTryStatement,
JsxOpeningElement,
Expand Down Expand Up @@ -1915,15 +1914,16 @@ export function transformTypeScript(context: TransformationContext) {
// we pass false as 'generateNameForComputedPropertyName' for a backward compatibility purposes
// old emitter always generate 'expression' part of the name as-is.
const name = getExpressionForPropertyName(member, /*generateNameForComputedPropertyName*/ false);
const valueExpression = transformEnumMemberDeclarationValue(member);
const evaluated = resolver.getEnumMemberValue(member);
const valueExpression = transformEnumMemberDeclarationValue(member, evaluated?.value);
const innerAssignment = factory.createAssignment(
factory.createElementAccessExpression(
currentNamespaceContainerName,
name,
),
valueExpression,
);
const outerAssignment = isSyntacticallyString(valueExpression) ?
const outerAssignment = typeof evaluated?.value === "string" || evaluated?.isSyntacticallyString ?
innerAssignment :
factory.createAssignment(
factory.createElementAccessExpression(
Expand All @@ -1948,12 +1948,11 @@ export function transformTypeScript(context: TransformationContext) {
*
* @param member The enum member node.
*/
function transformEnumMemberDeclarationValue(member: EnumMember): Expression {
const value = resolver.getConstantValue(member);
if (value !== undefined) {
return typeof value === "string" ? factory.createStringLiteral(value) :
value < 0 ? factory.createPrefixUnaryExpression(SyntaxKind.MinusToken, factory.createNumericLiteral(-value)) :
factory.createNumericLiteral(value);
function transformEnumMemberDeclarationValue(member: EnumMember, constantValue: string | number | undefined): Expression {
if (constantValue !== undefined) {
return typeof constantValue === "string" ? factory.createStringLiteral(constantValue) :
constantValue < 0 ? factory.createPrefixUnaryExpression(SyntaxKind.MinusToken, factory.createNumericLiteral(-constantValue)) :
factory.createNumericLiteral(constantValue);
}
else {
enableSubstitutionForNonQualifiedEnumMembers();
Expand Down
14 changes: 11 additions & 3 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5626,6 +5626,7 @@ export interface EmitResolver {
isEntityNameVisible(entityName: EntityNameOrEntityNameExpression, enclosingDeclaration: Node): SymbolVisibilityResult;
// Returns the constant value this property access resolves to, or 'undefined' for a non-constant
getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): string | number | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still need to have EnumMember?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not, but the same signature is public API on TypeChecker so maybe it’s best not to change it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, declarations.ts still calls getConstantValue but only on an enum value, which makes me think that the resolver wouldn't need that func at all anymore. But, ts.ts's tryGetConstEnumValue still calls it for property access / element accesses :(

getEnumMemberValue(node: EnumMember): EvaluatorResult | undefined;
getReferencedValueDeclaration(reference: Identifier): Declaration | undefined;
getReferencedValueDeclarations(reference: Identifier): Declaration[] | undefined;
getTypeReferenceSerializationKind(typeName: EntityName, location?: Node): TypeReferenceSerializationKind;
Expand Down Expand Up @@ -5958,6 +5959,13 @@ export const enum NodeCheckFlags {
InCheckIdentifier = 1 << 22,
}

/** @internal */
export interface EvaluatorResult<T extends string | number | undefined = string | number | undefined> {
value: T;
isSyntacticallyString: boolean;
resolvedOtherFiles: boolean;
}

// dprint-ignore
/** @internal */
export interface NodeLinks {
Expand All @@ -5968,7 +5976,7 @@ export interface NodeLinks {
resolvedSymbol?: Symbol; // Cached name resolution result
resolvedIndexInfo?: IndexInfo; // Cached indexing info resolution result
effectsSignature?: Signature; // Signature with possible control flow effects
enumMemberValue?: string | number; // Constant value of enum member
enumMemberValue?: EvaluatorResult; // Constant value of enum member
isVisible?: boolean; // Is this node visible
containsArgumentsReference?: boolean; // Whether a function-like declaration contains an 'arguments' reference
hasReportedStatementInAmbientContext?: boolean; // Cache boolean if we report statements in ambient context
Expand Down Expand Up @@ -10074,6 +10082,6 @@ export interface Queue<T> {

/** @internal */
export interface EvaluationResolver {
evaluateEntityNameExpression(expr: EntityNameExpression, location: Declaration | undefined): string | number | undefined;
evaluateElementAccessExpression(expr: ElementAccessExpression, location: Declaration | undefined): string | number | undefined;
evaluateEntityNameExpression(expr: EntityNameExpression, location: Declaration | undefined): EvaluatorResult;
evaluateElementAccessExpression(expr: ElementAccessExpression, location: Declaration | undefined): EvaluatorResult;
}
Loading