langserver/internal/refs: use per-file best-effort approach

Stephen Gutekanst · Stephen Gutekanst · commit 219b7f6480da · 2017-01-25T21:54:04.000-07:00
Previously `langserver/internal/refs` would abort on the first error found in a package. After this change, we make a best-effort approach to finding references on a per-file level instead of only at the per-package level at the callsite in `workspace_refs.go`. This is useful because while not all Go packages can be built, e.g. this one that I ran into in the wild which imports a broken-path `ntoolkit/component` package: https://github.com/shadowmint/go-component/blob/master/src/ntoolkit/component/component_fake_component_test.go We can still find useful information from them, e.g. references to `testing.T` above. This also occurs in e.g. this kubernetes staging directory: https://github.com/kubernetes/kubernetes/tree/master/staging So the use cases for this change are real, and there is no real harm in making this change.
diff --git a/langserver/internal/refs/refs.go b/langserver/internal/refs/refs.go
@@ -67,19 +67,19 @@ func (c *Config) Refs(emit func(*Ref)) error {
 		return nil
 	}
 
-	var firstErr error
+	var errs []string
 	for _, file := range c.PkgFiles {
 		ast.Inspect(file, func(n ast.Node) bool {
 			switch n := n.(type) {
 			case *ast.ImportSpec:
 				if err := ref(file, n.Pos(), n.End()); err != nil {
-					firstErr = err
+					errs = append(errs, err.Error())
 					return false
 				}
 
 			case *ast.SelectorExpr:
 				if err := ref(file, n.Sel.Pos(), n.Sel.End()); err != nil {
-					firstErr = err
+					errs = append(errs, err.Error())
 					return false
 				}
 
@@ -101,17 +101,17 @@ func (c *Config) Refs(emit func(*Ref)) error {
 						// since these fall into an edge case (do not represent
 						// real errors for us).
 						if _, ok := err.(*notPackageLevelDef); !ok {
-							firstErr = err
+							errs = append(errs, err.Error())
 							return false
 						}
 					}
 				}
 			}
 			return true
 		})
-		if firstErr != nil {
-			return firstErr
-		}
+	}
+	if len(errs) > 0 {
+		return errors.New(strings.Join(errs, "\n"))
 	}
 	return nil
 }

Original file line number	Diff line number	Diff line change
`@@ -67,19 +67,19 @@ func (c Config) Refs(emit func(Ref)) error {`
`67`	`67`	`return nil`
`68`	`68`	`}`
`69`	`69`
`70`		`- var firstErr error`
	`70`	`+ var errs []string`
`71`	`71`	`for _, file := range c.PkgFiles {`
`72`	`72`	`ast.Inspect(file, func(n ast.Node) bool {`
`73`	`73`	`switch n := n.(type) {`
`74`	`74`	`case *ast.ImportSpec:`
`75`	`75`	`if err := ref(file, n.Pos(), n.End()); err != nil {`
`76`		`- firstErr = err`
	`76`	`+ errs = append(errs, err.Error())`
`77`	`77`	`return false`
`78`	`78`	`}`
`79`	`79`
`80`	`80`	`case *ast.SelectorExpr:`
`81`	`81`	`if err := ref(file, n.Sel.Pos(), n.Sel.End()); err != nil {`
`82`		`- firstErr = err`
	`82`	`+ errs = append(errs, err.Error())`
`83`	`83`	`return false`
`84`	`84`	`}`
`85`	`85`
`@@ -101,17 +101,17 @@ func (c Config) Refs(emit func(Ref)) error {`
`101`	`101`	`// since these fall into an edge case (do not represent`
`102`	`102`	`// real errors for us).`
`103`	`103`	`if _, ok := err.(*notPackageLevelDef); !ok {`
`104`		`- firstErr = err`
	`104`	`+ errs = append(errs, err.Error())`
`105`	`105`	`return false`
`106`	`106`	`}`
`107`	`107`	`}`
`108`	`108`	`}`
`109`	`109`	`}`
`110`	`110`	`return true`
`111`	`111`	`})`
`112`		`- if firstErr != nil {`
`113`		`- return firstErr`
`114`		`- }`
	`112`	`+ }`
	`113`	`+ if len(errs) > 0 {`
	`114`	`+ return errors.New(strings.Join(errs, "\n"))`
`115`	`115`	`}`
`116`	`116`	`return nil`
`117`	`117`	`}`