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

#24210: Updated handling of undefined values in Enum types #39390

Closed
wants to merge 2 commits into from

Conversation

ggajos
Copy link

@ggajos ggajos commented Jul 2, 2020

Fixes #24210

I've managed to identify two problems in #24210.

  1. The magical 0.

This is produced in the code below, along with the cryptic exception. However, I believe that this error should still be produced when user provided some wierd value like "a" - "a".

            else if (enumKind === EnumKind.Literal) {
                error(initializer, Diagnostics.Computed_values_are_not_permitted_in_an_enum_with_string_valued_members);
                return 0;
            }

I've added additional clause to exclude the undefined from this logic. I've targeted the undefined keyword very specifically to avoid cases when something is evaluated to undefined (limit regression).

  1. Prefixing in substitution phase

The substitution phase is trying to add Enum name prefix (as it is doing for combining multiple enum members). Effectively converting Identifier to PropertyAccessExpression. In other words, converting undefined to EnumName.undefined. While this make sense in some cases, I believe that undefined should have higher precedence here.

I've added clause in transformer.ts::trySubstituteNamespaceExportedName to exclude the undefined from this processing.

@ghost
Copy link

ghost commented Jul 2, 2020

CLA assistant check
All CLA requirements met.

@sandersn sandersn self-requested a review September 2, 2020 23:54
@sandersn sandersn self-assigned this Sep 2, 2020
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 2, 2020
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I'm trying to clear out our PR backlog and finally reviewed this. Of course, I understand if you are not interested in finishing it.

@@ -34744,6 +34744,9 @@ namespace ts {
Diagnostics.const_enum_member_initializer_was_evaluated_to_a_non_finite_value);
}
}
else if (initializer.kind === SyntaxKind.Identifier && (initializer as Identifier).escapedText === "undefined") {
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be an error here? I thought undefined was supposed to be an error, but with an undefined-specific message.

}

enum TeInf {
"Infinity" = Infinity
Copy link
Member

Choose a reason for hiding this comment

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

Are Infinity and NaN supposed to be errors or not? Looking at the previous discussion, it seems only intended to be an error for const enum.

})(TeNaN || (TeNaN = {}));
var TeCornerValues;
(function (TeCornerValues) {
TeCornerValues[TeCornerValues["undefined"] = undefined] = "undefined";
Copy link
Member

Choose a reason for hiding this comment

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

undefined/NaN/Infinity are still turned into quoted strings. That's not correct, is it?

@sandersn
Copy link
Member

sandersn commented Jun 1, 2022

To help with PR housekeeping, I'm going to close this PR since it's pretty old now.

@sandersn sandersn closed this Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bad error message for enum initializers
3 participants