Fix bug with a tuple rhs #96

DavidArchibald · 2024-04-09T01:11:12Z

This ended up being a bit larger of a change than I had anticipated because of the deprecation of ast.Object.

DavidArchibald · 2024-04-09T01:11:33Z

cmd/errtrace/main.go

@@ -417,7 +419,7 @@ type parsedFile struct {

 func (cmd *mainCmd) parseFile(filename string, src []byte, opts rewriteOpts) (parsedFile, error) {
 	fset := token.NewFileSet()
-	f, err := parser.ParseFile(fset, filename, src, parser.ParseComments)
+	f, err := parser.ParseFile(fset, filename, src, parser.ParseComments|parser.SkipObjectResolution)


object resolution is now handled by go/types.

DavidArchibald · 2024-04-09T01:11:48Z

cmd/errtrace/main.go

+	config := types.Config{
+		Importer: importer.Default(),
+		Error: func(err error) {
+			// Ignore type checking errors.
+		},
+	}
+
+	// Ignore any error while parsing, attempt to gracefully handle it anyways.
+	pkg, _ := config.Check(filename, fset, []*ast.File{f}, nil)


This is the config that seems to be more resilient to errors.

DavidArchibald · 2024-04-09T01:12:14Z

cmd/errtrace/main.go

-	errorIndices []int                    // indices of error return values
+	numReturns   int                 // number of return values
+	errorIdents  []*ast.Ident        // identifiers for error return values (only if unnamed returns)
+	errorNames   map[string]struct{} // objects for error return values (only if named returns)


I'm unsure if this functionality is identical to before. It passes all tests but maybe there was a good reason to not store the name.

DavidArchibald · 2024-04-09T01:13:38Z

cmd/errtrace/main.go

 			}

-			// Assigning to a named error return value.
-			// Wrap the rhs in-place.
-			t.wrapExpr(1, assign.Rhs[i])


This line used to panic for an expression like _, err = fmt.Println("Hello, World!") in a defer block. Fixing this properly ended up being a bit of work because it doesn't seem like there's existing tuple-awareness already there.

DavidArchibald · 2024-04-09T01:17:40Z

cmd/errtrace/testdata/golden/single_rhs.go

+func multipleValueErrAssignment() (err error) {
+	defer func() {


There are two tests I didn't add:

Outside of the defer block:

_, err = fmt.Println("Hello, World!")

This one caused the import to be formatted differently in idempotent versus package, no idea why. If I had to guess outside the defer block there's some code that triggers gofmt.

This test would be inside the defer block:

_, err, _ := errtrace.Wrap2(fmt.Println("foo bar")), fmt.Errorf("foo"), fmt.Errorf("bar")

This is invalid syntax but the main problem was that in the package test mode it understands that Wrap2 returns two values but in the idempotent test mode it can't inspect the return type of Wrap2 (or any foreign/unrelated item) and so I had the code assume Wrap2 is valid and returns 1 value and so it tries to wrap fmt.Errorf("foo").

DavidArchibald · 2024-04-09T01:18:52Z

cmd/errtrace/main.go

+	}
+
+	// Ignore any error while parsing, attempt to gracefully handle it anyways.
+	pkg, _ := config.Check(filename, fset, []*ast.File{f}, nil)


As a note, the type checking will be improved if it parses the whole directory. An example would be calling foo() where foo is a function that returns a tuple but is declared in a sibling file. Doing that might be annoying though and I wrote a few assumptions to make it work on valid programs cleanly.

I didn't tackle this because the scope of this PR already grew a bit larger than I'd expected it to.

@DavidArchibald

In #96, @DavidArchibald reported (and provided a fix for) a bug where the following would cause a panic: ```go func f() (err error) { defer func() { _, err = fmt.Println("Hello, World!") }() return nil } ``` The panic was: ``` panic: runtime error: index out of range [1] with length 1 [recovered] panic: runtime error: index out of range [1] with length 1 goroutine 180 [running]: testing.tRunner.func1.2({0x1002cc500, 0x14000014a08}) [..]/go/1.22.2/libexec/src/testing/testing.go:1631 +0x1c4 testing.tRunner.func1() [..]/go/1.22.2/libexec/src/testing/testing.go:1634 +0x33c panic({0x1002cc500?, 0x14000014a08?}) [..]/go/1.22.2/libexec/src/runtime/panic.go:770 +0x124 braces.dev/errtrace/cmd/errtrace.(*walker).deferStmt.func1({0x1002df7a0?, 0x1400028e080}) [..]/braces.dev/errtrace/cmd/errtrace/main.go:925 +0x13c ``` And the panic was caused by the following code: ```go t.wrapExpr(1, assign.Rhs[i]) ``` The reason for the panic is that the LHS and RHS are imbalanced. The LHS contains two identifiers, but RHS contains only one expression. So `assign.Rhs[i]` panics when `i` is 1. The fix provided in #96 was more complex than is necessary for this case. This commit proposes a simpler fix: We already handle this kind of multi-return function when it's part of a `return` statement by using `WrapN` functions. ``` return fmt.Println("Hello, World!") // becomes return errtrace.Wrap2(fmt.Println("Hello, World!")) ``` We can use the same logic to handle the case where the multi-return function is part of an assignment in the defer statement. As with handling of return statements, this only supports cases where the error is the last return value. The following will be skipped: ``` _, err, _ = fmt.Println("Hello, World!") err, _ = fmt.Println("Hello, World!") ``` Test case borrowed from #96 and modified to represent the new behavior.

abhinav · 2024-04-15T04:46:34Z

Hey @DavidArchibald, thanks for the PR and the repro of the failure.

Really appreciate porting to go/types. I think that would've been good as its own independent change. I saw that go/ast.Object was deprecated in Go 1.22, so that was definitely on the "oh, we'll have to switch to full-fledged go/types at some point" moment.

That said, it doesn't look it's quite necessary in this PR.
So here's the panic that happens with the provided test case:

panic: runtime error: index out of range [1] with length 1 [recovered]
        panic: runtime error: index out of range [1] with length 1

goroutine 180 [running]:
testing.tRunner.func1.2({0x1002cc500, 0x14000014a08})
        [..]/go/1.22.2/libexec/src/testing/testing.go:1631 +0x1c4
testing.tRunner.func1()
        [..]/go/1.22.2/libexec/src/testing/testing.go:1634 +0x33c
panic({0x1002cc500?, 0x14000014a08?})
        [..]/go/1.22.2/libexec/src/runtime/panic.go:770 +0x124
braces.dev/errtrace/cmd/errtrace.(*walker).deferStmt.func1({0x1002df7a0?, 0x1400028e080})
        [..]/braces.dev/errtrace/cmd/errtrace/main.go:925 +0x13c
go/ast.inspector.Visit(0x14000369660, {0x1002df7a0?, 0x1400028e080?})
        [..]/go/1.22.2/libexec/src/go/ast/walk.go:386 +0x38

And you're right, this is the panicking line:

t.wrapExpr(1, assign.Rhs[i])

Fixing that doesn't require reworking the logic.
There are two possibilities with the assignment:

The assignment is either one LHS per RHS:

x, y, z := f1(), f2(), f3()

Or it's one RHS for the entire LHS:

x, y, z := f()

For the former, we can just wrap the one expression that's assigning to the err.

x, y, z := f1(), errtrace.Wrap(f2()), f3()

For the latter, we can re-use the WrapN mechanism, provided that the error is the last return value—this is something we already assume in a lot of the expression wrapping code.

x, y, z := Wrap3(f())

We may want to provide a non-WrapN thing in the future to handle the other cases, but this doesn't require reworking how the entire function works.

I've put up an alternative fix in #100. Please let me know whether that fixes your issue as well.

RE: errorObjs vs errorNames:
The reason we were using *ast.Object there was because it guarantees that we're referring to exactly that named return and not variables that shadow it. That we're not testing for it was an oversight. I've added a test case for this in #101.
A go/types based version of the same logic will probably need to track types.Objects similarly so that they refer to the same value, and not a shadowed one with the same name.

@DavidArchibald

In #96, @DavidArchibald reported (and provided a fix for) a bug where the following would cause a panic: ```go func f() (err error) { defer func() { _, err = fmt.Println("Hello, World!") }() return nil } ``` The panic was: ``` panic: runtime error: index out of range [1] with length 1 [recovered] panic: runtime error: index out of range [1] with length 1 goroutine 180 [running]: testing.tRunner.func1.2({0x1002cc500, 0x14000014a08}) [..]/go/1.22.2/libexec/src/testing/testing.go:1631 +0x1c4 testing.tRunner.func1() [..]/go/1.22.2/libexec/src/testing/testing.go:1634 +0x33c panic({0x1002cc500?, 0x14000014a08?}) [..]/go/1.22.2/libexec/src/runtime/panic.go:770 +0x124 braces.dev/errtrace/cmd/errtrace.(*walker).deferStmt.func1({0x1002df7a0?, 0x1400028e080}) [..]/braces.dev/errtrace/cmd/errtrace/main.go:925 +0x13c ``` And the panic was caused by the following code: ```go t.wrapExpr(1, assign.Rhs[i]) ``` The reason for the panic is that the LHS and RHS are imbalanced. The LHS contains two identifiers, but RHS contains only one expression. So `assign.Rhs[i]` panics when `i` is 1. The fix provided in #96 was more complex than is necessary for this case. This commit proposes a simpler fix: We already handle this kind of multi-return function when it's part of a `return` statement by using `WrapN` functions. ``` return fmt.Println("Hello, World!") // becomes return errtrace.Wrap2(fmt.Println("Hello, World!")) ``` We can use the same logic to handle the case where the multi-return function is part of an assignment in the defer statement. As with handling of return statements, this only supports cases where the error is the last return value. The following will be skipped: ``` _, err, _ = fmt.Println("Hello, World!") err, _ = fmt.Println("Hello, World!") ``` Test case borrowed from #96 and modified to represent the new behavior.

abhinav · 2024-05-05T18:08:21Z

Closing in favor of #100

DavidArchibald commented Apr 9, 2024

View reviewed changes

DavidArchibald force-pushed the main branch from 38a1d2a to ab56d13 Compare April 9, 2024 01:22

Fix bug with a tuple rhs

963f302

DavidArchibald force-pushed the main branch from ab56d13 to 963f302 Compare April 10, 2024 14:17

abhinav mentioned this pull request Apr 15, 2024

fix(cmd): Handle imbalanced LHS and RHS in defer assignment #100

Merged

abhinav closed this May 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug with a tuple rhs #96

Fix bug with a tuple rhs #96

DavidArchibald commented Apr 9, 2024 •

edited

Loading

DavidArchibald Apr 9, 2024

DavidArchibald Apr 9, 2024

DavidArchibald Apr 9, 2024

DavidArchibald Apr 9, 2024

DavidArchibald Apr 9, 2024 •

edited

Loading

DavidArchibald Apr 9, 2024 •

edited

Loading

abhinav commented Apr 15, 2024 •

edited

Loading

abhinav commented May 5, 2024

		func multipleValueErrAssignment() (err error) {
		defer func() {

Fix bug with a tuple rhs #96

Fix bug with a tuple rhs #96

Conversation

DavidArchibald commented Apr 9, 2024 • edited Loading

DavidArchibald Apr 9, 2024

Choose a reason for hiding this comment

DavidArchibald Apr 9, 2024

Choose a reason for hiding this comment

DavidArchibald Apr 9, 2024

Choose a reason for hiding this comment

DavidArchibald Apr 9, 2024

Choose a reason for hiding this comment

DavidArchibald Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

DavidArchibald Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

abhinav commented Apr 15, 2024 • edited Loading

abhinav commented May 5, 2024

DavidArchibald commented Apr 9, 2024 •

edited

Loading

DavidArchibald Apr 9, 2024 •

edited

Loading

DavidArchibald Apr 9, 2024 •

edited

Loading

abhinav commented Apr 15, 2024 •

edited

Loading