Skip to content

Computed Properties aren't bound correctly during Object/Class evaluation #27864

@joeywatts

Description

@joeywatts
Contributor

TypeScript Version: 3.2.0-dev.20181011

Search Terms: computed property expression

Code

const classes = [];
for (let i = 0; i <= 10; ++i) {
  classes.push(
    class A {
      [i] = "my property";
    }
  );
}
for (const clazz of classes) {
  console.log(Object.getOwnPropertyNames(new clazz()));
}

Expected behavior: The log statements indicate that each class in classes has a different property name (i should be evaluated at the time of the class evaluation and all instances of that class should have a property name corresponding to that evaluation of i).

Actual behavior: Compiled code logs:

[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]
[ '10' ]

Playground Link

Activity

mheiber

mheiber commented on Oct 12, 2018

@mheiber
Contributor

There's a type error on the line with the computed property:

A computed property name in a class property declaration must refer to an expression whose type is a literal type or a 'unique symbol' type.

However

  • It's still possible to produce correct emit in this case, even though there's a type error (if that's desirable)
  • The class fields proposal allows code like in the example. Chrome Canary implements the spec and produces the expected output for the example code.
ghost added
BugA bug in TypeScript
on Oct 12, 2018
ghost added on Oct 12, 2018
added
Effort: ModerateRequires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual".
on Oct 12, 2018
DanielRosenwasser

DanielRosenwasser commented on Oct 12, 2018

@DanielRosenwasser
Member

Taking PRs and marking it as moderate (assuming you are familiar with spec-reading and the transform pipeline).

mheiber

mheiber commented on Oct 13, 2018

@mheiber
Contributor

What are the implications of getting rid of the type error? Will allowing arbitrary string and symbol values in computed field declarations affect inference?

Neuroboy23

Neuroboy23 commented on Oct 16, 2018

@Neuroboy23

Type errors aside, here's one possible emit that works correctly. I haven't seen any other cases of invoking an IIFE with parameters like this. Are there any?

var classes = [];
for (var i = 0; i <= 10; ++i) {
    classes.push(
        function(i) {
            function A() {
                this[i] = "my property";
            }
            return A;
        }(i)
    );
}
mheiber

mheiber commented on Oct 18, 2018

@mheiber
Contributor

Maybe we can lean on the iife stuff that's already done to transpile let and const.

The currently generated code for Joey's example when targeting ES2015 is:

var _a, _b;
"use strict";
const classes = [];
for (let i = 0; i <= 10; ++i) {
    classes.push((_b = class A {
            constructor() {
                this[_a] = "my property";
            }
        },
        _a = i,
        _b));
}
for (const clazz of classes) {
    console.log(Object.getOwnPropertyNames(new clazz()));
}

If we move _a and _b into the containing scope of the class and use let then the output is correct:

"use strict";
const classes = [];
for (let i = 0; i <= 10; ++i) {
    let _a, _b; // <---- was moved
    classes.push((_b = class A {
            constructor() {
                this[_a] = "my property";
            }
        },
        _a = i,
        _b));
}
for (const clazz of classes) {
    console.log(Object.getOwnPropertyNames(new clazz()));
}

Update: would this be a big architectural change? If I understand correctly, lexicalEnvironmentStack is used in hoisting declarations, and is defined in transformer.ts, rather than in a particular transformer.

Kingwl

Kingwl commented on Oct 23, 2018

@Kingwl
Contributor

the special temporary variable and hoist is happened in ts transformer before es2015
perhaps we need a block level hoist?

added a commit that references this issue on Oct 26, 2018
db524fd
mheiber

mheiber commented on Oct 26, 2018

@mheiber
Contributor

@Kingwl agreed. we're trying that approach in joeywatts#15
used that approach in #28708

22 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    BugA bug in TypeScriptDomain: TransformsRelates to the public transform APIEffort: ModerateRequires experience with the TypeScript codebase, but feasible. Harder than "Effort: Casual".Help WantedYou can do this

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Participants

      @DanielRosenwasser@joeywatts@rbuckton@xialvjun@mheiber

      Issue actions

        Computed Properties aren't bound correctly during Object/Class evaluation · Issue #27864 · microsoft/TypeScript