dst icon indicating copy to clipboard operation
dst copied to clipboard

panic when incorrectly formatted code

Open reinierkors opened this issue 5 years ago • 4 comments

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.

reinierkors avatar Aug 26 '20 09:08 reinierkors

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.

dave avatar Aug 26 '20 09:08 dave

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 https://github.com/dave/dst/blob/5fa8d6ebe49a6b04afa15ee8f982d210f8a00b80/decorator/decorator-fragment.go#L128 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.

jlewi avatar Oct 06 '23 00:10 jlewi