Q: Does this behavior actually makes sense? Based on my understanding of the expected behavior of HasType this seems wrong.

Don't want to ignore the comment so just chiming in to say I am unsure 😅 IMO I don't really understand the use case for HasType; if there isn't one, I'm inclined to suggest we do not export it from lib/errors and discourage its use, to keep things simple

To me, it seems like the implementation of HasType is wrong. It ought to instead be something along the lines of:

  typ := reflect.TypeOf(t)
  zero := reflect.Zero(typ)
  return errors.As(err, &zero)

Or perhaps, the bug is with our multiError type, not sure.

I think I tracked down the cause of this.

For a multi-error, we mark the last element as the Cause or result of Unwrap. code

func (e *multiError) Cause() error  { return e.errs[len(e.errs)-1] }
func (e *multiError) Unwrap() error { return e.errs[len(e.errs)-1] }

HasType calls If which calls UnwrapOnce inside its predicate function while doing a depth-first traversal, which has this:

func UnwrapOnce(err error) (cause error) {
	switch e := err.(type) {
	case interface{ Cause() error }:
		return e.Cause()
	case interface{ Unwrap() error }:
		return e.Unwrap()
	}
	return nil
}

Notice this comment:

// UnwrapOnce treats multi-errors (those implementing the
// `Unwrap() []error` interface as leaf-nodes since they cannot
// reasonably be iterated through to a single cause

This means that HasType will only compare against the last element in a multiError. 🤦🏽

In contrast, Is handles multi-errors explicitly, and As also handles multi-errors explicitly.

With a slightly different API, here's an alternate implementation of HasType that doesn't suffer from the same problem. It seems like we should be able to tweak this using some comby-fu.

func HasType[T error](err error) bool {
	var zero T
	return As(err, &zero)
}

IMO I don't really understand the use case for HasType

The use case, as I understand it, is "is this error type present in the tree" without caring about the actual value.

Branch fixing the implementation of HasType. https://github.com/sourcegraph/sourcegraph/tree/vg/replace-HasType

Will create a PR with that after merging this.

Is it ignored because you want to be selective toward what gets committed here, amongst what it finds, or is that because we'll always intend this folder content to be ignored?

I've added a more detailed comment here, does that answer your question?

# Generated by rapid after finding counter-examples.
#
# When re-running tests locally during development,
# this enables faster failure finding.
# https://github.com/flyingmutant/rapid/commit/4db076b4aa4d41036416da11abd5bbfc228a324e
#
# During development, if a check fails, it makes sense
# to disable or refine the check, and add specific,
# minimal tests using the counter-examples found by rapid.

See https://github.com/sourcegraph/sourcegraph/pull/62992/files#r1622408899

If this is always going to be filled with examples generated locally, then it'll always end up empty in CI.

Additionally, if the folder doesn't exist, the glob eval will break in CI. Best to add a .gitkeep in there if you truly want to keep it.

Hmm, not sure why this got added, I have no memory of modifying any BUILD files myself. I'll remove this and see if something breaks. AIUI, the only purpose is to record failures, so next time it can try them quickly.

flyingmutant/rapid@4db076b

However, in CI, the test won't be re-run, so it doesn't make sense to save the file.

Removed in https://github.com/sourcegraph/sourcegraph/pull/62992/commits/c1d8bcff5b55a639bfe501e217ee721b22fefa26