Proposal: Rethink path/node removal

[danez]

Inspired by babel/minify#300, babel/minify#375 and #4896 I was thinking how to properly solve all this cases. Here a short introduction to the problem:

Assuming this code:

active ? a() : b();

Now assuming a transform removes all a() calls, and the resulting code gets

active ? : b();

This is not valid js syntax anymore and babel currently throws somewhere after calling path.remove() in this case.

Currently we have babel handling some of this cases (loops, and arrow functions) but not all of them. Instead of adding more and more rules how to delete certain nodes with certain parents I propose a breaking change to the remove functionality of paths:

Changing path.remove()

I propose changing path.remove() to do what it says, removal of a node/path. If the resulting AST is not valid it should throw. Also remove the current removal-hooks for loops and arrow functions and throw instead if for example a loop ends up with the body node being null.

Introduce path.isRemovable()

A boolean method that returns true if it is safe to remove the path or false if not.

Introduce path.getSafeReplacement()

A function that returns a safe replacement for the current node. It would return a new node that is a "nullable" variant for the current node (Identifier<"undefined">, BlockStatement, ...) or throw if it is not possible (like var a = 1; Remove Identifier a here)

Introduce path.removeOrReplace();

This would more be a sum of the 3 methods I mentioned before and might not be necessary at all, but could be easier to use for plugin authors.

removeOrReplace() {
   if (this.isRemovable()) {
      this.remove();
   } else {
      this.replaceWith(this.getSafeReplacement());
   }
}

The new functions could be added in 6.x and the change to remove() in 7.0, so that people can migrate.

The ultimate goal of this proposal is to make remove() do what it name says and give plugin authors the tools to handle all the edge cases where remove is not as easy and not every plugin should handle all this cases on their own and duplicate the logic of if-parent-is-ternary-operator then-replace-with-undefined otherwise-if-....

All of this is just a proposal and I would like to hear what you think about it. If you have better ideas or what you don't like about it.

I want to do more research and see how other handle that (estraverse for example).

@boopathi @kangax @loganfsmyth @DrewML

[boopathi]

I think that nullify will have more corner cases that can't be handled safely.

For example,

a + b; // => a + void 0
a && b // => a && void 0
a * b // => a * void 0
var foo = [ a, b ];
foo.map(x => baz(x)); // unintended undefined

It's easier to simply use replaceWith(getSafeReplacement()) instead of inspecting what getSafeReplacement returns. If the Node returned by getSafeReplacement needs to be inspected before replacement, then we might as well understand the context around the path to be removed and create that Node accordingly.

Also, removeOrReplace is what remove does already, right? I would assume there is no correct remove function and path.remove() is just doing removeOrReplace as you defined.

I feel I'm just going around in circles. But I like the idea of having small helper functions broken down instead of a monolith remove that handles everything internally. This way we can have a balance between being correct vs not throwing everywhere.