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

Issue with reusing AST nodes #6375

Closed
hzoo opened this Issue Oct 3, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@hzoo
Member

hzoo commented Oct 3, 2017

Just want to write this down since we're seeing the same kind of fix for issues and we should look into solving this generally.

Immutable asts would be a larger change, so maybe #5971 or infra in our unit tests to check uniqueness would help

#6046
#6054
#6374

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Oct 4, 2017

Member

I can work on this tomorrow, I would like to have a quick prototype to measure the perf impact (if any).

Member

xtuc commented Oct 4, 2017

I can work on this tomorrow, I would like to have a quick prototype to measure the perf impact (if any).

@Andarist Andarist closed this Oct 4, 2017

@Andarist Andarist reopened this Oct 4, 2017

@Andarist

This comment has been minimized.

Show comment
Hide comment
@Andarist

Andarist Oct 4, 2017

Member

closed by accident with a fat finger :<

Member

Andarist commented Oct 4, 2017

closed by accident with a fat finger :<

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc Oct 5, 2017

Member

It's more complicated than I thought, the traverse algorithm is not easy to understand.

I wanted to use Immutable.js with a tree datastructure.

I don't know what we would do with mutations like: path.node.id.name = "a";, it would have a big perf impact to allow that (since it would bypass the partial copy optimization). Only path.Insert*, path.remove or path.replace* would be possible.

I've looked into OCaml and GHC, both compiler are self-hosted and uses an immutable tree. They are fast enough to not need to merge visitors and only use passPerPreset-like behavior.

Member

xtuc commented Oct 5, 2017

It's more complicated than I thought, the traverse algorithm is not easy to understand.

I wanted to use Immutable.js with a tree datastructure.

I don't know what we would do with mutations like: path.node.id.name = "a";, it would have a big perf impact to allow that (since it would bypass the partial copy optimization). Only path.Insert*, path.remove or path.replace* would be possible.

I've looked into OCaml and GHC, both compiler are self-hosted and uses an immutable tree. They are fast enough to not need to merge visitors and only use passPerPreset-like behavior.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 5, 2017

Member

@loganfsmyth had ideas a long time ago: https://github.com/babel/notes/blob/master/2016/2016-08/august-01.md#potential-api-changes-for-traversal around this.

And yes we would have to prevent assignment and only allow path methods

Member

hzoo commented Oct 5, 2017

@loganfsmyth had ideas a long time ago: https://github.com/babel/notes/blob/master/2016/2016-08/august-01.md#potential-api-changes-for-traversal around this.

And yes we would have to prevent assignment and only allow path methods

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 1, 2017

Member

Happening more so we really need a fix

#6517
#6657

Member

hzoo commented Nov 1, 2017

Happening more so we really need a fix

#6517
#6657

@hzoo hzoo added the Priority: High label Nov 1, 2017

@nicolo-ribaudo

This comment has been minimized.

Show comment
Hide comment
@nicolo-ribaudo
Member

nicolo-ribaudo commented Nov 29, 2017

Related: #3428

@lock lock bot added the outdated label May 2, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 2, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.