code review 40630043: go.tools/oracle: improvements to command set and perfor...

go.tools/oracle: improvements to command set and performance.

Command set:
- what: an extremely fast query that parses a single
  file and returns the AST stack, package name and the
  set of query modes that apply to the current selection.
  Intended for GUI tools that need to grey out UI elements.
- definition: shows the definition of an identifier.
- pointsto: the PTA features of 'describe' have been split
  out into their own command.
- describe: with PTA stripped out, the cost is now bounded by
  type checking.

Performance:
- The importer.Config.TypeCheckFuncBodies predicate supports
  setting the 'IgnoreFuncBodies' typechecker flag on a
  per-package basis.  This means we can load dependencies from
  source more quickly if we only need exported types.
  (We avoid gcimport data because it may be absent or stale.)
  This also means we can run type-based queries on packages
  that aren't part of the pointer analysis scope. (Yay.)
- Modes that require only type analysis of the query package
  run a "what" query first, and restrict their analysis scope
  to just that package and its dependencies (sans func
  bodies), making them much faster.
- We call newOracle not oracle.New in Query, so that the
  'needs' bitset isn't ignored (oops!).  This makes the
  non-PTA queries faster.

Also:
- removed vestigial timers junk.
- pos.go: existing position utilties split out into own file.
  Added parsePosFlag utility.
- numerous cosmetic tweaks.

+ very basic tests.

To do in follow-ups:
- sophisticated editor integration of "what".
- better tests.
- refactoring of control flow as described in comment.
- changes to "implements", "describe" commands.
- update design doc + user manual.

[adonovan, 2013-12-12 19:38:15]

Hello crawshaw@google.com (cc: golang-dev@googlegroups.com, gri@golang.org),

I'd like you to review this change to
https://code.google.com/p/go.tools

[Dominik Honnef, 2013-12-13 08:40:05]

https://codereview.appspot.com/40630043/diff/130001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/40630043/diff/130001/cmd/oracle/oracle.el#newc...
cmd/oracle/oracle.el:40: ;; makes it harder to use the oracle.
I disagree that it makes it particularly harder to use; all you need is a hook
on go-mode to enable go-oracle-mode as well.

But if you want you can get rid of this mode and just extend go-mode's keymap.

[crawshaw1, 2013-12-13 13:44:36]

LGTM

https://codereview.appspot.com/40630043/diff/130001/importer/importer.go
File importer/importer.go (right):

https://codereview.appspot.com/40630043/diff/130001/importer/importer.go#newc...
importer/importer.go:85: // The supplied IgnoreFuncBodies is ignored; the
effective
may read better as "is not used"

https://codereview.appspot.com/40630043/diff/130001/oracle/pos.go
File oracle/pos.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/pos.go#newcode109
oracle/pos.go:109: if xi, err := os.Stat(x); err == nil {
Lstat? (given the optimization)

https://codereview.appspot.com/40630043/diff/130001/oracle/serial/serial.go
File oracle/serial/serial.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/serial/serial.go#n...
oracle/serial/serial.go:115: type AST struct {
I think this is an overuse of "AST". How about Region?

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go
File oracle/what.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go#newcode38
oracle/what.go:38: enable["callgraph"] = true                 // whole program;
always enabled
enable := map[string]bool{
  "callgraph": true,
  "implements": true,
  ...
}

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go#newcode154
oracle/what.go:154: // is absDir a prefix of segs?
comment seems unrelated.

[adonovan, 2013-12-13 15:00:36]

https://codereview.appspot.com/40630043/diff/130001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/40630043/diff/130001/cmd/oracle/oracle.el#newc...
cmd/oracle/oracle.el:40: ;; makes it harder to use the oracle.
On 2013/12/13 08:40:05, Dominik Honnef wrote:
> I disagree that it makes it particularly harder to use; all you need is a hook
> on go-mode to enable go-oracle-mode as well.
> 
> But if you want you can get rid of this mode and just extend go-mode's keymap.


I'd like to remove all obstacles to making the oracle "just work" for ordinary
Go+Emacs users.  The extra mode seems like one to me, since it requires oracle
users to add yet another line of configuration to their ~/.emacs file.

https://codereview.appspot.com/40630043/diff/130001/importer/importer.go
File importer/importer.go (right):

https://codereview.appspot.com/40630043/diff/130001/importer/importer.go#newc...
importer/importer.go:85: // The supplied IgnoreFuncBodies is ignored; the
effective
On 2013/12/13 13:44:38, crawshaw1 wrote:
> may read better as "is not used"

Done.

https://codereview.appspot.com/40630043/diff/130001/oracle/pos.go
File oracle/pos.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/pos.go#newcode109
oracle/pos.go:109: if xi, err := os.Stat(x); err == nil {
On 2013/12/13 13:44:38, crawshaw1 wrote:
> Lstat? (given the optimization)

When we say "denote the same file", we mean that open/read/write on x will
appear to affect y, so Stat is correct.

(In my experience, LStat is rarely the right tool for the job.  Obviously it
exists for a reason, which is when the topology of the link graph is important,
but for tools that just use open/read/write, Lstat tends to result in surprising
behaviour.)

https://codereview.appspot.com/40630043/diff/130001/oracle/serial/serial.go
File oracle/serial/serial.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/serial/serial.go#n...
oracle/serial/serial.go:115: type AST struct {
On 2013/12/13 13:44:38, crawshaw1 wrote:
> I think this is an overuse of "AST". How about Region?

This type corresponds exactly to an ast.Node, not an arbitrary region of a file,
so don't be confused by the fact two thirds of the information it contains are
Start and End.

I'm open to other names though.  Syntax, ASTNode, SyntaxTree, ...?

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go
File oracle/what.go (right):

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go#newcode38
oracle/what.go:38: enable["callgraph"] = true                 // whole program;
always enabled
On 2013/12/13 13:44:38, crawshaw1 wrote:
> enable := map[string]bool{
>   "callgraph": true,
>   "implements": true,
>   ...
> }

Done.

https://codereview.appspot.com/40630043/diff/130001/oracle/what.go#newcode154
oracle/what.go:154: // is absDir a prefix of segs?
On 2013/12/13 13:44:38, crawshaw1 wrote:
> comment seems unrelated.

Oops, deleted.

[adonovan, 2013-12-13 15:04:58]

*** Submitted as
https://code.google.com/p/go/source/detail?r=4e56dc65c681&repo=tools ***

go.tools/oracle: improvements to command set and performance.

Command set:
- what: an extremely fast query that parses a single
  file and returns the AST stack, package name and the
  set of query modes that apply to the current selection.
  Intended for GUI tools that need to grey out UI elements.
- definition: shows the definition of an identifier.
- pointsto: the PTA features of 'describe' have been split
  out into their own command.
- describe: with PTA stripped out, the cost is now bounded by
  type checking.

Performance:
- The importer.Config.TypeCheckFuncBodies predicate supports
  setting the 'IgnoreFuncBodies' typechecker flag on a
  per-package basis.  This means we can load dependencies from
  source more quickly if we only need exported types.
  (We avoid gcimport data because it may be absent or stale.)
  This also means we can run type-based queries on packages
  that aren't part of the pointer analysis scope. (Yay.)
- Modes that require only type analysis of the query package
  run a "what" query first, and restrict their analysis scope
  to just that package and its dependencies (sans func
  bodies), making them much faster.
- We call newOracle not oracle.New in Query, so that the
  'needs' bitset isn't ignored (oops!).  This makes the
  non-PTA queries faster.

Also:
- removed vestigial timers junk.
- pos.go: existing position utilties split out into own file.
  Added parsePosFlag utility.
- numerous cosmetic tweaks.

+ very basic tests.

To do in follow-ups:
- sophisticated editor integration of "what".
- better tests.
- refactoring of control flow as described in comment.
- changes to "implements", "describe" commands.
- update design doc + user manual.

R=crawshaw, dominik.honnef
CC=golang-dev, gri
https://codereview.appspot.com/40630043

[adonovan, 2013-12-13 18:29:47]

*** Abandoned ***