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

.visit(...) does not visit nodes that do not extend Printable #123

Closed
jamestalmage opened this issue Aug 11, 2015 · 4 comments
Closed

.visit(...) does not visit nodes that do not extend Printable #123

jamestalmage opened this issue Aug 11, 2015 · 4 comments

Comments

@jamestalmage
Copy link
Contributor

This may make sense / not matter for processing ECMAScript related AST's, but it is a weird requirement to force on implementors of custom AST's.

This is a blocker for #57

@jamestalmage
Copy link
Contributor Author

It appears this was intentional in @843b55b.

I have not yet figured out why visiting non-Printable nodes would be a bad idea, even in the ECMAScript context.

Assuming there are good reasons for it, perhaps we generalize path-visitor to take a custom filter function. The default would be to allow everything, but types.visit would expose an instance that filtered out non-Printable nodes.

Also (or possibly alternatively), it might be good to extend field definitions (i.e. def(...).field(...)) with a visitable flag, and make a types.getVisitableFields, or just pass a getVisitableFields implementation to the generalized path-visitor.

@benjamn
Copy link
Owner

benjamn commented Aug 11, 2015

I'm also having trouble remembering the original rationale… I think it was just for performance reasons (not visiting .loc, mostly), but that can be avoided in other ways.

@jamestalmage
Copy link
Contributor Author

I think that Printable.check(...) line may also be what prevents it from trying to visit fields with primitive values.

@benjamn
Copy link
Owner

benjamn commented Aug 21, 2015

Digging deeper into the background, visitation used to be restricted to subtypes of Node, which is more restrictive than Printable. I switched to Printable in this commit in order to allow visiting non-Node printable things like comments (but still forbid things like SourceLocations).

I'm persuaded by your argument that we should relax the restriction even further. In order to be visitable, I think an object just needs to have a string-valued .type field, since that's what we look up in this._methodNameTable anyway.

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