cmd/oracle: usability improvements to "describe", "referrers"

Emacs integration:
- eliminate oracle minor mode
- in go-mode, bind F5, F6 to "describe", "referrers".
  This reverts a previous policy decision but convenience matters too.
- don't insist on an analysis scope for modes that don't do PTA.
- don't hide the filename as "▶"; show the last 20 chars.
  (Especially useful for "referrers" mode.)
- output postprocessing: don't get stuck in a loop if the output
  is not as expected (e.g. when it includes a panic log).

referrers:
- show the matching lines (like grep does).
  We do the I/O in parallel.

Change-Id: I86b18c1d3a4d9fa4242984cba62b314796669d8e
Reviewed-on: https://go-review.googlesource.com/8120
Reviewed-by: David Crawshaw <crawshaw@golang.org>

cmd/oracle/oracle.el

@@ -36,22 +36,21 @@
   nil
   "History of values supplied to `go-oracle-set-scope'.")
 
-;; TODO(adonovan): I'd like to get rid of this separate mode since it
+;; Extend go-mode-map.
-;; makes it harder to use the oracle.
+(let ((m go-mode-map))
-(defvar go-oracle-mode-map
+  (define-key m (kbd "C-c C-o t") #'go-oracle-describe) ; t for type
-  (let ((m (make-sparse-keymap)))
+  (define-key m (kbd "C-c C-o f") #'go-oracle-freevars)
-    (define-key m (kbd "C-c C-o t") #'go-oracle-describe) ; t for type
+  (define-key m (kbd "C-c C-o g") #'go-oracle-callgraph)
-    (define-key m (kbd "C-c C-o f") #'go-oracle-freevars)
+  (define-key m (kbd "C-c C-o i") #'go-oracle-implements)
-    (define-key m (kbd "C-c C-o g") #'go-oracle-callgraph)
+  (define-key m (kbd "C-c C-o c") #'go-oracle-peers)  ; c for channel
-    (define-key m (kbd "C-c C-o i") #'go-oracle-implements)
+  (define-key m (kbd "C-c C-o r") #'go-oracle-referrers)
-    (define-key m (kbd "C-c C-o c") #'go-oracle-peers)  ; c for channel
+  (define-key m (kbd "C-c C-o d") #'go-oracle-definition)
-    (define-key m (kbd "C-c C-o r") #'go-oracle-referrers)
+  (define-key m (kbd "C-c C-o p") #'go-oracle-pointsto)
-    (define-key m (kbd "C-c C-o d") #'go-oracle-definition)
+  (define-key m (kbd "C-c C-o s") #'go-oracle-callstack)
-    (define-key m (kbd "C-c C-o p") #'go-oracle-pointsto)
+  (define-key m (kbd "C-c C-o <") #'go-oracle-callers)
-    (define-key m (kbd "C-c C-o s") #'go-oracle-callstack)
+  (define-key m (kbd "C-c C-o >") #'go-oracle-callees)
-    (define-key m (kbd "C-c C-o <") #'go-oracle-callers)
+  (define-key m (kbd "<f5>") #'go-oracle-describe)
-    (define-key m (kbd "C-c C-o >") #'go-oracle-callees)
+  (define-key m (kbd "<f6>") #'go-oracle-referrers))
-    m))
 
 ;; TODO(dominikh): Rethink set-scope some. Setting it to a file is
 ;; painful because it doesn't use find-file, and variables/~ aren't
@@ -84,11 +83,11 @@ specify to 'go build'."
         (error "You must specify a non-empty scope for the Go oracle"))
     (setq go-oracle-scope scope)))
 
-(defun go-oracle--run (mode)
+(defun go-oracle--run (mode &optional need-scope)
   "Run the Go oracle in the specified MODE, passing it the
-selected region of the current buffer.  Process the output to
+selected region of the current buffer.  If NEED-SCOPE, prompt for
-replace each file name with a small hyperlink.  Display the
+a scope if not already set.  Process the output to replace each
-result."
+file name with a small hyperlink.  Display the result."
   (if (not buffer-file-name)
       (error "Cannot use oracle on a buffer without a file name"))
   ;; It's not sufficient to save a modified buffer since if
@@ -96,8 +95,9 @@ result."
   ;; disturb the selected region.
   (if (buffer-modified-p)
       (error "Please save the buffer before invoking go-oracle"))
-  (if (string-equal "" go-oracle-scope)
+  (and need-scope
-      (go-oracle-set-scope))
+       (string-equal "" go-oracle-scope)
+       (go-oracle-set-scope))
   (let* ((filename (file-truename buffer-file-name))
          (posflag (if (use-region-p)
                       (format "-pos=%s:#%d,#%d"
@@ -107,7 +107,6 @@ result."
                     (format "-pos=%s:#%d"
                             filename
                             (1- (position-bytes (point))))))
-         ;; This would be simpler if we could just run 'go tool oracle'.
          (env-vars (go-root-and-paths))
          (goroot-env (concat "GOROOT=" (car env-vars)))
          (gopath-env (concat "GOPATH=" (mapconcat #'identity (cdr env-vars) ":"))))
@@ -138,14 +137,23 @@ result."
             (p 1))
         (while (not (null p))
           (let ((np (compilation-next-single-property-change p 'compilation-message)))
-            ;; TODO(adonovan): this can be verbose in the *Messages* buffer.
-            ;; (message "Post-processing link (%d%%)" (/ (* p 100) (point-max)))
             (if np
                 (when (equal (line-number-at-pos p) (line-number-at-pos np))
-                  ;; np is (typically) the space following ":"; consume it too.
+                  ;; Using a fixed width greatly improves readability, so
-                  (put-text-property p np 'display "▶")
+                  ;; if the filename is longer than 20, show ".../last/17chars.go".
+                  ;; This usually includes the last segment of the package name.
+                  ;; Don't show the line or column number.
+                  (let* ((loc (buffer-substring p np)) ; "/home/foo/go/pkg/file.go:1:2-3:4"
+                         (i (search ":" loc)))
+                    (setq loc (cond
+                               ((null i)  "...")
+                               ((>= i 17) (concat "..." (substring loc (- i 17) i)))
+                               (t         (substring loc 0 i))))
+                    ;; np is (typically) the space following ":"; consume it too.
+                    (put-text-property p np 'display (concat loc ":")))
                   (goto-char np)
-                  (insert " ")))
+                  (insert " ")
+                  (incf np))) ; so we don't get stuck (e.g. on a panic stack dump)
             (setq p np)))
         (message nil))
 
@@ -157,23 +165,23 @@ result."
 (defun go-oracle-callees ()
   "Show possible callees of the function call at the current point."
   (interactive)
-  (go-oracle--run "callees"))
+  (go-oracle--run "callees" t))
 
 (defun go-oracle-callers ()
   "Show the set of callers of the function containing the current point."
   (interactive)
-  (go-oracle--run "callers"))
+  (go-oracle--run "callers" t))
 
 (defun go-oracle-callgraph ()
   "Show the callgraph of the current program."
   (interactive)
-  (go-oracle--run "callgraph"))
+  (go-oracle--run "callgraph" t))
 
 (defun go-oracle-callstack ()
   "Show an arbitrary path from a root of the call graph to the
 function containing the current point."
   (interactive)
-  (go-oracle--run "callstack"))
+  (go-oracle--run "callstack" t))
 
 (defun go-oracle-definition ()
   "Show the definition of the selected identifier."
@@ -188,7 +196,7 @@ function containing the current point."
 (defun go-oracle-pointsto ()
   "Show what the selected expression points to."
   (interactive)
-  (go-oracle--run "pointsto"))
+  (go-oracle--run "pointsto" t))
 
 (defun go-oracle-implements ()
   "Describe the 'implements' relation for types in the package
@@ -205,25 +213,18 @@ containing the current point."
   "Enumerate the set of possible corresponding sends/receives for
 this channel receive/send operation."
   (interactive)
-  (go-oracle--run "peers"))
+  (go-oracle--run "peers" t))
 
 (defun go-oracle-referrers ()
   "Enumerate all references to the object denoted by the selected
 identifier."
   (interactive)
-  (go-oracle--run "referrers"))
+  (go-oracle--run "referrers" t))
 
 (defun go-oracle-whicherrs ()
   "Show globals, constants and types to which the selected
 expression (of type 'error') may refer."
   (interactive)
-  (go-oracle--run "whicherrs"))
+  (go-oracle--run "whicherrs" t))
-
-;; TODO(dominikh): better docstring
-(define-minor-mode go-oracle-mode "Oracle minor mode for go-mode
-
-Keys specific to go-oracle-mode:
-\\{go-oracle-mode-map}"
-  nil " oracle" go-oracle-mode-map)
 
 (provide 'go-oracle)

oracle/oracle.go

@@ -225,6 +225,13 @@ func Query(args []string, mode, pos string, ptalog io.Writer, buildContext *buil
 
 	conf := loader.Config{Build: buildContext}
 
+	// TODO(adonovan): tolerate type errors if we don't need SSA form.
+	// First we'll need ot audit the non-SSA modes for robustness
+	// in the face of type errors.
+	// if minfo.needs&needSSA == 0 {
+	// 	conf.AllowErrors = true
+	// }
+
 	// Determine initial packages.
 	args, err := conf.FromArgs(args, true)
 	if err != nil {

oracle/referrers.go

@@ -5,15 +5,20 @@
 package oracle
 
 import (
+	"bytes"
 	"fmt"
 	"go/ast"
 	"go/token"
+	"io/ioutil"
 	"sort"
 
 	"golang.org/x/tools/go/types"
 	"golang.org/x/tools/oracle/serial"
 )
 
+// TODO(adonovan): use golang.org/x/tools/refactor/importgraph to choose
+// the scope automatically.
+
 // Referrers reports all identifiers that resolve to the same object
 // as the queried identifier, within any package in the analysis scope.
 //
@@ -41,6 +46,7 @@ func referrers(o *Oracle, qpos *QueryPos) (queryResult, error) {
 	sort.Sort(byNamePos(refs))
 
 	return &referrersResult{
+		qpos:  qpos,
 		query: id,
 		obj:   obj,
 		refs:  refs,
@@ -71,21 +77,54 @@ func (p byNamePos) Less(i, j int) bool { return p[i].NamePos < p[j].NamePos }
 func (p byNamePos) Swap(i, j int)      { p[i], p[j] = p[j], p[i] }
 
 type referrersResult struct {
+	qpos  *QueryPos
 	query *ast.Ident   // identifier of query
 	obj   types.Object // object it denotes
 	refs  []*ast.Ident // set of all other references to it
 }
 
 func (r *referrersResult) display(printf printfFunc) {
-	if r.query.Pos() != r.obj.Pos() {
+	printf(r.obj, "%d references to %s", len(r.refs), r.obj)
-		printf(r.query, "reference to %s", r.obj.Name())
+
+	// Show referring lines, like grep.
+	type fileinfo struct {
+		refs     []*ast.Ident
+		linenums []int       // line number of refs[i]
+		data     chan []byte // file contents
 	}
-	// TODO(adonovan): pretty-print object using same logic as
+	var fileinfos []*fileinfo
-	// (*describeValueResult).display.
+	fileinfosByName := make(map[string]*fileinfo)
-	printf(r.obj, "defined here as %s", r.obj)
+
+	// First pass: start the file reads concurrently.
 	for _, ref := range r.refs {
-		if r.query != ref {
+		posn := r.qpos.fset.Position(ref.Pos())
-			printf(ref, "referenced here")
+		fi := fileinfosByName[posn.Filename]
+		if fi == nil {
+			fi = &fileinfo{data: make(chan []byte)}
+			fileinfosByName[posn.Filename] = fi
+			fileinfos = append(fileinfos, fi)
+
+			// First request for this file:
+			// start asynchronous read.
+			go func() {
+				content, err := ioutil.ReadFile(posn.Filename)
+				if err != nil {
+					content = []byte(fmt.Sprintf("error: %v", err))
+				}
+				fi.data <- content
+			}()
+		}
+		fi.refs = append(fi.refs, ref)
+		fi.linenums = append(fi.linenums, posn.Line)
+	}
+
+	// Second pass: print refs in original order.
+	// One line may have several refs at different columns.
+	for _, fi := range fileinfos {
+		content := <-fi.data // wait for I/O completion
+		lines := bytes.Split(content, []byte("\n"))
+		for i, ref := range fi.refs {
+			printf(ref, "%s", lines[fi.linenums[i]-1])
 		}
 	}
 }