errorsbp: Fix a corner case in BatchSize

[fishy]

I missed a corner case in 1edc07c, that if a batched error was single wrapped again BatchSize would not recognize it as batched error. Fix the issue and updated the test case to cover it.

[kylelemons]

(above) If err implements is no longer quite true, since it works if any error in the tree implements Unwrap().

It's also probably worth noting that, with the support we're adding, BatchSize() != len(GetErrors()). Do we want that invariant to be true?

Does BatchSize need to be an xFS traversal using Unwrap() error / Unwrap() []error to actually count the number of errors? now that we're using errors.As, it seems like it might...

[fishy]

I'll update the doc.

As for BatchSize() != len(GetErrors()), yes I'm aware and that's an intentional decision from the last PR. The rational is that even though we try to flatten it while adding to the batch, we only want to flatten it when we can be sure that we won't lose important context when doing so, which is not a guarantee if we try to flatten everything that implements Unwrap() []error, here is an example:

func foo(...) error {
  ...
  return fmt.Errorf("some very important context info: %w, %w", err1, err2)
}

func bar(...) error {
  var batch errorsbp.Batch
  ...
  batch.Add(foo(...)) // if we try to flatten it here, we have no easy way to preserve the context info from foo
  ...
  return batch.Compile()
}

and also that's only a problem if the code somehow mixes errorsbp.Batch with errors.Join/fmt.Errorf batches. the hope is that

1. people should stop using errorsbp.Batch if they already upgraded to 1.20 and only use errors.Join/fmt.Errorf instead
2. we'll deprecate errorsbp.Batch as soon as we drop support for 1.19.

As to your last point, I don't think that's necessary. errors.As(err, &unwrapper) will do Unwrap() error for us until it finds the first Unwrap() []error, and those Unwrap() error layers it skipped will not really increase the number of wrapped errors in the tree.

[fishy]

do you want me to update the doc comment to also call out/explain the BatchSize() != len(GetErrors()) case?

[kylelemons]

yeah, I think it's worth documenting explicitly BatchSize() != len(GetErrors()) to reduce the chance that someone tries that on accident thinking it'll work.

[fishy]

done