Skip to content

Conversation

pcj
Copy link
Contributor

@pcj pcj commented Dec 15, 2021

While investigating googleapis/api-linter#908, added the following log statements:

func (l *linker) checkForUnusedImports(filename string) {
	log.Println("checkForUnusedImports", filename)
	r := l.files[filename]
	usedImports := l.usedImports[r.fd]
	node := r.nodes[r.fd]
	fileNode, _ := node.(*ast.FileNode)
	for i, dep := range r.fd.Dependency {
		log.Println("checkForUnusedImports", i, dep)

		if _, ok := usedImports[dep]; !ok {
			isPublic := false
			// it's fine if it's a public import
			for _, j := range r.fd.PublicDependency {
				if i == int(j) {
					isPublic = true
					break
				}
			}
			if isPublic {
				break
			}
			var pos *SourcePos
			if fileNode != nil {
				log.Printf("checkForUnusedImports fileNode decls: %+v (%T)", fileNode.Decls, fileNode.Decls)

				for i, decl := range fileNode.Decls {
					log.Println("checkForUnusedImports decls", i, decl)

					imp, ok := decl.(*ast.ImportNode)
					if !ok {
						continue
					}
					if imp.Name.AsString() == dep {
						pos = imp.Start()
					}
				}
			}
			if pos == nil {
				pos = ast.UnknownPos(r.fd.GetName())
			}
			log.Println("checkForUnusedImports err warn", r.errs, pos, dep)

			r.errs.warn(pos, errUnusedImport(dep))
		}
	}
}

And observed the following:

2021/12/15 05:33:29 checkForUnusedImports rosetta/rosetta/pluto/v1/service.proto
2021/12/15 05:33:29 checkForUnusedImports 0 google/protobuf/timestamp.proto
2021/12/15 05:33:29 checkForUnusedImports err warn <nil> rosetta/rosetta/pluto/v1/service.proto google/protobuf/timestamp.proto
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x130ea32]

goroutine 1 [running]:
github.com/jhump/protoreflect/desc/protoparse.(*errorHandler).warn(...)
	external/com_github_jhump_protoreflect/desc/protoparse/errors.go:80
github.com/jhump/protoreflect/desc/protoparse.(*linker).checkForUnusedImports(0xc000094c30, 0xc00002a751, 0x26)
	external/com_github_jhump_protoreflect/desc/protoparse/linker.go:931 +0x472
github.com/jhump/protoreflect/desc/protoparse.Parser.ParseFiles(0xc0003ed6c0, 0x1, 0x1, 0x0, 0xc0004cc790, 0x0, 0x0, 0x1, 0xc000129080, 0x0, ...)
	external/com_github_jhump_protoreflect/desc/protoparse/parser.go:238 +0x373
main.(*cli).lint(0xc000143740, 0xc00040faa0, 0x1b47370, 0x2, 0x2, 0x0, 0x0)
	external/com_github_googleapis_api_linter/cmd/api-linter/cli.go:161 +0x427
main.runCLI(0xc000142010, 0xb, 0xb, 0x0, 0x0)
	external/com_github_googleapis_api_linter/cmd/api-linter/main.go:44 +0x85
main.main()
	external/com_github_googleapis_api_linter/cmd/api-linter/main.go:37 +0x76

So this panic occurred because the errs struct on the parseResult was nil. This led me to the current diff, which recovers the panic in my case.

While investigating googleapis/api-linter#908, added the following log statements:

```go
func (l *linker) checkForUnusedImports(filename string) {
	log.Println("checkForUnusedImports", filename)
	r := l.files[filename]
	usedImports := l.usedImports[r.fd]
	node := r.nodes[r.fd]
	fileNode, _ := node.(*ast.FileNode)
	for i, dep := range r.fd.Dependency {
		log.Println("checkForUnusedImports", i, dep)

		if _, ok := usedImports[dep]; !ok {
			isPublic := false
			// it's fine if it's a public import
			for _, j := range r.fd.PublicDependency {
				if i == int(j) {
					isPublic = true
					break
				}
			}
			if isPublic {
				break
			}
			var pos *SourcePos
			if fileNode != nil {
				log.Printf("checkForUnusedImports fileNode decls: %+v (%T)", fileNode.Decls, fileNode.Decls)

				for i, decl := range fileNode.Decls {
					log.Println("checkForUnusedImports decls", i, decl)

					imp, ok := decl.(*ast.ImportNode)
					if !ok {
						continue
					}
					if imp.Name.AsString() == dep {
						pos = imp.Start()
					}
				}
			}
			if pos == nil {
				pos = ast.UnknownPos(r.fd.GetName())
			}
			log.Println("checkForUnusedImports err warn", r.errs, pos, dep)

			r.errs.warn(pos, errUnusedImport(dep))
		}
	}
}
```

And observed the following:

```
2021/12/15 05:33:29 checkForUnusedImports rosetta/rosetta/pluto/v1/service.proto
2021/12/15 05:33:29 checkForUnusedImports 0 google/protobuf/timestamp.proto
2021/12/15 05:33:29 checkForUnusedImports err warn <nil> rosetta/rosetta/pluto/v1/service.proto google/protobuf/timestamp.proto
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x130ea32]

goroutine 1 [running]:
github.com/jhump/protoreflect/desc/protoparse.(*errorHandler).warn(...)
	external/com_github_jhump_protoreflect/desc/protoparse/errors.go:80
github.com/jhump/protoreflect/desc/protoparse.(*linker).checkForUnusedImports(0xc000094c30, 0xc00002a751, 0x26)
	external/com_github_jhump_protoreflect/desc/protoparse/linker.go:931 +0x472
github.com/jhump/protoreflect/desc/protoparse.Parser.ParseFiles(0xc0003ed6c0, 0x1, 0x1, 0x0, 0xc0004cc790, 0x0, 0x0, 0x1, 0xc000129080, 0x0, ...)
	external/com_github_jhump_protoreflect/desc/protoparse/parser.go:238 +0x373
main.(*cli).lint(0xc000143740, 0xc00040faa0, 0x1b47370, 0x2, 0x2, 0x0, 0x0)
	external/com_github_googleapis_api_linter/cmd/api-linter/cli.go:161 +0x427
main.runCLI(0xc000142010, 0xb, 0xb, 0x0, 0x0)
	external/com_github_googleapis_api_linter/cmd/api-linter/main.go:44 +0x85
main.main()
	external/com_github_googleapis_api_linter/cmd/api-linter/main.go:37 +0x76
```

So this panic occurred because the `errs` struct on the `parseResult` was nil.  This led me to the current diff, which recovers the panic in my case.
@pcj
Copy link
Contributor Author

pcj commented Dec 15, 2021

Actually, looking at this a bit longer I think the original intent was #472.

@jhump
Copy link
Owner

jhump commented Mar 16, 2022

Sorry I didn't merge this when I merged #472. This seems like a reasonable CYA change, just in case there are any other places that prefer the parse result's error reporter field.

@jhump jhump merged commit 6774f04 into jhump:master Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants