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

Figure out an approach to NodePath 'hub' object tracking with better cache behavior #6437

Open
loganfsmyth opened this Issue Oct 6, 2017 · 1 comment

Comments

Projects
None yet
3 participants
@loganfsmyth
Member

loganfsmyth commented Oct 6, 2017

I recently realized we've got bug with our invalidation logic for NodePath that can cause some real weirdness. Here it is on ASTExplorer and here:

var mem = template("property.access")();

traverse(mem, {noScope: true});

var p2 = path.insertAfter(mem)[0];

console.log(p2.get("expression").hub);

will print undefined for hub even through p2.hub is the Hub class instance.

Which is surprising because in

if (!hub && parentPath) {
, the hub value is explicitly supposed to pass down.

It fails in this case because on

if (pathCheck.node === targetNode) {
if a value is found in the cache, we never bother bailing out.

The problem is, what do we do. Two options come to mind:

  1. Overwrite the .hub on the cache result.
  2. Only accept items from the cache if the hub is correct.

Option 1

Pretty gross because we could just as easily be introducing another bug by removing a Hub instance that was supposed to be there

Option 2

Sounds reasonable, until you realize that we actually crucially rely on this gross caching behavior to instantiate NodePath with the correct Hub instance in the first place.

In File init in

we create the root NodePath, but then we never really do anything with it. Traversal begins on
traverse(file.ast, visitor, file.scope);
, which never explicitly mentions this.path at all. That path ends up being used, thus using the root Hub, because it is present in the cache. If we explicitly test for the Hub in the cache, we'll break this weird cache-based pass-through logic.

Solution?

To me, the "feature" we're breaking in option 2 is totally a mistake to rely on since it is essentially relying on sideeffects to the extreme. This means we need a way to do two things:

  1. Initialize the NodePath up front, so it can be stored on the File class instance
  2. Perform a traversal using this NodePath, while ensuring that same traversal logic we have now is preserved.

The exact details on how we do those two things is up in the air, but feels like something we should figure out ASAP.

Incidentally, this same bug is the reason we have to do

traverse.clearNode(node);
to explicitly clear the cache, otherwise every node we get pack from babel-template would trigger this bug, due to the internal traversal, which doesn't have a hub, polluting the cache.

@loganfsmyth loganfsmyth added this to the Babel 7.next milestone Oct 6, 2017

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo

nicolo-ribaudo Oct 6, 2017

Member

Would making hub a getter be too expensive? e.g.

  get hub() {
    return this
_hub || (this.parentPath && this.parentPath.hub);
  }
Member

nicolo-ribaudo commented Oct 6, 2017

Would making hub a getter be too expensive? e.g.

  get hub() {
    return this
_hub || (this.parentPath && this.parentPath.hub);
  }
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment