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

Cannot Assign Arrow Function Macro #63

Open
tecumsehcommunications opened this issue Apr 17, 2024 · 1 comment
Open

Cannot Assign Arrow Function Macro #63

tecumsehcommunications opened this issue Apr 17, 2024 · 1 comment

Comments

@tecumsehcommunications
Copy link

tecumsehcommunications commented Apr 17, 2024

My arrow function macro is as follows:

(macro =>
 (function ()
           (= env this)
           (= atoms ((. Array from) (. arguments 0 values)))
              
  (= propertyIdentifiers
     ((. atoms map)
      (function (atom) (return ((. env compile) atom)))))

  (= properties ((. propertyIdentifiers map)
                   (function (identifier)
                    (return (object type "Property"
                                    key identifier
                                    value identifier
                                    shorthand true)))))
  
  (= estree (object 
                                  type "ArrowFunctionExpression"
                                  generator false
                                  async false
                                  params properties
                                  body ((. this compile) (. arguments 1))))

  (return estree)))

I show the Ecmascript it produces with:

(macro showEcma
 (lambda (expression)
    (return `((. console log)
              ,((. this string)
                ((. this compileToJs) 
                 ((. this compile) expression)))))
    (return null)))

And I show the AST representation with:

(macro showAST
       (lambda (expression)
               (return ((. console log)                         
                           ((. this compile) expression))))) 

When I run :

    (showEcma (= var1
             (=> (a b c)
                 (block ((. console log) a)
                        ((. console log) b)
                        ((. console log) c)))))

I get:

var1 = (a, b, c) => {
    console.log(a);
    console.log(b);
    console.log(c);
};

When I showAST I get:

{
  type: 'AssignmentExpression',
  operator: '=',
  left: {
    type: 'Identifier',
    name: 'var1',
    loc: { start: [Object], end: [Object] }
  },
  right: {
    type: 'ArrowFunctionExpression',
    generator: false,
    async: false,
    params: [ [Object], [Object], [Object] ],
    body: { type: 'BlockStatement', body: [Array], loc: [Object] },
    loc: { start: [Object], end: [Object] }
  },
  loc: {
    start: { offset: 22, line: 1, column: 22 },
    end: { offset: 23, line: 1, column: 23 }
  }
}

Looks good to me. But when I go live with it:

(= var1 (=> (a b c)
                 (block ((. console log) a)
                        ((. console log) b)
                        ((. console log) c))))

I get:

[Error] AssignmentExpression `right` member must be an expression node
At line 1, offset 1:

Any ideas?

Kevin

@anko
Copy link
Owner

anko commented May 15, 2024

Hi, and thanks for your bug report.

I haven't looked at this project's code in a while, so I'll talk through my thoughts:

That error comes from esvalid, which is the package used internally by the compiler here to sanity-check any AST before it's fed to escodegen for code generation, because escodegen doesn't do validation.

I'm doing this validation because writing macros that construct estree is complex enough as-is, so I'd like to catch and display errors early, for users' benefit.

However, esvalid hasn't been updated for a long time, so it doesn't support newer estree spec additions, such as arrow functions, even though escodegen can generate code for them. So in a792d03 I introduced this filter which is supposed to ignore esvalid errors relating to nodes that it doesn't support. It's not ideal, but the intention is to ignore errors with the unsupported new stuff, while retaining useful errors for estree nodes which validation is supported.

These are the errors for your code, before filtering:

[
  [InvalidAstError: AssignmentExpression `right` member must be an expression node] {
    node: {
      type: 'AssignmentExpression',
      operator: '=',
      left: [Object],
      right: [Object],
      loc: [Object]
    }
  },
  [InvalidAstError: unrecognised node type: "ArrowFunctionExpression"] {
    node: {
      type: 'ArrowFunctionExpression',
      generator: false,
      async: false,
      params: [Array],
      body: [Object],
      loc: [Object]
    }
  }
]

It looks like when an AssignmentExpression has an estree type that esvalid doesn't recognise (such as a ArrowFunctionExpression), it reports two errors: one for the unrecognised estree type (which gets removed by my filter), and also one for the AssignmentExpression that contains it (which isn't removed by my filter, because esvalid supports that estree type).

My error-filter is based on the false assumption that estree would only report an error for the unrecognised estree type in such cases.


I'm leaning toward fixing this by removing esvalid altogether. It is badly out of date, and it's clear that filtering out its errors is more cumbersome than I expected.

That would mean invalid estree would be passed to escodegen. That means it may generate invalid JS, or accidentally generate valid JS for invalid estree. Those problems would not be caught at compile-time. But at least then valid programs like yours would be accepted.

I may get around to that at some point. If you get to it first, I would welcome a PR.


In a perfect world, esvalid (or a fork) would support the full and current estree spec, and we could validate it all. But that would be quite a lot of really boring work.

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

No branches or pull requests

2 participants