Leftover AST nodes in compiled output

[gabejohnson]

Adapted from #605

import { isPunctuator, fromPunctuator } from './helpers' for syntax;
syntax m = ctx => {
  let delim = ctx.next().value;
  var arr = [];

  for (let item of ctx.contextify(delim)) {
    if (!isPunctuator(item, ',')) {
      arr.push(item);
      arr.push(fromPunctuator(item, ','));
      arr.push(item);
    } else {
      arr.push(item);
    }
  }
  return #`[${arr}]`;
}

m {
  1, 2, 3, "sdf"
}

// expected
[1, 1, 2, 2, 3, 3, "sdf", "sdf"];

// actual
;
;
[1, 1, 2, 2, 3, 3, "sdf", "sdf"];

This puzzled me for a long time as I assumed it was an issue w/ ASI. Then I tried looking at the generated AST before codegen:

{
  exportedNames: [],
  items: [
    { type: 'EmptyStatement', loc: null },
    {
      declaration: {
        kind: 'syntax',
        declarators: [{
        binding: {
          name: {
            token: {..., value: 'm', ...},
            bindings: Map {...},
            scopesets: {...}
          },
          type: 'BindingIdentifier',
          loc: null
        },
        init: {
          params: {
            items: [...],
            rest: null,
            type: 'FormalParameters',
            loc: null
          },
          body: {
            directives: [],
            statements: [...],
            type: 'FunctionBody',
            loc: null
          },
          type: 'ArrowExpression',
          loc: null
        },
        type: 'VariableDeclarator',
        loc: null
      }],
        type: 'VariableDeclaration',
        loc: null
      },
      type: 'VariableDeclarationStatement',
      loc: null
    }, {
      expression: {
        elements: [...],
        type: 'ArrayExpression',
        loc: null
    },
    type: 'ExpressionStatement',
    loc: null
  },
  exports: []
}

It looks like an import ... for syntax is being replaced w/ an EmptyStatement. This is reasonable, but it would probably be better to just remove it from the list of items.

The more concerning issue is that the macro definition isn't being erased. It's not displaying, I'm guessing, because shift-codegen doesn't know what to do w/ VariableDeclaration#kind equal to syntax.

I know it doesn't affect the how the code executes, but it looks messy and I'd rather have it cleared up before v3.0.0 is released.

@disnet do you have any pointers to where the erasure should be happening?

[disnet]

So the macro definition is removed in sweet-to-shift-reducer here just before the AST passed off to shift (it does the same empty statement trick as imports). We actually need to keep the macro def in the intermediary AST because modules reference the same AST multiple times (for each phase).

The empty statement trick is used because the reducer requires each reduction method to return a valid term. We could add a cleaning pass the removed redundant empty statements at the appropriate places (probably in sweet-to-shift-reducer).

[gabejohnson]

That makes sense. I'll investigate why I was seeing the macro definition in the reduced AST. If I find time this week I'll try to fix it too.

[gabejohnson]

Or is the reduction happening during codegen?

[disnet]

It's reduced to a shift-compatible AST here and then that reduced AST is codegened here.

[gabejohnson]

We can filter them out here, here and here.

Is there anywhere filtering out EmptyStatements shouldn't be a mechanical process (other than for loops)? Another option is to not use the EmptyStatement trick and instead just filter imports and syntax declarations out anywhere we might see them.

[disnet]

Is there anywhere filtering out EmptyStatements shouldn't be a mechanical process (other than for loops)?

IfStatement maybe?

Another option is to not use the EmptyStatement trick and instead just filter imports and syntax declarations out anywhere we might see them.

It's a nice optimization. Macro authors might write macros that expand to more EmptyStatements than necessary so optimizing them away would be nice.

[gabejohnson]

IfStatement maybe?

I think that's taken care of by https://github.com/sweet-js/sweet.js/blob/62a63cd09762f9ab9b3a89147d882344dad8d1f5/src/sweet-to-shift-reducer.js#L91

An EmptyStatement as the consequent of an IfStatement should not be removed.

Macro authors might write macros that expand to more EmptyStatements than necessary so optimizing them away would be nice.

Makes sense. I'll filter them out at the places indicated.

Any idea why they just started showing up recently?

[disnet]

Any idea why they just started showing up recently?

Don't think they are recent. Macro definitions have always (I think?) been converted to EmptyStatements; the import for syntax is new with modules of course so maybe that's what made you notice?