wrapcheck icon indicating copy to clipboard operation
wrapcheck copied to clipboard

Ignore errors in closures passed to specific functions

Open SOF3 opened this issue 1 year ago • 4 comments

I have a utility function like this:

type Slice[T any] []T

func (slice Slice[T]) TryForEach(message string, fn func(T) error) error {
    for _, item := range slice {
        if err := fn(item); err != nil {
            return errors.Wrap(err, message)
        }
    }
    return nil
}

However, when I call this function:

slice.TryForEach("message", func(value T) error {
    return otherpkg.DoSomething(value)
}

I get an error for not wrapping DoSomething.

TryForEach actually already wraps the error. It would be useful to have an ignore directive like this:

wrapcheck:
  ignoreClosureInArgList:
    - func: "TryForEach"
      param: "fn"

such that the lint is disabled for top-level return statements (and not including nested closures) in closures if the closure is directly passed as the fn parameter for a function matching TryForEach.

SOF3 avatar Jun 27 '24 03:06 SOF3

I'm happy to contribute this feature myself, would just like to hear some opinions on the design first.

SOF3 avatar Jun 27 '24 03:06 SOF3

Hmm, that is an interesting use case. Quite specific, it seems.

I'm not opposed to it, but I'm currently thinking of the value versus adding a line level ignore. Is this something that happens enough to you that it's worth the implementation effort for you?

tomarrell avatar Jun 27 '24 11:06 tomarrell

Well, I use the TryForEach function a lot in the implementation of a certain interface in my project, so I have to add //nolint:wrapcheck above the whole function for every implementation of this interface. I agree this is a bit niche; would be great if anyone could propose a more reusable ignore rule.

SOF3 avatar Jun 27 '24 13:06 SOF3

Fair enough, alright, if you want to open a PR with a proposed implementation I'd be happy to review it.

tomarrell avatar Jul 09 '24 13:07 tomarrell

I would also be interested in this! We use CommitWrite functions to simplify database transactions:

// CommitWrite opens a read-write database transaction.
func (db *Service) CommitWrite(ctx context.Context, fn func(*sql.Tx) error) error {
	tx, err := db.conn.BeginTx(ctx, nil)
	if err != nil {
		return fmt.Errorf("sqlite.CommitWrite: %w", err)
	}
	defer func() {
		if rollbackErr := tx.Rollback(); rollbackErr != nil && !errors.Is(rollbackErr, sql.ErrTxDone) {
			db.logger.Error().Err(err).Msg("Failed to rollback read-write transaction")
		}
	}()

	if err = fn(tx); err != nil {
		return fmt.Errorf("sqlite.CommitWrite: %w", err)
	}

	return tx.Commit()
}

Which is called like:

if err = s.db.CommitWrite(ctx, func(tx *sql.Tx) error {
	if err = s.db.CreateUser(ctx, tx, user); err != nil {
		return err
	}

	return nil
}); err != nil {
	return fmt.Errorf("funcName: %w", err)
}

It'd be great if we could specify a function name to ignore perhaps (not an ideal solution of course). E.g. ignore sqlite.Service.CommitWrite with sqlite being the originating package, then Service.CommitWrite being the function. Or even just the CommitWrite function itself.

I'd be happy to take a look at implementing this myself, but figured I'd mention my use-case here beforehand as I don't currently have the time to do so.

wolveix avatar Aug 19 '25 19:08 wolveix