panic when incorrectly formatted code

[reinierkors]

Hello,

This might be a real edge case, but just wanting to share.
When for example, missing a package main you will get a panic:
panic: runtime error: invalid memory address or nil pointer dereference

func TestDecorator(t *testing.T) {
	code := `import "github.com/MyPackage/Name/package1"


func main() {
	fmt.Println("Hello!")
}`
	decorator.Parse(code)
}

I think earlier versions did not fail this test, and feel like it should gracefully return an error.

[dave]

It's been a while since I created the dst package so I'd have to familiarise myself with the internals before making a call on this one. But, a panic is definitely not ideal and it should be returning an error.

However, I'm pretty sure you'd never want to feed any less than an entire valid go file into dst... because in ast the comments are stored on the ast.File object, so trying to parse and decorate a fragment (just an ast.Node on it's own without the ast.File) wouldn't work at all. I guess in your example this is more of a mistake rather than the intention.

[jlewi]

I hit this as well when using ParseFile.

fset := token.NewFileSet()
fset.AddFile(path, 1, 0)
root, err := decorator.ParseFile(fset, path, nil, parser.ParseComments)

The contents of the file being parsed.

func (s *SomeStruct) SomeFunc() {}

This ends up causing a panic here

dst/decorator/decorator-fragment.go

Line 128 in 5fa8d6e

max := tokenf.Base() + tokenf.Size()

Because tokenf is nil.

Its nil because astf.Pos() uses the package line to determine the first line of the file

https://github.com/golang/go/blob/10da3b64af1aebfd146fa3b7ecf765ee1b0f0d7d/src/go/ast/ast.go#L1054

But f.package is nil because there is no package statement in the file.

In my case its expected that files maybe missing the package statement; I was hoping to use the dst package to fix that. But it looks like the dst package will panic in that case.

Not expecting a fix since it looks like the package isn't being actively developed. Just intended as an FYI for anyone else hitting this issue. I'll probably work around it by ensuring there is a package statement in all my code.

[Toliak]

Hello! I have run into the same problem. Is there any update on that issue?

Description

The segmentation violation happens when the decorator.ParseFile method parses an empty file.

Steps to reproduce

0. Prepare project files

go.mod

module example/dst-reproduce

go 1.18

require github.com/dave/dst v0.27.3

require (
	golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4 // indirect
	golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f // indirect
	golang.org/x/tools v0.1.12 // indirect
)

main.go

package main

import (
	"go/parser"
	"go/token"
	"log"

	"github.com/dave/dst/decorator"
)

func main() {
	filePath := "./test-code.go"
	fset := token.NewFileSet()

	fileNode, err := decorator.ParseFile(fset, filePath, nil, parser.ParseComments)
	if err != nil {
		log.Printf("Error parsing %s: %v\n", filePath, err)
		return
	}

	log.Printf("File node: %#v\n", fileNode)
}

1. Create test-code.go (example provided below): Empty
2. Run go run main.go
3. Receive: Segmentation fault (not ok)

Examples

Behavior if test-code.go empty (wrong)

echo >test-code.go

go run main.go
# panic: runtime error: invalid memory address or nil pointer dereference
# [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x5094fa]
# 
# goroutine 1 [running]:
# go/token.(*File).Base(...)
#         /usr/lib/go-1.19/src/go/token/position.go:113
# github.com/dave/dst/decorator.(*fileDecorator).fragment.func1(0xc0000bc280)
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/decorator-fragment.go:128 +0x15a
# github.com/dave/dst/decorator.(*fileDecorator).fragment(0xc000057d60, {0x59b790?, 0xc0000bc280?})
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/decorator-fragment.go:170 +0xb7
# github.com/dave/dst/decorator.(*Decorator).DecorateNode(0xc0000bf030, {0x59b790?, 0xc0000bc280?})
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/decorator.go:131 +0x13c
# github.com/dave/dst/decorator.(*Decorator).DecorateFile(...)
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/decorator.go:109
# github.com/dave/dst/decorator.(*Decorator).ParseFile(0xc0000bf030, {0x56ab08?, 0x7fffd832e998?}, {0x0?, 0x0?}, 0x40?)
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/decorator.go:81 +0x70
# github.com/dave/dst/decorator.ParseFile(0x7fffff7bf5b8?, {0x56ab08, 0xe}, {0x0, 0x0}, 0x405bf8?)
#         /root/go/pkg/mod/github.com/dave/dst@v0.27.3/decorator/helpers.go:23 +0x179
# main.main()
#         /data/main.go:15 +0x48
# exit status 2

echo $?
# 1

OS Info

• Launched in the docker. Image used: debian:bookworm-20240812, platform: linux/amd64
• Go version: go1.19.8 linux/amd64

[dave]

As mentioned before, dst is only designed to process proper, compilable code. So fragments and any code that the Go compiler can't compile will break it. This is by design.

I agree an error would be preferable to a panic, but as you point out I haven't worked on this codebase for years so I don't think this is enough of a bug to warrant more work.