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

cmd: Add an errtrace command #11

Merged
merged 22 commits into from
Nov 19, 2023
Merged

cmd: Add an errtrace command #11

merged 22 commits into from
Nov 19, 2023

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Nov 19, 2023

Adds a new errtrace binary that instruments existing Go code
with errtrace.Wrap calls.

This operates by searching for functions that return errors,
and inside those functions, wrapping all returns with errtrace.Wrap.

To allow for future usage with -toolexec or similar,
the code edits all ensure that line numbers do not change.
We may want to reconsider this behavior in the future.

This has some amount of work pending, but the PoC works.

  • More testing with more corner cases
  • Detect overlapping edits: this should be considered a bug
  • Handle expected overlapping edits, e.g. return func() { return err }().
  • Handling of statements like return foo()
    where foo() returns x, err -- this currently panics logs and skips.
    This needs helper Wrap{N} functions
    parameterized over the non-error return types,
    and that still won't handle multiple error returns
    like the rest of the program. That should be fine.
  • Handling of package naming conflict:
    The code has room for using a name other than "errtrace"
    for the package, but the conflict isn't currently detected.
    This probably needs making two passes over the file
    to find a unique identifier.

Adds a new errtrace binary that instruments existing Go code
with errtrace.Wrap calls.

This operates by searching for functions that return errors,
and inside those functions, wrapping all returns with `errtrace.Wrap`.

To allow for future usage with -toolexec or similar,
the code edits all ensure that line numbers do not change.
We may want to reconsider this behavior in the future.

Pending:

- More testing with more corner cases
- Detect overlapping edits: this should be considered a bug
- Handling of statements like `return foo()`
  where `foo()` returns `x, err` -- this will currently panic.
  This needs helper `Wrap{N}` functions
  parameterized over the non-error return types,
  and that still won't handle multiple error returns
  like the rest of the program. That should be fine.
- Handling of package naming conflict:
  The code has room for using a name other than "errtrace"
  for the package, but the conflict isn't currently detected.
  This probably needs making two passes over the file
  to find a unique identifier.
This will address cases like the following,
where the return values come from another functionc all.

    return foo()
    // where func foo() (int, error)

While we're figuring out an API for this,
just skip over them with a loud warning.
Thw warning should be dropped before we give this to others.
The editing scheme might not be our best option here.
This model cannot handle

    return func() error {
        return errors.New("foo")
    }()

Because the outer closure overlaps with the inner.

I think the editing scheme needs to separate open and close of wrapping
so that "add errtrace.Wrap(" and "add )" are separate edits,
that do not overlap with the inside of the return expression.
Drop use of slices and cmp to remain compatible with Go 1.20 for now.
Test makes use of the `max` built-in.
Separates wrapEdit into a separate "open" and "close" edits.
This is necessary to handle cases where an expression
inside a wrapped expression also needs to be wrapped.
Capture names of error return values once when entering the function
and then re-use that for all wrap calls.
The edits are all insertions at specific positions.
There's no replacement of text otherwise -- at least at this time.

Simplify the interface:

- rename it to insert
- rename Start to Pos
- drop the End method
- rename implementations to match
If the program does not import 'errtrace',
and the identifier is used anywhere in the code,
use a named import for errtrace with a different name.
If there's an existing import group,
add the import to the last group
rather than creating a new import statement.

This will play nicer with autoformatting.
Errors that implement Unwrap method
should be able to return their inner method
without having it re-wrapped.
Instead of recording full AST nodes, just record positions.
This should make unit testing easier.
If we encounter a deferred function literal inside a function
with a named error,
also wrap assignments to the named error inside the deferred function.
cmd/errtrace/main.go Outdated Show resolved Hide resolved
cmd/errtrace/main.go Outdated Show resolved Hide resolved
cmd/errtrace/main.go Outdated Show resolved Hide resolved
}

// Assigning to a named error return value.
// Wrap the rhs in-place.
Copy link
Contributor

Choose a reason for hiding this comment

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

if the deferred function has multiple assignments to the same error value, i think this will wrap multiple times -- should we only wrap the last assignment?

don't have to address this immediately - maybe just a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't we want all assignments to be affected, though?

defer func() {
  if cond {
    err = fmt.Errorf("cond: %w", err)
  } else {
    err = fmt.Errorf("not cond: %w", err)
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, better to wrap multiple times than miss one of the cases. Don't think we can do more without more advanced control flow analysis.

cmd/errtrace/main_test.go Show resolved Hide resolved
cmd/errtrace/testdata/golden/named_returns.go Show resolved Hide resolved
Rename the variable for clarity.
Ensure that we don't rewrap variables that have already been wrapped.
The re-assignment detection is pretty simplistic;
to get smarter about corner cases, we'll need control flow graph analysis
which seems unnecessary at this time.
Buffer the entire file in memory
and write to it only after the entire file has been processed.
@abhinav abhinav merged commit 2583784 into main Nov 19, 2023
6 checks passed
@abhinav abhinav deleted the abg/cmd branch November 19, 2023 23:46
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.

2 participants