Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Apr 15, 2024

In #96, @DavidArchibald reported (and provided a fix for)
a bug where the following would cause a panic:

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:

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.

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 abhinav mentioned this pull request Apr 15, 2024
Comment on lines 925 to 927
// (1) x, y, err := f1(), f2(), f3()
// (2) x, y, err := f() // returns multiple values
// (3) x, err, z := f() // returns multiple values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment nit: as discussed offline, these would probably be = rather than := since that would create a new variable, so would be skipped by the above t.errorObjs check

cmd/errtrace/main.go Outdated Show resolved Hide resolved
@abhinav abhinav enabled auto-merge (squash) April 15, 2024 05:04
@abhinav abhinav merged commit 88e6abb into main Apr 15, 2024
13 checks passed
@abhinav abhinav deleted the defer-assign-multi-return branch April 15, 2024 05:07
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.

None yet

2 participants