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

Store type information directly in the AST #340

Closed
dominikh opened this issue Sep 15, 2018 · 1 comment
Closed

Store type information directly in the AST #340

dominikh opened this issue Sep 15, 2018 · 1 comment
Labels
idea Ideas for new tools/features/frameworks/... performance wontfix

Comments

@dominikh
Copy link
Owner

The design of go/ast and go/types is rather modular, in that go/ast is a purely untyped AST (albeit with semi-working object resolution, which is superseded by go/types). go/types stores all of its information in maps (Defs, Uses and so on). This approach has two drawbacks:

  1. Performance. Using maps for many many thousands of objects is significantly more memory and CPU intensive than storing them directly in fields of AST nodes.
  2. Ease of use. While separating AST and type information makes for an elegant implementation, it doesn't make for very elegant use. One has to constantly jump between two different domains. The lack of methods and fields on AST nodes also makes relations between the two domains much harder to discover. Furthermore, it complicates passing around data in a static analysis framework. Instead of just passing around the AST, we also have to pass around the correct types.Info. Adding to that go/packages decision to have one types.Info per checked package, instead of a single types.Info for the entire program. makes the mapping even more difficult

I have a WIP branch that forks and merges go/ast and go/types. Significant performance improvements have been observed. Additionally, the AST portions of the code could be simplified due to the removal of ast.Scope and ast.Object. Most checks could be transformed rather simply; replacing info.ObjectOf(ident) with ident.Obj() and so on. The only significant change in idioms is that we can no longer iterate over Info.Types and similar, something that unused does. Instead, we now have to walk the AST and extract the data that we need. This, however, isn't really an issue. Overall, it is still more performant than the original code. Furthermore, unused is the only client doing this, and it will soon be rewritten in terms of SSA, making both Info.Types and AST walking unnecessary.

Initially, I had hoped to keep go/ast and go/types as two separate packages, by introducing a third package containing just the relevant definitions that go/ast will depend on. Unfortunately, go/types depends on a lot of internal state in these definitions, which made splitting it into two packages infeasible.

@dominikh dominikh added the idea Ideas for new tools/features/frameworks/... label Sep 15, 2018
@dominikh dominikh added started Issues we've started working on performance labels Jan 28, 2019
@dominikh
Copy link
Owner Author

The results were great, but we're committed to porting to the go/analysis framework now.

@dominikh dominikh removed the started Issues we've started working on label Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Ideas for new tools/features/frameworks/... performance wontfix
Projects
None yet
Development

No branches or pull requests

1 participant