Skip to content

Element type of enum should include enum type #55713

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

Closed
cpcallen opened this issue Sep 11, 2023 · 6 comments
Closed

Element type of enum should include enum type #55713

cpcallen opened this issue Sep 11, 2023 · 6 comments
Labels
Duplicate An existing issue was already created

Comments

@cpcallen
Copy link

cpcallen commented Sep 11, 2023

🔎 Search Terms

enum element type computed index reverse lookup

🕗 Version & Regression Information

  • This is the behaviour in every version I tried, and I reviewed the FAQ for entries about enums

⏯ Playground Link

https://www.typescriptlang.org/play?#code/KYOwrgtgBAolDeBBAvgbgFDoGYHsBOUAFAMY4gDOALlANbACeUAliLAJQLpTdQA2w1APqCoAXlgBtOvQC6GHlAD0i7gD0A-Fx6kKOfgDpeOAOaFK9AA7AcWKMLYZkQA

💻 Code

enum E {A};

for (const key in E) {
    let __ = E[key];
    //  ^?
    console.log(typeof __);
}

🙁 Actual behaviour

The (tsc-reported) type of __ is string

🙂 Expected behaviour

The (tsc-reported) type of __ should be 'A' | E, or at least string | number`.

Additional information about the issue

This issue singles out one of several different problems discussed in #39627.

See also #39627 and #42457, which discuss iterating over enum entries in different contexts.

Background

We are migrating a (Closure type system) JavaScript library to TypeScript.

In times past our code looked like:

/** @class */
function C() { /* ... */ }

C.prototype.E_A = 0;
C.prototype.E_B = 1;

In preparation for the migration to TypeScript, and since the E_x properties are used as an enum, we first converted them to an @enum but included code to maintain backwards compatibility as follows:

/** @enum */
const E = {
    A: 0,
    B: 1,
};

class C{
  constructor () {
    for (const key in E) {
       this['E_' + key] = E[key];
    };
  }
}

My initial naïve conversion to TypeScript:

enum E {
    A,
    B,
};

class C {
  constructor () {
    // Copy enum values onto this for backwards-compatibility:
    for (const key in E) {
       this['E_' + key] = E[key];
    };
  }
}

helpfully elicited the (correct and useful) error "Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'C'. No index signature with a parameter of type 'string' was found on type 'C'."

Since this is only for backwards compatibility with legacy client code I don't wish to add an index signature to type C (new client code written in TypeScript should use the enum directly), so instead I attempted to simply coerce the type of this to a allow the assignments:

    for (const key in E) {
       (this as unknown as Record<string, number>)['E_' + key] = E[key];
    };

This produces the error message "Type 'string' is not assignable to type 'number'.(2322)", which is useful, insofar as it (eventually) alerted me tho the change of semantics of E, which now also includes the reverse-lookup table, but it is incorrect because the actual type of E[key] should be something like (keyof typeof E) | E, or at least string | E (or failing that even just string | number)—but definitely not just string.

This inspecificity of the type of an enum when using it for reverse lookups has previously been the subject of issues #38806 and #50933, but those issues do not highlight the fact that the type only supports reverse lookups.

Surprisingly, adding a guard clause is not sufficient to satisfy the type checker:

    for (const key in E) {
       if (typeof E[key] === 'string') continue;
       (this as unknown as Record<string, number>)['E_' + key] = E[key];
    };

This still complains that E[key] is a string even though string has been explicitly eliminated. [Update: this turns out to be due to #10530—or rather any of its many supposed duplicates, since the specific original example reported there has been fixed.]

Adding a temporary variable however does work:

    for (const key in E) {
       const value = E[key];
       if (typeof value === 'string') continue;
       (this as unknown as Record<string, number>)['E_' + key] = value;
    };

because tsc 'correctly' deduces that value has type never. It is not clear why it does not make the same deduction about E[key] in the previous snippet.

@andrewbranch
Copy link
Member

Duplicate of #39627. Simpler repro:

enum E {A};

for (const key in E) {
    let __ = E[key];
    //  ^?
    console.log(typeof __);
}

https://www.typescriptlang.org/play?#code/KYOwrgtgBAolDeBBAvgbgFDoGYHsBOUAFAMY4gDOALlANbACeUAliLAJQLpTdQA2w1APqCoAXlgBtOvQC6GHlAD0i7gD0A-Fx6kKOfgDpeOAOaFK9AA7AcWKMLYZkQA

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 11, 2023
@fatcerberus
Copy link

Isn’t this just another manifestation of #10530 or am I misreading the issue? Because according to the OP narrowing E[key] doesn’t work (which is expected since key isn’t a literal) but assigning to a temp variable first does.

@andrewbranch
Copy link
Member

andrewbranch commented Sep 11, 2023

No, because E[key] is just always string; everything after that is kind of a red herring.

Edit: well, that’s the answer to

It is not clear why it does not make the same deduction about E[key] in the previous snippet

and I guess #10530 does stand in the way to making the original work as intended, but I feel like the primary thing going on is the lack of string | E type in the first place.

@cpcallen cpcallen changed the title Computed index lookup on enum should not be assumed to be a reverse lookup Element type of enum should include enum type Sep 12, 2023
@cpcallen
Copy link
Author

Duplicate of #39627.

I had a good look at that issue before filing this one—apologies for not citing it—but it appeared to be:

  • Reporting that the reverse mapping is surprising and unexpected,
  • Discussing the demerits of mixed string/numeric enums, and
  • Reporting that noImplicityAny unexpectedly causes a compile error in the all-string enum case.

…but upon more careful re-reading I see that amongst those other topics it does indeed discuss the question of the element type of numeric-only enums:

  1. With enums that have at least one numeric member (Test1 and Test2), the inferred type of loop variable (key) should ideally be the union of all keys and numeric values or at least string | number, not just string.

Isn’t this just another manifestation of #10530 or am I misreading the issue?Because according to the OP narrowing E[key] doesn’t work (which is expected since key isn’t a literal) but assigning to a temp variable first does.

Thanks for pointing to that issue, though I see that it is a bit underspecified: the actual reported issue has been fixed. You are correct in observing that part of my problem is due to the issue reported in many of the bugs closed as duplicates of that one, however.

I feel like the primary thing going on is the lack of string | E type in the first place.

Agreed, and I have updated the title and description of this one accordingly.

@fatcerberus
Copy link

Thanks for pointing to that issue, though I see that it is a bit underspecified: the actual reported issue has been fixed.

Yep, I’m aware. Even the maintainers tend to dupe to that one, and I’ve voiced my opinion a few times that I wish there was a more accurate dupe, as the obj[key] narrowing problem comes up quite often—almost as often as 9998 dupes 😅

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

4 participants