code review 9502043: go.tools/oracle: an oracle that answers questions about...

go.tools/oracle: an oracle that answers questions about Go source code.

+ Tests.
+ Emacs integration.
+ Emacs integration test.
+ very rudimentary Vim integration.  Needs some love from a Vim user.

TODO (in follow-ups):
- More tests would be good.
  We'll need to make the output order deterministic in more places.
- Documentation.

[adonovan, 2013-08-02 02:09:08]

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

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

[adonovan, 2013-08-22 19:44:36]

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

Please take another look.

[Dominik Honnef, 2013-08-22 20:06:46]

Probably getting ahead of you here :)

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode15
cmd/oracle/oracle.el:15: (defvar go-command
Should this be part of go-mode? We invoke the go tool a lot in there already.

Also, if this is configurable, use defcustom.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode44
cmd/oracle/oracle.el:44: previous scope if invoked interactively.
It always prompts

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode71
cmd/oracle/oracle.el:71: replace each filenames with a small hyperlink.  Display
the
each file name

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode85
cmd/oracle/oracle.el:85: (1- (position-bytes (region-beginning)))
Use go--position-bytes. XEmacs doesn't have position-bytes, go--position-bytes
falls back to point, so it can work for files without unicode at least.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode93
cmd/oracle/oracle.el:93: "GOMAXPROCS=8"
GOMAXPROCS should probably not be set?

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:104: (apply #'call-process (append (list "/usr/bin/env" nil
t nil) args))
/usr/bin/env won't work on Windows

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:187: ;; TODO(adonovan): do this only with the user's
permission.
Probably just tell the user how to do it in his .emacs

[adonovan, 2013-08-22 20:28:04]

Thanks for the review!

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode15
cmd/oracle/oracle.el:15: (defvar go-command
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> Should this be part of go-mode? We invoke the go tool a lot in there already.

Perhaps, but this approach (which I like, but I realize I'm in a minority of
users that switch between different GOROOT/GOPATH trees within a single editor
session) requires that other elisp code not look at Emacs's own $GOROOT and
$GOPATH values; when fork/execing other (non-'go') tools, they must be wrapped
by 'go env', etc.  That may be quite burdendsome.  I haven't yet looked at the
other elisp code with an eye to this.

> Also, if this is configurable, use defcustom. 

Done.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode44
cmd/oracle/oracle.el:44: previous scope if invoked interactively.
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> It always prompts

Fixed.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode71
cmd/oracle/oracle.el:71: replace each filenames with a small hyperlink.  Display
the
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> each file name

Done.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode85
cmd/oracle/oracle.el:85: (1- (position-bytes (region-beginning)))
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> Use go--position-bytes. XEmacs doesn't have position-bytes, go--position-bytes
> falls back to point, so it can work for files without unicode at least.

Done.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode93
cmd/oracle/oracle.el:93: "GOMAXPROCS=8"
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> GOMAXPROCS should probably not be set?

I'm not sure what the runtime team's approved solution is here, but this
variable needs to be set somewhere to achieve maximum concurrency during SSA
construction.  Is it better to set it within the application's main function?

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:187: ;; TODO(adonovan): do this only with the user's
permission.
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> Probably just tell the user how to do it in his .emacs

I agree that's probably the best approach, but it couples users' ~/.emacs files
to our function names while this code is still experimental.  I'll do that when
we announce and launch v1.0.
(Added TODO.)

[Dominik Honnef, 2013-08-22 20:35:18]

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode15
cmd/oracle/oracle.el:15: (defvar go-command
On 2013/08/22 20:28:04, adonovan wrote:
> On 2013/08/22 20:06:46, Dominik Honnef wrote:
> > Should this be part of go-mode? We invoke the go tool a lot in there
already.
> 
> Perhaps, but this approach (which I like, but I realize I'm in a minority of
> users that switch between different GOROOT/GOPATH trees within a single editor
> session) requires that other elisp code not look at Emacs's own $GOROOT and
> $GOPATH values; when fork/execing other (non-'go') tools, they must be wrapped
> by 'go env', etc.  That may be quite burdendsome.  I haven't yet looked at the
> other elisp code with an eye to this.

go-mode uses `go env GOROOT GOPATH` to obtain those two variables (mainly
because GOROOT can be implied); running non-go tools might be a problem though.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode93
cmd/oracle/oracle.el:93: "GOMAXPROCS=8"
On 2013/08/22 20:28:04, adonovan wrote:
> On 2013/08/22 20:06:46, Dominik Honnef wrote:
> > GOMAXPROCS should probably not be set?
> 
> I'm not sure what the runtime team's approved solution is here, but this
> variable needs to be set somewhere to achieve maximum concurrency during SSA
> construction.  Is it better to set it within the application's main function?

Well, I'd guess that the right GOMAXPROCS value for optimum SSA performance
depends on the user's hardware. Maybe just add a customizable variable in emacs,
for an oracle-specific GOMAXPROCS value? we can default it to e.g. 8 for people
who don't bother with customization.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:187: ;; TODO(adonovan): do this only with the user's
permission.
On 2013/08/22 20:28:04, adonovan wrote:
> On 2013/08/22 20:06:46, Dominik Honnef wrote:
> > Probably just tell the user how to do it in his .emacs
> 
> I agree that's probably the best approach, but it couples users' ~/.emacs
files
> to our function names while this code is still experimental.  I'll do that
when
> we announce and launch v1.0.
> (Added TODO.)

I suppose anyone who uses experimental tools will read the instructions and is
aware of the experimental nature.

[Dominik Honnef, 2013-08-22 20:38:16]

https://codereview.appspot.com/9502043/diff/47001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/47001/cmd/oracle/oracle.el#newcode25
cmd/oracle/oracle.el:25: :group 'go)
TODO should be 'go-oracle if we decide against merging go-command into go-mode,
to avoid confusion. But I guess we'll merge it, in which case TODO remove

[Dominik Honnef, 2013-08-22 20:42:23]

Does the oracle depend on any uncommitted changes to SSA or the importer? I'm
getting compile errors here:

dominikh@dominikh-pc $ go get -u code.google.com/p/go.tools/ssa
dominikh@dominikh-pc $ go get -u code.google.com/p/go.tools/pointer
dominikh@dominikh-pc $ go get -u code.google.com/p/go.tools/importer
dominikh@dominikh-pc $ go get -u code.google.com/p/go.tools/go/types
dominikh@dominikh-pc $ go install code.google.com/p/go.tools/cmd/oracle
# code.google.com/p/go.tools/oracle
/home/dominikh/prj/go/src/code.google.com/p/go.tools/oracle/oracle.go:113:
imp.InitialPackagesFromArgs undefined (type *importer.Importer has no field or
method InitialPackagesFromArgs)
/home/dominikh/prj/go/src/code.google.com/p/go.tools/oracle/oracle.go:171:
o.prog.CreateTestMainPackage undefined (type *ssa.Program has no field or method
CreateTestMainPackage)

[adonovan, 2013-08-22 20:55:50]

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcode93
cmd/oracle/oracle.el:93: "GOMAXPROCS=8"
On 2013/08/22 20:35:18, Dominik Honnef wrote:
> On 2013/08/22 20:28:04, adonovan wrote:
> > On 2013/08/22 20:06:46, Dominik Honnef wrote:
> > > GOMAXPROCS should probably not be set?
> > 
> > I'm not sure what the runtime team's approved solution is here, but this
> > variable needs to be set somewhere to achieve maximum concurrency during SSA
> > construction.  Is it better to set it within the application's main
function?
> 
> Well, I'd guess that the right GOMAXPROCS value for optimum SSA performance
> depends on the user's hardware. Maybe just add a customizable variable in
emacs,
> for an oracle-specific GOMAXPROCS value? we can default it to e.g. 8 for
people
> who don't bother with customization.

I've copied to main.go some startup code used by all Google binaries, which sets
maxprocs to NumCPUs; that what bradfitz recommended.

I've deleted this line.

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:104: (apply #'call-process (append (list "/usr/bin/env" nil
t nil) args))
On 2013/08/22 20:06:46, Dominik Honnef wrote:
> /usr/bin/env won't work on Windows

Good point.  Does Emacs provide any alternative to mutating and restoring its
own environment around call-process?

https://codereview.appspot.com/9502043/diff/47001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/47001/cmd/oracle/oracle.el#newcode25
cmd/oracle/oracle.el:25: :group 'go)
On 2013/08/22 20:38:16, Dominik Honnef wrote:
> TODO should be 'go-oracle if we decide against merging go-command into
go-mode,
> to avoid confusion. But I guess we'll merge it, in which case TODO remove

Added:
;; TODO(adonovan): move this when we merge with go-mode.el.
I think that's what you meant.

[Dominik Honnef, 2013-08-22 21:00:55]

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/44001/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:104: (apply #'call-process (append (list "/usr/bin/env" nil
t nil) args))
On 2013/08/22 20:55:50, adonovan wrote:
> On 2013/08/22 20:06:46, Dominik Honnef wrote:
> > /usr/bin/env won't work on Windows
> 
> Good point.  Does Emacs provide any alternative to mutating and restoring its
> own environment around call-process?

Temporarily changing process-environment should do the trick, and apparently the
oddly named server-with-environment macro does just that.

[adonovan, 2013-08-22 21:22:49]

On 2013/08/22 21:00:55, Dominik Honnef wrote:
> Temporarily changing process-environment should do the trick, and apparently
the
> oddly named server-with-environment macro does just that.

Actually this macro projects just the environment down to a subset... but using
its dynamic binding trick I can do what I want in a one-liner.

[Dominik Honnef, 2013-08-22 21:53:25]

https://codereview.appspot.com/9502043/diff/46053/oracle/peers.go
File oracle/peers.go (right):

https://codereview.appspot.com/9502043/diff/46053/oracle/peers.go#newcode33
oracle/peers.go:33: return nil, o.errorf(o.queryPath[0], "there is no
send/receive here")
Is it possible to support "for _ = range ch" (with point on ch) as well and
treat it like a receive op?

[adonovan, 2013-08-22 22:33:23]

On 22 August 2013 17:53, <dominik.honnef@gmail.com> wrote:

>
>
https://codereview.appspot.**com/9502043/diff/46053/oracle/**peers.go<https:/...
> File oracle/peers.go (right):
>
> https://codereview.appspot.**com/9502043/diff/46053/oracle/**
>
peers.go#newcode33<https://codereview.appspot.com/9502043/diff/46053/oracle/peers.go#newcode33>
> oracle/peers.go:33: return nil, o.errorf(o.queryPath[0], "there is no
> send/receive here")
> Is it possible to support "for _ = range ch" (with point on ch) as well
> and treat it like a receive op?
>
>
https://codereview.appspot.**com/9502043/<https://codereview.appspot.com/9502...
>

Good point---I'd forgotten about those implicit receives.  Will fix.

[Dominik Honnef, 2013-08-22 22:42:47]

Sorry for the ton of messages, but I'm kind of discovering independent "issues"
here as I go along in testing.

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el#newcode70
cmd/oracle/oracle.el:70: (defun go-oracle--run (mode)
You seem to be relying on tab-width to be 8, which isn't the case in many
setups, leading to confusing indentation such as (replicated with spaces):

Interface may contain these concrete types:
    Foo, may point to:
  complit

I'm not sure if that's a problem inherent to oracle.el or rather describe.go,
since it also affects other editors, terminals, etc.

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:134: (goto-char (point-min))
You're being confused by the Window Point. The point you're setting in the
buffer does not necessarily match the point that a window is using for that
particular buffer (think two windows displaying the same buffer at two different
positions as one common example of this outside of elisp madness.)

You have two options: Either use goto-char before displaying the buffer, or use
set-window-point. But you probably should display the buffer at the very end,
anyway.

See
http://www.gnu.org/software/emacs/manual/html_node/elisp/Window-Point.html#Wi...
for more information.

[adonovan, 2013-08-26 20:51:56]

> Sorry for the ton of messages, but I'm kind of discovering independent
"issues" here as I go along in testing.

Don't apologise!  These are all good comments---keep 'em coming.

thanks
alan

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el#newcode70
cmd/oracle/oracle.el:70: (defun go-oracle--run (mode)
On 2013/08/22 22:42:48, Dominik Honnef wrote:
> You seem to be relying on tab-width to be 8, which isn't the case in many
> setups, leading to confusing indentation such as (replicated with spaces):
> 
> Interface may contain these concrete types:
>     Foo, may point to:
>   complit
> 
> I'm not sure if that's a problem inherent to oracle.el or rather describe.go,
> since it also affects other editors, terminals, etc.

Good point.  I've fixed this in all places in matters and will avoid it in
future.

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/oracle.el#newcod...
cmd/oracle/oracle.el:134: (goto-char (point-min))
On 2013/08/22 22:42:48, Dominik Honnef wrote:
> You're being confused by the Window Point. The point you're setting in the
> buffer does not necessarily match the point that a window is using for that
> particular buffer (think two windows displaying the same buffer at two
different
> positions as one common example of this outside of elisp madness.)
> 
> You have two options: Either use goto-char before displaying the buffer, or
use
> set-window-point. But you probably should display the buffer at the very end,
> anyway.
> 
> See
>
http://www.gnu.org/software/emacs/manual/html_node/elisp/Window-Point.html#Wi...
> for more information.


D'oh.  Of course.  Fixed.

[Dominik Honnef, 2013-08-26 23:33:09]

https://codereview.appspot.com/9502043/diff/46053/oracle/implements.go
File oracle/implements.go (right):

https://codereview.appspot.com/9502043/diff/46053/oracle/implements.go#newcode9
oracle/implements.go:9: // package-level named types in the containing the query
position.
one "the" too many

https://codereview.appspot.com/9502043/diff/72001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/72001/cmd/oracle/oracle.el#newcode7
cmd/oracle/oracle.el:7: ;;; % go install code.google.com/p/go.tools/cmd/oracle
`go install` is unnecessary, `go get` already does that

[adonovan, 2013-08-27 13:57:55]

https://codereview.appspot.com/9502043/diff/46053/oracle/implements.go
File oracle/implements.go (right):

https://codereview.appspot.com/9502043/diff/46053/oracle/implements.go#newcode9
oracle/implements.go:9: // package-level named types in the containing the query
position.
On 2013/08/26 23:33:09, Dominik Honnef wrote:
> one "the" too many

Oops.  Missing word "package" restored.

https://codereview.appspot.com/9502043/diff/72001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/72001/cmd/oracle/oracle.el#newcode7
cmd/oracle/oracle.el:7: ;;; % go install code.google.com/p/go.tools/cmd/oracle
On 2013/08/26 23:33:09, Dominik Honnef wrote:
> `go install` is unnecessary, `go get` already does that

So it does.  Removed.

[crawshaw1, 2013-08-27 14:19:58]

LGTM

Leaving oracle.el to Dominik.

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/main.go
File cmd/oracle/main.go (right):

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/main.go#newcode49
cmd/oracle/main.go:49: // TODO(adonovan): the caller must---before go/build.init
To avoid confusion for now you could add an init function here that panics if
cgo is enabled.

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go
File oracle/oracle.go (right):

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go#newcode86
oracle/oracle.go:86: return fmt.Errorf("Invalid mode type '%s'.", mode)
%q

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go#newcode108
oracle/oracle.go:108: warnings = append(warnings, warning{pos, format, args})
While I'm reasonably confident there's no issue here, in general I worry about
delaying formatting, as it isn't obvious when you call a printf-like function
that the values being passed in need to be valid later.

https://codereview.appspot.com/9502043/diff/72001/oracle/peers.go
File oracle/peers.go (right):

https://codereview.appspot.com/9502043/diff/72001/oracle/peers.go#newcode31
oracle/peers.go:31: goto found
You could add a function and write return found(arrowPos).

[adonovan, 2013-08-27 15:20:54]

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/main.go
File cmd/oracle/main.go (right):

https://codereview.appspot.com/9502043/diff/46053/cmd/oracle/main.go#newcode49
cmd/oracle/main.go:49: // TODO(adonovan): the caller must---before go/build.init
On 2013/08/27 14:19:59, crawshaw1 wrote:
> To avoid confusion for now you could add an init function here that panics if
> cgo is enabled.

A panic seems excessive; I've made it print a warning for now.

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go
File oracle/oracle.go (right):

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go#newcode86
oracle/oracle.go:86: return fmt.Errorf("Invalid mode type '%s'.", mode)
On 2013/08/27 14:19:59, crawshaw1 wrote:
> %q

Done.

https://codereview.appspot.com/9502043/diff/72001/oracle/oracle.go#newcode108
oracle/oracle.go:108: warnings = append(warnings, warning{pos, format, args})
On 2013/08/27 14:19:59, crawshaw1 wrote:
> While I'm reasonably confident there's no issue here, in general I worry about
> delaying formatting, as it isn't obvious when you call a printf-like function
> that the values being passed in need to be valid later.

Good point; luckily warnf is called only within the pointer analysis package,
not often, and in all cases with immutable values, but I'll certainly keep this
in mind going forward.

I plan to change the pointer API so that all the results are returned via a
Result object with accessors instead of via callbacks.  When I do that, I'll
format the warnings eagerly.

https://codereview.appspot.com/9502043/diff/72001/oracle/peers.go
File oracle/peers.go (right):

https://codereview.appspot.com/9502043/diff/72001/oracle/peers.go#newcode31
oracle/peers.go:31: goto found
On 2013/08/27 14:19:59, crawshaw1 wrote:
> You could add a function and write return found(arrowPos).

I did that initially, but somehow it looked worse.
Done.  (Doesn't seem so bad now.)

[Dominik Honnef, 2013-08-27 15:25:45]

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el#newcode84
cmd/oracle/oracle.el:84: (env (split-string (shell-command-to-string (concat
go-command " env GOROOT GOPATH")) "\n"))
Any particular reason this isn't using go-root-and-paths? Also I'm not sure why
you're only keeping the first GOPATH

[adonovan, 2013-08-27 15:32:21]

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el#newcode84
cmd/oracle/oracle.el:84: (env (split-string (shell-command-to-string (concat
go-command " env GOROOT GOPATH")) "\n"))
On 2013/08/27 15:25:45, Dominik Honnef wrote:
> Any particular reason this isn't using go-root-and-paths? Also I'm not sure
why
> you're only keeping the first GOPATH

go-root-and-paths returns (goroot gopath1 ... gopathn).  This code computes
GOROOT="goroot" GOPATH="gopath1:...:gopathn", so they're not quite the same,
though clearly related.  I could call go-root-and-paths and then join things
back up, but that seemed perverse.

This is a workaround for the fact that oracle is not invoked via the 'go'
command, which in some setups (including mine) is wrapper script that sets
GOPATH and GOROOT based on the working directory.  Hopefully this can all go
away one day.

(Today I learned that shell-command-to-string includes stderr too, which seems
like a bug to me.)

[Dominik Honnef, 2013-08-27 15:36:48]

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el#newcode84
cmd/oracle/oracle.el:84: (env (split-string (shell-command-to-string (concat
go-command " env GOROOT GOPATH")) "\n"))
On 2013/08/27 15:32:21, adonovan wrote:
> On 2013/08/27 15:25:45, Dominik Honnef wrote:
> > Any particular reason this isn't using go-root-and-paths? Also I'm not sure
> why
> > you're only keeping the first GOPATH
> 
> go-root-and-paths returns (goroot gopath1 ... gopathn).  This code computes
> GOROOT="goroot" GOPATH="gopath1:...:gopathn", so they're not quite the same,
> though clearly related.  I could call go-root-and-paths and then join things
> back up, but that seemed perverse.

Ah, never mind the "only the first GOPATH" comment, I thought `go env ...`
returned every GOPATH on a separate line, but that's me doing that. I still
think that using go-root-and-paths is cleaner, since that's just using the head
and concating the tail – you don't replicate the process handling. But that's
your call

> 
> This is a workaround for the fact that oracle is not invoked via the 'go'
> command, which in some setups (including mine) is wrapper script that sets
> GOPATH and GOROOT based on the working directory.  Hopefully this can all go
> away one day.
> 
> (Today I learned that shell-command-to-string includes stderr too, which seems
> like a bug to me.)

shell-command-to-string behaving that way is something a lot of people rely in a
lot of places, including me ;) If you need it separated, there's more advanced
functions for it; if you need stderr to be ignored, redirect it, it's a shell
command after all.

[adonovan, 2013-08-27 16:07:48]

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el
File cmd/oracle/oracle.el (right):

https://codereview.appspot.com/9502043/diff/88001/cmd/oracle/oracle.el#newcode84
cmd/oracle/oracle.el:84: (env (split-string (shell-command-to-string (concat
go-command " env GOROOT GOPATH")) "\n"))
On 2013/08/27 15:36:49, Dominik Honnef wrote:
> On 2013/08/27 15:32:21, adonovan wrote:
> > On 2013/08/27 15:25:45, Dominik Honnef wrote:
> > > Any particular reason this isn't using go-root-and-paths? Also I'm not
sure
> > why
> > > you're only keeping the first GOPATH
> > 
> > go-root-and-paths returns (goroot gopath1 ... gopathn).  This code computes
> > GOROOT="goroot" GOPATH="gopath1:...:gopathn", so they're not quite the same,
> > though clearly related.  I could call go-root-and-paths and then join things
> > back up, but that seemed perverse.
> 
> Ah, never mind the "only the first GOPATH" comment, I thought `go env ...`
> returned every GOPATH on a separate line, but that's me doing that. I still
> think that using go-root-and-paths is cleaner, since that's just using the
head
> and concating the tail – you don't replicate the process handling. But that's
> your call

Ok, done.


> > This is a workaround for the fact that oracle is not invoked via the 'go'
> > command, which in some setups (including mine) is wrapper script that sets
> > GOPATH and GOROOT based on the working directory.  Hopefully this can all go
> > away one day.
> > 
> > (Today I learned that shell-command-to-string includes stderr too, which
seems
> > like a bug to me.)
> 
> shell-command-to-string behaving that way is something a lot of people rely in
a
> lot of places, including me ;) 

Yeah, I have no doubt that it's too late to change it.

> if you need stderr to be ignored, redirect it, it's a shell
> command after all.

Easier said than done in a portable way!

[adonovan, 2013-08-27 21:58:28]

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

go.tools/oracle: an oracle that answers questions about Go source code.

+ Tests.
+ Emacs integration.
+ Emacs integration test.
+ very rudimentary Vim integration.  Needs some love from a Vim user.

TODO (in follow-ups):
- More tests would be good.
  We'll need to make the output order deterministic in more places.
- Documentation.

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

[r, 2013-08-27 22:05:30]

https://codereview.appspot.com/9502043/diff/99001/cmd/oracle/main.go
File cmd/oracle/main.go (right):

https://codereview.appspot.com/9502043/diff/99001/cmd/oracle/main.go#newcode1
cmd/oracle/main.go:1: // oracle: a tool for answering questions about Go source
code.
missing copyright.
check other files too.

where is the documentation?

sorry to arrive late...

[adonovan, 2013-08-27 22:09:07]

On 27 August 2013 18:05, <r@golang.org> wrote:

>
>
https://codereview.appspot.**com/9502043/diff/99001/cmd/**oracle/main.go<http...
> File cmd/oracle/main.go (right):
>
> https://codereview.appspot.**com/9502043/diff/99001/cmd/**
>
oracle/main.go#newcode1<https://codereview.appspot.com/9502043/diff/99001/cmd/oracle/main.go#newcode1>
> cmd/oracle/main.go:1: // oracle: a tool for answering questions about Go
> source code.
> missing copyright.
> check other files too.
>

Oops, will send a CL.


> where is the documentation?
>

Writing it as we speak.