Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Commit

Permalink
API docs: add tests & fix issue indexing illegal moby/moby Go code (#189
Browse files Browse the repository at this point in the history
)

* API docs: add test for failure to index moby/moby illegal Go code
* CHANGELOG: add v1.6.6 entry

Signed-off-by: Stephen Gutekanst <[email protected]>
  • Loading branch information
Stephen Gutekanst authored Aug 13, 2021
1 parent f53d65f commit 6860e46
Show file tree
Hide file tree
Showing 9 changed files with 350 additions and 10 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ All notable changes to `lsif-go` are documented in this file.

## Unreleased changes

## v1.6.6

### Fixed

- An issue where illegal code (conflicting test/non-test symbol names, such as in some moby/moby packages) would fail to index. [#186](https://github.com/sourcegraph/lsif-go/pull/186)

## v1.6.5

### Fixed
Expand Down
34 changes: 26 additions & 8 deletions internal/indexer/documentation.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,13 +530,17 @@ func (d *docsIndexer) indexGenDecl(p *packages.Package, f *ast.File, node *ast.G
}
switch node.Tok {
case token.CONST:
constDocs := d.indexConstVar(p, t, i, "const", isTestFile)
constDocs.docsMarkdown = blockDocsMarkdown + constDocs.docsMarkdown
result.consts = append(result.consts, constDocs)
constDocs, ok := d.indexConstVar(p, t, i, "const", isTestFile)
if ok {
constDocs.docsMarkdown = blockDocsMarkdown + constDocs.docsMarkdown
result.consts = append(result.consts, constDocs)
}
case token.VAR:
varDocs := d.indexConstVar(p, t, i, "var", isTestFile)
varDocs.docsMarkdown = blockDocsMarkdown + varDocs.docsMarkdown
result.vars = append(result.vars, varDocs)
varDocs, ok := d.indexConstVar(p, t, i, "var", isTestFile)
if ok {
varDocs.docsMarkdown = blockDocsMarkdown + varDocs.docsMarkdown
result.vars = append(result.vars, varDocs)
}
}
}
case *ast.TypeSpec:
Expand Down Expand Up @@ -601,13 +605,27 @@ func (t constVarDocs) result() *documentationResult {
}
}

func (d *docsIndexer) indexConstVar(p *packages.Package, in *ast.ValueSpec, nameIndex int, typ string, isTestFile bool) constVarDocs {
func (d *docsIndexer) indexConstVar(p *packages.Package, in *ast.ValueSpec, nameIndex int, typ string, isTestFile bool) (constVarDocs, bool) {
var result constVarDocs
name := in.Names[nameIndex]
result.label = fmt.Sprintf("%s %s", typ, name.String())
result.name = name.String()
result.searchKey = p.Name + "." + name.String()
result.def = p.TypesInfo.Defs[name]
if result.def == nil {
// TODO(slimsag): some packages have illegally conflicting symbols, like:
//
// * testdata/conflicting_test_symbols/sandbox_unsupported.go:ErrNotImplemented
// * testdata/conflicting_test_symbols/sandbox_unsupported_test.go:ErrNotImplemented
//
// These examples were pulled from the moby/moby project in the wild (so yes, it really
// happens.) In such cases, p.TypesInfo.Defs here would be `nil`. One could argue this is
// a bug in go/packages in which, because both files are analyzed, one identifier "wins"
// (say, the test file's) while the other becomes `nil` - but I would argue it's not
// go/packages responsibility necessarily to handle analyzing illegal Go code aside from in
// a best-effort way, so we should handle this in a best-effort way, too: drop the symbol.
return constVarDocs{}, false
}

result.tags = []protocol.Tag{}
if typ == "const" {
Expand Down Expand Up @@ -638,7 +656,7 @@ func (d *docsIndexer) indexConstVar(p *packages.Package, in *ast.ValueSpec, name
}

result.docsMarkdown = godocToMarkdown(in.Doc.Text())
return result
return result, true
}

type typeDocs struct {
Expand Down
7 changes: 5 additions & 2 deletions internal/indexer/indexer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,11 @@ func TestIndexer_shouldVisitPackage(t *testing.T) {
}
}
autogold.Want("visited", map[string]bool{
"github.com/sourcegraph/lsif-go/internal/testdata": true,
"github.com/sourcegraph/lsif-go/internal/testdata/duplicate_path_id": true,
"github.com/sourcegraph/lsif-go/internal/testdata": true,
"github.com/sourcegraph/lsif-go/internal/testdata/conflicting_test_symbols": false,
"github.com/sourcegraph/lsif-go/internal/testdata/conflicting_test_symbols [github.com/sourcegraph/lsif-go/internal/testdata/conflicting_test_symbols.test]": true,
"github.com/sourcegraph/lsif-go/internal/testdata/conflicting_test_symbols.test": false,
"github.com/sourcegraph/lsif-go/internal/testdata/duplicate_path_id": true,
"…/secret": true,
"…/shouldvisit/notests": true,
"…/shouldvisit/tests": false,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
{
"pathID": "/conflicting_test_symbols",
"documentation": {
"identifier": "conflicting_test_symbols",
"newPage": true,
"searchKey": "conflicting_test_symbols",
"tags": [
"private",
"package"
]
},
"label": {
"kind": "plaintext",
"value": "Package osl"
},
"detail": {
"kind": "markdown",
"value": ""
},
"children": [
{
"node": {
"pathID": "/conflicting_test_symbols#var",
"documentation": {
"identifier": "var",
"newPage": false,
"searchKey": "",
"tags": [
"private"
]
},
"label": {
"kind": "plaintext",
"value": "Variables"
},
"detail": {
"kind": "plaintext",
"value": ""
},
"children": [
{
"node": {
"pathID": "/conflicting_test_symbols#ErrNotImplemented",
"documentation": {
"identifier": "ErrNotImplemented",
"newPage": false,
"searchKey": "osl.ErrNotImplemented",
"tags": [
"variable",
"interface"
]
},
"label": {
"kind": "plaintext",
"value": "var ErrNotImplemented"
},
"detail": {
"kind": "markdown",
"value": "```Go\nvar ErrNotImplemented = errors.New(\"not implemented\")\n```\n\nErrNotImplemented is for platforms which don't implement sandbox \n\n"
},
"children": null
}
}
]
}
},
{
"node": {
"pathID": "/conflicting_test_symbols#func",
"documentation": {
"identifier": "func",
"newPage": false,
"searchKey": "",
"tags": [
"private"
]
},
"label": {
"kind": "plaintext",
"value": "Functions"
},
"detail": {
"kind": "plaintext",
"value": ""
},
"children": [
{
"node": {
"pathID": "/conflicting_test_symbols#GenerateKey",
"documentation": {
"identifier": "GenerateKey",
"newPage": false,
"searchKey": "osl.GenerateKey",
"tags": [
"function"
]
},
"label": {
"kind": "plaintext",
"value": "func GenerateKey(containerID string) string"
},
"detail": {
"kind": "markdown",
"value": "```Go\nfunc GenerateKey(containerID string) string\n```\n\nGenerateKey generates a sandbox key based on the passed container id. \n\n"
},
"children": null
}
},
{
"node": {
"pathID": "/conflicting_test_symbols#NewSandbox",
"documentation": {
"identifier": "NewSandbox",
"newPage": false,
"searchKey": "osl.NewSandbox",
"tags": [
"function"
]
},
"label": {
"kind": "plaintext",
"value": "func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error)"
},
"detail": {
"kind": "markdown",
"value": "```Go\nfunc NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error)\n```\n\nNewSandbox provides a new sandbox instance created in an os specific way provided a key which uniquely identifies the sandbox \n\n"
},
"children": null
}
},
{
"node": {
"pathID": "/conflicting_test_symbols#newKey",
"documentation": {
"identifier": "newKey",
"newPage": false,
"searchKey": "osl.newKey",
"tags": [
"function",
"private"
]
},
"label": {
"kind": "plaintext",
"value": "func newKey(t *testing.T) (string, error)"
},
"detail": {
"kind": "markdown",
"value": "```Go\nfunc newKey(t *testing.T) (string, error)\n```\n\n"
},
"children": null
}
},
{
"node": {
"pathID": "/conflicting_test_symbols#verifySandbox",
"documentation": {
"identifier": "verifySandbox",
"newPage": false,
"searchKey": "osl.verifySandbox",
"tags": [
"function",
"private"
]
},
"label": {
"kind": "plaintext",
"value": "func verifySandbox(t *testing.T, s Sandbox)"
},
"detail": {
"kind": "markdown",
"value": "```Go\nfunc verifySandbox(t *testing.T, s Sandbox)\n```\n\n"
},
"children": null
}
}
]
}
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# Package osl

## Index

* [Variables](#var)
* [var ErrNotImplemented](#ErrNotImplemented)
* [Functions](#func)
* [func GenerateKey(containerID string) string](#GenerateKey)
* [func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error)](#NewSandbox)
* [func newKey(t *testing.T) (string, error)](#newKey)
* [func verifySandbox(t *testing.T, s Sandbox)](#verifySandbox)


## <a id="var" href="#var">Variables</a>

```
tags: [private]
```

### <a id="ErrNotImplemented" href="#ErrNotImplemented">var ErrNotImplemented</a>

```
searchKey: osl.ErrNotImplemented
tags: [variable interface]
```

```Go
var ErrNotImplemented = errors.New("not implemented")
```

ErrNotImplemented is for platforms which don't implement sandbox

## <a id="func" href="#func">Functions</a>

```
tags: [private]
```

### <a id="GenerateKey" href="#GenerateKey">func GenerateKey(containerID string) string</a>

```
searchKey: osl.GenerateKey
tags: [function]
```

```Go
func GenerateKey(containerID string) string
```

GenerateKey generates a sandbox key based on the passed container id.

### <a id="NewSandbox" href="#NewSandbox">func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error)</a>

```
searchKey: osl.NewSandbox
tags: [function]
```

```Go
func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error)
```

NewSandbox provides a new sandbox instance created in an os specific way provided a key which uniquely identifies the sandbox

### <a id="newKey" href="#newKey">func newKey(t *testing.T) (string, error)</a>

```
searchKey: osl.newKey
tags: [function private]
```

```Go
func newKey(t *testing.T) (string, error)
```

### <a id="verifySandbox" href="#verifySandbox">func verifySandbox(t *testing.T, s Sandbox)</a>

```
searchKey: osl.verifySandbox
tags: [function private]
```

```Go
func verifySandbox(t *testing.T, s Sandbox)
```

Original file line number Diff line number Diff line change
Expand Up @@ -1131,6 +1131,9 @@
]
}
},
{
"pathID": "/conflicting_test_symbols"
},
{
"pathID": "/duplicate_path_id"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ testdata is a small package containing sample Go source code used for testing th

* Subpages
* [internal](internal.md)
* [conflicting_test_symbols](conflicting_test_symbols.md)
* [duplicate_path_id](duplicate_path_id.md)
* [Constants](#const)
* [const AliasedString](#AliasedString)
Expand Down
23 changes: 23 additions & 0 deletions internal/testdata/conflicting_test_symbols/sandbox_unsupported.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// From https://github.com/moby/moby/blob/master/libnetwork/osl/sandbox_unsupported.go
// Build tag constraints removed here to ensure this code is tested on CI.

package osl

import "errors"

var (
// ErrNotImplemented is for platforms which don't implement sandbox
ErrNotImplemented = errors.New("not implemented")
)

// NewSandbox provides a new sandbox instance created in an os specific way
// provided a key which uniquely identifies the sandbox
func NewSandbox(key string, osCreate, isRestore bool) (Sandbox, error) {
return nil, ErrNotImplemented
}

// GenerateKey generates a sandbox key based on the passed
// container id.
func GenerateKey(containerID string) string {
return ""
}
Loading

0 comments on commit 6860e46

Please sign in to comment.