Improving the AST for computed-name properties

[bradzacher]

One of the more difficult pieces of the AST to work with is properties. Why? Because computed-name properties and statically-named properties are a single AST node.

This is a huge footgun that most people don't think about when writing static analysis code or transforms on top of the AST. I see a lot of code which looks like this:

function analyseProperty(prop: Property) {
  if (property.key.type === 'Identifier') {
    analyseStaticPropertyName(property.key);
  }
}

This code looks correct at a quick glance, until you realise that if prop.computed === true, then an Identifier has a very different meaning:

const prop = 1;
const x = {
  prop: 2,   // declares a property named "prop" with the value 2
  [prop]: 2, // declares a property named "1" with the value 2
};

Often times these bugs lie dormant in code as computed properties aren't super-regularly used (in my experience), but the bug can cause some really frustrating linting experiences - or in the case of code transforms it can lead to broken code in production.

There's obviously no truly backwards-compatible way of fixing this. I have two ideas for how the AST might be updated to fix this.

---

Create a new node for computed-name properties

Currently my best idea is to create a new node like:

interface PropertyBase { /* ... */ }

interface Property extends PropertyBase {
  type: 'Property';
  key: Identifier | StringLiteral;
}
interface ComputedProperty extends PropertyBase {
  type: 'ComputedProperty';
  computedKey: Expression;
}

The reason you'd probably want a new key entirely is to stop people just updating to this pattern and falling into exactly the same state that existed before:

function analyseProperty(prop: Property | ComputedProperty) {
  if (property.key.type === 'Identifier') {
    analyseStaticPropertyName(property.key);
  }
}

PROs:

• Old tools that explicitly guard their object property iterations against SpreadElements will continue to work correctly if the guard was prop.type === 'Property'.

CONs:

• The variation in the key naming is kinda ugly.
• Tools that assume "if not SpreadElement, is instead a Property" will crash because .key is now potentially undefined.

---

Create a new node just for the property key

Another idea would be to rework the .key property:

interface StaticPropertyKey {
  type: 'StaticPropertyKey';
  key: Identifier | StringLiteral;
}

interface ComputedPropertyKey {
  type: 'ComputedPropertyKey';
  expression: Expression;
}

interface Property {
  // ...
  key: StaticPropertyKey | ComputedPropertyKey;
  // remove the computed property
}

Again - the two types have different property names to prevent passthrough chaining like:

function analyseProperty(prop: Property) {
  if (property.key.key.type === 'Identifier') {
    analyseStaticPropertyName(property.key);
  }
}

PROs:

• Cleaner approach as it nicely encapsulates the computed AST variation within the key, rather than it existing on the property.
• Will not explicitly crash any well-written tooling because checks like prop.key.type === 'Identifier' will now break.

CONs:

• Completely backwards incompatible because old tooling will no longer be able to analyse property keys.

---

I'd love to hear some thoughts about this.
Is this too much of a breaking change? Is this just an AST state that we need to grit our teeth and bear? Or is there something we could do about this?

[forivall]

Judging by how estree focuses on stability of the api, i think the

Create a new node just for the property key

option is out of the question. If you want an ast format that takes that approach, then the Shift AST is something to look at, if you weren't currently aware of it. For example, for your example, its representation is the following:

Shift AST JSON

{
  "type": "ObjectExpression",
  "properties": [
    {
      "type": "DataProperty",
      "name": {
        "type": "StaticPropertyName",
        "value": "prop"
      },
      "expression": {
        "type": "LiteralNumericExpression",
        "value": 2
      }
    },
    {
      "type": "DataProperty",
      "name": {
        "type": "ComputedPropertyName",
        "expression": {
          "type": "IdentifierExpression",
          "name": "prop"
        }
      },
      "expression": {
        "type": "LiteralNumericExpression",
        "value": 2
      }
    }
  ]
}

As far as the first possibility goes, there's also MethodDefinition to be concerned about too, so that's another new node type of something like "ComputedMethodDefinition" and idk about that 🤷‍♀️. Also, computed is part of the old es5 MemberExpression, so that explains its origins.

Overall, i think the ideal solution for your use case (guessing that you're working in typescript) is to improve the typescript definitions you're using with something like

type ObjectPropertyMaybeComputed = ObjectProperty & (
  | { computed: false; key: Identifier | StringLiteral | NumericLiteral }
  | { computed: true; key: Expression }
);

[bradzacher]

Using another AST representation is entirely out of the question here, as I work on @typescript-eslint.

---

Overall, i think the ideal solution for your use case (guessing that you're working in typescript) is to improve the typescript definitions you're using with something like

We've already done this, and have had types for this for some time.
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/ast-spec/src/element/Property/spec.ts

However it doesn't solve the problem.
As per my original snippets - Expression includes Identifier. But as mentioned - a computed Identifier is completely different to a static Identifier.

---

That's 100% true - Property was what I was thinking about because I was touching the types, but really Property, PropertyDefinition, MethodDefinition, and MemberExpression all suffer from the same issue.

[forivall]

Yeah, no worries, I hadn't noticed that you were working on typescript-eslint until after i mentioned the Shift AST.

I think this is just a problem we have to live with in ESTree, as MemberExpression's computed goes back to the Mozilla Parser API.

[nzakas]

Unfortunately, we just can’t make changes like this to ESTree. As I’ve mentioned in other issues, ESTree isn’t really a spec that people follow, it’s more of a loose agreement between AST producers/consumers about how things should work. We can’t make breaking changes to ESTree because there’s no way to synchronize making such changes across every ESTree implementation. In the worst case tools stop working together and in the best case consumers end up needing to check for two different possible ways of doing the same thin, which will also introduce bugs.

I personally hate the way that object literal members were defined in ESTree and wish we had something better, but by the time we started discussing it, there were already too many implementations out there to get an agreement to change it.

So,in general, we need to live with the ugliness of existing parts of ESTree and just try to do better when we are able to add new nodes.