Skip to content

Commit

Permalink
fix: handle comment parsing with windows line endings (dagger#8217)
Browse files Browse the repository at this point in the history 
Our comment parsing code was previously very inflexible, and didn't
handle carriage returns in the comment.

This is because of this helpful little note in go's comment ast struct
declaration:

	// The Text field contains the comment text without carriage returns (\r) that
	// may have been present in the source. Because a comment's end position is
	// computed using len(Text), the position reported by [Comment.End] does not match the
	// true source end position for comments containing carriage returns.

So calling `.End()` on a comment essentially returns absolutely nonsense
when there are carriage returns - there is no good way of recovering
these.

Instead, we switch to another approach of detecting comment doc+line
comments - instead of attempting to look for the last element of the
line in the comment, we perform an entirely line-based approach, by
checking if the comment starts/ends on the correct line.

This is relatively fiddly, and is a bit fragile, so added some more
tests as well. Also, handling block comments `/* */` becomes a bit
trickier, so extended tests to handle those as well.

Signed-off-by: Justin Chadwell <[email protected]>
  • Loading branch information
@jedevc
jedevc authored Aug 28, 2024
1 parent c5b8ecb commit e1ff54f
Show file tree
Hide file tree
Showing 4 changed files with 224 additions and 145 deletions.
31 changes: 18 additions & 13 deletions cmd/codegen/generator/go/templates/modules.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -998,24 +998,29 @@ func (ps *parseState) commentForFuncField(fnDecl *ast.FuncDecl, unpackedParams [
}

if allowDocComment {
// take the last position in the last line, and try and find a
// comment that contains it
npos := tokenFile.LineStart(tokenFile.Line(pos)) - 1
for _, comment := range f.Comments {
if comment.Pos() <= npos && npos <= comment.End() {
docComment = comment
// find the line that ends the last comment, and check that
// it's right before the declaration
for _, commentGroup := range f.Comments {
comment := commentGroup.List[len(commentGroup.List)-1]

// we do this little line-counting dance since comment.End()
// returns nonsense when we have carriage returns
lastLine := tokenFile.Line(comment.Pos()) + strings.Count(comment.Text, "\n")
if lastLine == line || lastLine == line-1 {
docComment = commentGroup
break
}
}
}

if allowLineComment {
// take the last position in the current line, and try and find a
// comment that contains it
npos := tokenFile.LineStart(tokenFile.Line(pos)+1) - 1
for _, comment := range f.Comments {
if comment.Pos() <= npos && npos <= comment.End() {
lineComment = comment
// find the line that starts the comment and check that's on
// the same line as the declaration
for _, commentGroup := range f.Comments {
comment := commentGroup.List[0]

if tokenFile.Line(comment.Pos()) == line {
lineComment = commentGroup
break
}
}
Expand Down Expand Up @@ -1096,7 +1101,7 @@ func (ps *parseState) functionCallArgCode(t types.Type, access *Statement) (type
}
}

var pragmaCommentRegexp = regexp.MustCompile(`\+\s*(\S+?)(?:=(.+?))?(?:\r?\n|$)`)
var pragmaCommentRegexp = regexp.MustCompile(`[ \t]*\+[ \t]*(\S+?)(?:=(.+?))?(?:\r?\n|$)`)

// parsePragmaComment parses a dagger "pragma", that is used to define additional metadata about a parameter.
func parsePragmaComment(comment string) (data map[string]string, rest string) {
Expand Down
8 changes: 8 additions & 0 deletions cmd/codegen/generator/go/templates/modules_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ func TestParsePragmaComment(t *testing.T) {
},
rest: "",
},
{
name: "single key with leading whitespace",
comment: " \t +foo",
expected: map[string]string{
"foo": "",
},
rest: "",
},
{
name: "single key-value",
comment: "+foo=bar",
Expand Down
292 changes: 173 additions & 119 deletions core/integration/module_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1239,68 +1239,70 @@ func (b *Baz) Hello() (string, error) {
func (ModuleSuite) TestGoDocs(ctx context.Context, t *testctx.T) {
c := connect(ctx, t)

modGen := c.Container().From(golangImage).
WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
WithWorkdir("/work").
With(daggerExec("init", "--source=.", "--name=minimal", "--sdk=go")).
WithNewFile("main.go", goSignatures)
lineEndings := map[string]string{
"lf": "\n",
"crlf": "\r\n",
}
for name, lineEnding := range lineEndings {
t.Run(name, func(ctx context.Context, t *testctx.T) {
modGen := c.Container().From(golangImage).
WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
WithWorkdir("/work").
With(daggerExec("init", "--source=.", "--name=minimal", "--sdk=go")).
WithNewFile("main.go", strings.ReplaceAll(goSignatures, "\n", lineEnding))

logGen(ctx, t, modGen.Directory("."))

obj := inspectModuleObjects(ctx, t, modGen).Get("0")
require.Equal(t, "Minimal", obj.Get("name").String())

logGen(ctx, t, modGen.Directory("."))

obj := inspectModuleObjects(ctx, t, modGen).Get("0")
require.Equal(t, "Minimal", obj.Get("name").String())

hello := obj.Get(`functions.#(name="hello")`)
require.Equal(t, "hello", hello.Get("name").String())
require.Empty(t, hello.Get("description").String())
require.Empty(t, hello.Get("args").Array())

// test the args-based form
echoOpts := obj.Get(`functions.#(name="echoOpts")`)
require.Equal(t, "echoOpts", echoOpts.Get("name").String())
require.Equal(t, "EchoOpts does some opts things", echoOpts.Get("description").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "the message to echo", echoOpts.Get("args.0.description").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())

// test the inline struct form
echoOpts = obj.Get(`functions.#(name="echoOptsInline")`)
require.Equal(t, "echoOptsInline", echoOpts.Get("name").String())
require.Equal(t, "EchoOptsInline does some opts things", echoOpts.Get("description").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "the message to echo", echoOpts.Get("args.0.description").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())

// test the arg-based form (with pragmas)
echoOpts = obj.Get(`functions.#(name="echoOptsPragmas")`)
require.Equal(t, "echoOptsPragmas", echoOpts.Get("name").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "", echoOpts.Get("args.0.defaultValue").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "\"...\"", echoOpts.Get("args.1.defaultValue").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "3", echoOpts.Get("args.2.defaultValue").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())
hello := obj.Get(`functions.#(name="hello")`)
require.Equal(t, "hello", hello.Get("name").String())
require.Empty(t, hello.Get("description").String())
require.Empty(t, hello.Get("args").Array())

// test the args-based form
echoOpts := obj.Get(`functions.#(name="echoOpts")`)
require.Equal(t, "echoOpts", echoOpts.Get("name").String())
require.Equal(t, "EchoOpts does some opts things", echoOpts.Get("description").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "the message to echo", echoOpts.Get("args.0.description").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())

// test the inline struct form
echoOpts = obj.Get(`functions.#(name="echoOptsInline")`)
require.Equal(t, "echoOptsInline", echoOpts.Get("name").String())
require.Equal(t, "EchoOptsInline does some opts things", echoOpts.Get("description").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "the message to echo", echoOpts.Get("args.0.description").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())

// test the arg-based form (with pragmas)
echoOpts = obj.Get(`functions.#(name="echoOptsPragmas")`)
require.Equal(t, "echoOptsPragmas", echoOpts.Get("name").String())
require.Len(t, echoOpts.Get("args").Array(), 3)
require.Equal(t, "msg", echoOpts.Get("args.0.name").String())
require.Equal(t, "", echoOpts.Get("args.0.defaultValue").String())
require.Equal(t, "suffix", echoOpts.Get("args.1.name").String())
require.Equal(t, "String to append to the echoed message", echoOpts.Get("args.1.description").String())
require.Equal(t, "\"...\"", echoOpts.Get("args.1.defaultValue").String())
require.Equal(t, "times", echoOpts.Get("args.2.name").String())
require.Equal(t, "3", echoOpts.Get("args.2.defaultValue").String())
require.Equal(t, "Number of times to repeat the message", echoOpts.Get("args.2.description").String())
})
}
}

func (ModuleSuite) TestGoDocsEdgeCases(ctx context.Context, t *testctx.T) {
c := connect(ctx, t)

modGen := c.Container().From(golangImage).
WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
WithWorkdir("/work").
With(daggerExec("init", "--source=.", "--name=minimal", "--sdk=go")).
WithNewFile("main.go", `package main
src := `package main
// Minimal is a thing
type Minimal struct {
Expand Down Expand Up @@ -1347,68 +1349,120 @@ func (m *Minimal) HelloFinal(
foo string) string { // woops
return foo
}
`,
)
logGen(ctx, t, modGen.Directory("."))

obj := inspectModuleObjects(ctx, t, modGen).Get("0")
require.Equal(t, "Minimal", obj.Get("name").String())
require.Equal(t, "Minimal is a thing", obj.Get("description").String())

hello := obj.Get(`functions.#(name="hello")`)
require.Equal(t, "hello", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 5)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "", hello.Get("args.1.description").String())
require.Equal(t, "baz", hello.Get("args.2.name").String())
require.Equal(t, "hello", hello.Get("args.2.description").String())
require.Equal(t, "qux", hello.Get("args.3.name").String())
require.Equal(t, "", hello.Get("args.3.description").String())
require.Equal(t, "x", hello.Get("args.4.name").String())
require.Equal(t, "lol", hello.Get("args.4.description").String())

hello = obj.Get(`functions.#(name="helloMore")`)
require.Equal(t, "helloMore", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 2)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo here", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "bar here", hello.Get("args.1.description").String())

hello = obj.Get(`functions.#(name="helloMoreInline")`)
require.Equal(t, "helloMoreInline", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 2)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo here", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "", hello.Get("args.1.description").String())

hello = obj.Get(`functions.#(name="helloAgain")`)
require.Equal(t, "helloAgain", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 3)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "docs for bar", hello.Get("args.1.description").String())
require.Equal(t, "baz", hello.Get("args.2.name").String())
require.Equal(t, "", hello.Get("args.2.description").String())

hello = obj.Get(`functions.#(name="helloFinal")`)
require.Equal(t, "helloFinal", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 1)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())

require.Len(t, obj.Get(`fields`).Array(), 2)
prop := obj.Get(`fields.#(name="x")`)
require.Equal(t, "x", prop.Get("name").String())
require.Equal(t, "X is this", prop.Get("description").String())
prop = obj.Get(`fields.#(name="y")`)
require.Equal(t, "y", prop.Get("name").String())
require.Equal(t, "", prop.Get("description").String())
func (m *Minimal) HelloMultiple(
foo string, /* foo 1 */ /* foo 2 */
/* bar 1 */ /* bar 2 */
bar string,
) string {
return foo + bar
}
func (m *Minimal) HelloWeird(
/* foo
data */ foo string,
bar /* bar data */ string,
/* baz data */ baz string,
) string {
return foo + bar + baz
}
`

c := connect(ctx, t)

lineEndings := map[string]string{
"lf": "\n",
"crlf": "\r\n",
}
for name, lineEnding := range lineEndings {
t.Run(name, func(ctx context.Context, t *testctx.T) {
modGen := c.Container().From(golangImage).
WithMountedFile(testCLIBinPath, daggerCliFile(t, c)).
WithWorkdir("/work").
With(daggerExec("init", "--source=.", "--name=minimal", "--sdk=go")).
WithNewFile("main.go", strings.ReplaceAll(src, "\n", lineEnding))

logGen(ctx, t, modGen.Directory("."))

obj := inspectModuleObjects(ctx, t, modGen).Get("0")
require.Equal(t, "Minimal", obj.Get("name").String())
require.Equal(t, "Minimal is a thing", obj.Get("description").String())

hello := obj.Get(`functions.#(name="hello")`)
require.Equal(t, "hello", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 5)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "", hello.Get("args.1.description").String())
require.Equal(t, "baz", hello.Get("args.2.name").String())
require.Equal(t, "hello", hello.Get("args.2.description").String())
require.Equal(t, "qux", hello.Get("args.3.name").String())
require.Equal(t, "", hello.Get("args.3.description").String())
require.Equal(t, "x", hello.Get("args.4.name").String())
require.Equal(t, "lol", hello.Get("args.4.description").String())

hello = obj.Get(`functions.#(name="helloMore")`)
require.Equal(t, "helloMore", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 2)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo here", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "bar here", hello.Get("args.1.description").String())

hello = obj.Get(`functions.#(name="helloMoreInline")`)
require.Equal(t, "helloMoreInline", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 2)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo here", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "", hello.Get("args.1.description").String())

hello = obj.Get(`functions.#(name="helloAgain")`)
require.Equal(t, "helloAgain", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 3)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "docs for bar", hello.Get("args.1.description").String())
require.Equal(t, "baz", hello.Get("args.2.name").String())
require.Equal(t, "", hello.Get("args.2.description").String())

hello = obj.Get(`functions.#(name="helloFinal")`)
require.Equal(t, "helloFinal", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 1)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "", hello.Get("args.0.description").String())

hello = obj.Get(`functions.#(name="helloMultiple")`)
require.Equal(t, "helloMultiple", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 2)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo 1\n foo 2", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "bar 1\n bar 2", hello.Get("args.1.description").String())

hello = obj.Get(`functions.#(name="helloWeird")`)
require.Equal(t, "helloWeird", hello.Get("name").String())
require.Len(t, hello.Get("args").Array(), 3)
require.Equal(t, "foo", hello.Get("args.0.name").String())
require.Equal(t, "foo\ndata", hello.Get("args.0.description").String())
require.Equal(t, "bar", hello.Get("args.1.name").String())
require.Equal(t, "bar data", hello.Get("args.1.description").String())
require.Equal(t, "baz", hello.Get("args.2.name").String())
require.Equal(t, "baz data", hello.Get("args.2.description").String())

require.Len(t, obj.Get(`fields`).Array(), 2)
prop := obj.Get(`fields.#(name="x")`)
require.Equal(t, "x", prop.Get("name").String())
require.Equal(t, "X is this", prop.Get("description").String())
prop = obj.Get(`fields.#(name="y")`)
require.Equal(t, "y", prop.Get("name").String())
require.Equal(t, "", prop.Get("description").String())
})
}
}

func (ModuleSuite) TestGoWeirdFields(ctx context.Context, t *testctx.T) {
Expand Down
Loading

0 comments on commit e1ff54f

Please sign in to comment.