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

ChainExpression #98

Closed
neumatho opened this issue Sep 28, 2020 · 17 comments · Fixed by #100
Closed

ChainExpression #98

neumatho opened this issue Sep 28, 2020 · 17 comments · Fixed by #100

Comments

@neumatho
Copy link

Hi,

For the expression (a?.import("string")?.import.meta??(a)), the argument "string" is stored as a ChainExpression node with a Literal in expression property. The ChainExpression should be skipped and the Literal should be stored directly in arguments instead. According to the AST specifications, it is not allowed to have a Literal in expression property in a ChainExpression node:

https://github.com/estree/estree/blob/master/es2020.md#chainexpression

-Thomas

@KFlash
Copy link
Contributor

KFlash commented Sep 28, 2020

Cc @3cp

@neumatho
Copy link
Author

neumatho commented Sep 29, 2020

The same problem with a?.[x]. Here you create an ExpressionStatement with a MemberExpression in expression and a ChainExpression in property. It should be the other way around. ExpressionStatement with a ChainExpression in expression and the MemberExpression in expression.

Another thing. Since this is official in the AST specification, I guess chain expression should be enabled by default without the Next option or what?

@KFlash
Copy link
Contributor

KFlash commented Sep 29, 2020

Sounds reasonable. As said before some students of mine did this code, and I personally avoid ESTree.

@aladdin-add Ideas?

@KFlash
Copy link
Contributor

KFlash commented Sep 29, 2020

@neumatho So I get you correct. The result you want is like this but with ESTree AST ?

@neumatho
Copy link
Author

neumatho commented Sep 29, 2020

I want it like this, which is the way Acorn generates it and seems to comply with the specification:

{
  "type": "Program",
  "body": [
    {
      "type": "ExpressionStatement",
      "expression": {
        "type": "ChainExpression",
        "expression": {
          "type": "MemberExpression",
          "object": {
            "type": "Identifier",
            "name": "a"
          },
          "property": {
            "type": "Identifier",
            "name": "x"
          },
          "computed": true,
          "optional": true
        }
      }
    }
  ],
  "sourceType": "module"
}

@neumatho
Copy link
Author

and for (a?.import("string")?.import.meta??(a)) is should look like this:

{
  "type": "Program",
  "body": [
    {
      "type": "ExpressionStatement",
      "expression": {
        "type": "LogicalExpression",
        "left": {
          "type": "ChainExpression",
          "expression": {
            "type": "MemberExpression",
            "object": {
              "type": "MemberExpression",
              "object": {
                "type": "CallExpression",
                "callee": {
                  "type": "MemberExpression",
                  "object": {
                    "type": "Identifier",
                    "name": "a"
                  },
                  "property": {
                    "type": "Identifier",
                    "name": "import"
                  },
                  "computed": false,
                  "optional": true
                },
                "arguments": [
                  {
                    "type": "Literal",
                    "value": "string",
                    "raw": "\"string\""
                  }
                ],
                "optional": false
              },
              "property": {
                "type": "Identifier",
                "name": "import"
              },
              "computed": false,
              "optional": true
            },
            "property": {
              "type": "Identifier",
              "name": "meta"
            },
            "computed": false,
            "optional": false
          }
        },
        "operator": "??",
        "right": {
          "type": "Identifier",
          "name": "a"
        }
      }
    }
  ],
  "sourceType": "module"
}

@3cp
Copy link
Member

3cp commented Sep 29, 2020

I did the migration from old estree spec to the new spec ChainExpression.
I can reproduce all the bugs here, clearly we missed some test coverage. Will need some time to understand where went wrong, hopefully not as nasty as those known bugs in destruction.

@3cp
Copy link
Member

3cp commented Sep 30, 2020

The bug is from I used a global flag here.

parser.flags |= Flags.HasOptionalChaining;

This is to record whether an expression contains optional chain, so that later we can wrap the whole expression in ChainExpression node according to the spec.

But the usage of global flag is wrong, in a?.[x], there are actually two expressions: a?.[] and a sub expression x. The sub expression needs to be parsed independently, but now the parser saw the global flag (set by previous ?. token) and mistakenly think x needs wrap.

@KFlash any suggestion on how to parse it correctly? Some flag to be saved with the expression itself, rather than globally?

@KFlash
Copy link
Contributor

KFlash commented Sep 30, 2020

Maybe try to look at the Escaya source.

@3cp
Copy link
Member

3cp commented Sep 30, 2020

The escaya is using old syntax OptionalExpression which doesn't require wrapping, just like the old meriyah implementation I think.

@KFlash
Copy link
Contributor

KFlash commented Oct 1, 2020

Escaya are just following the ECMA specs. I updated the chaining code in escaya now to follow the specs 100%. It seems like ESTree tries to do the same, but more complex.

Where is the wrapping?

@3cp
Copy link
Member

3cp commented Oct 1, 2020

Where is the wrapping?

meriyah/src/parser.ts

Lines 3990 to 3999 in d3de211

// Finalize ChainExpression
// FIXME: current implementation does not invalidate destructuring like `({ a: x?.obj['a'] } = {})`
if (inChain === 0 && (parser.flags & Flags.HasOptionalChaining) === Flags.HasOptionalChaining) {
parser.flags = (parser.flags | Flags.HasOptionalChaining) ^ Flags.HasOptionalChaining;
expr = finishNode(parser, context, start, line, column, {
type: 'ChainExpression',
expression: expr as (ESTree.CallExpression | ESTree.MemberExpression)
});
}

@KFlash
Copy link
Contributor

KFlash commented Oct 1, 2020

@neumatho
Copy link
Author

It looks like it is still needed to enable the next option for optional chaining. I think this should not be needed, since it is in the official specification now.

-Thomas

@neumatho
Copy link
Author

There are still some issues with this. Try a?.b[3].c?.(x).d, then Acorn outputs this tree:

{
  "type": "Program",
  "start": 0,
  "end": 16,
  "body": [
    {
      "type": "ExpressionStatement",
      "start": 0,
      "end": 16,
      "expression": {
        "type": "ChainExpression",
        "start": 0,
        "end": 16,
        "expression": {
          "type": "MemberExpression",
          "start": 0,
          "end": 16,
          "object": {
            "type": "CallExpression",
            "start": 0,
            "end": 14,
            "callee": {
              "type": "MemberExpression",
              "start": 0,
              "end": 9,
              "object": {
                "type": "MemberExpression",
                "start": 0,
                "end": 7,
                "object": {
                  "type": "MemberExpression",
                  "start": 0,
                  "end": 4,
                  "object": {
                    "type": "Identifier",
                    "start": 0,
                    "end": 1,
                    "name": "a"
                  },
                  "property": {
                    "type": "Identifier",
                    "start": 3,
                    "end": 4,
                    "name": "b"
                  },
                  "computed": false,
                  "optional": true
                },
                "property": {
                  "type": "Literal",
                  "start": 5,
                  "end": 6,
                  "value": 3,
                  "raw": "3"
                },
                "computed": true,
                "optional": false
              },
              "property": {
                "type": "Identifier",
                "start": 8,
                "end": 9,
                "name": "c"
              },
              "computed": false,
              "optional": false
            },
            "arguments": [
              {
                "type": "Identifier",
                "start": 12,
                "end": 13,
                "name": "x"
              }
            ],
            "optional": true
          },
          "property": {
            "type": "Identifier",
            "start": 15,
            "end": 16,
            "name": "d"
          },
          "computed": false,
          "optional": false
        }
      }
    }
  ],
  "sourceType": "module"
}

Meriyah has problems with the [3] and (x) part.

Also the ({})?.a["b"] parses wrong. It should look like this:

{
  "type": "Program",
  "start": 0,
  "end": 12,
  "body": [
    {
      "type": "ExpressionStatement",
      "start": 0,
      "end": 12,
      "expression": {
        "type": "ChainExpression",
        "start": 0,
        "end": 12,
        "expression": {
          "type": "MemberExpression",
          "start": 0,
          "end": 12,
          "object": {
            "type": "MemberExpression",
            "start": 0,
            "end": 7,
            "object": {
              "type": "ObjectExpression",
              "start": 1,
              "end": 3,
              "properties": []
            },
            "property": {
              "type": "Identifier",
              "start": 6,
              "end": 7,
              "name": "a"
            },
            "computed": false,
            "optional": true
          },
          "property": {
            "type": "Literal",
            "start": 8,
            "end": 11,
            "value": "b",
            "raw": "\"b\""
          },
          "computed": true,
          "optional": false
        }
      }
    }
  ],
  "sourceType": "module"
}

-Thomas

@3cp
Copy link
Member

3cp commented Oct 14, 2020

In case I missed this, can you create a new issue or add to #110? Thx.

@3cp
Copy link
Member

3cp commented Oct 15, 2020

This is fixed in latest commit in #112.
Added your test cases too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants