I'm not sure if I fully understand this case. I guess that you're creating the error returned to the first k.Pos() called after k.Seek(0, io.SeekEnd), but this piece code is not exactly self-explanatory. It took me a while to figure out what do you want to achieve by the cryptic condition fr.pos != initialPosition .

And "not allowed to seek to the current pos" is not a very helpful message either (or let's call it a comment, because it's not printed anywhere). It made me think at first that instead of whence == io.SeekCurrent && fr.pos != initialPosition you must have meant whence == io.SeekCurrent && offset == 0 (so as not to allow seeking to the current fr.pos).

What about this:

	switch {
	case whence == io.SeekCurrent && fr.pos == -1:
		return 0, errors.New("artificial error when seeking with io.SeekCurrent after seeking to end")
	// ...
	default: // whence == io.SeekEnd
		fr.pos = -1
	}

It would help me if I saw the code for the first time.

I agree. Done.

The tool https://github.com/golangci/golangci-lint noticed this problem:

$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai
kaitai\stream.go:67:14: Error return value of `k.Seek` is not checked (errcheck)
        defer k.Seek(curPos, io.SeekStart)
                    ^

This might potentially lead to a bug that this call fails, but because its error return value is ignored, k.Size() will happily return err == nil (despite the fact that the stream pointer failed to return to the original position for some reason and we're clearly in a situation of failure).

I think the code should be like this:

-func (k *Stream) Size() (int64, error) {
+func (k *Stream) Size() (int64, err error) {

See https://yourbasic.org/golang/defer/#use-func-to-return-a-value and https://blog.learngoprogramming.com/5-gotchas-of-defer-in-go-golang-part-iii-36a1ab3d6ef1#afb5.

Could you please adjust the tests to check this case as well?

Perhaps we can make the failingReader more generic by restoring its normal behavior (i.e. io.SeekEnd behaving as normal, not setting pos to -1) and instead passing a param in it (e.g. numSeeksLimit) that limits the number of succeeding Seek() calls. Then we can just go iterate 0..(numSeeksThatSizeMethodDoes - 1) and check for each limit that the Size() function always returns an error and restores the stream position to its original state.

I've added two test cases:

1. fails to get the initial pos
2. the deferred seek fails

I don't quite get why this check function is declared for each case individually. All cases use failingReader:

which in turn always returns artificialError if failingCondition (also known as mustFail) is true:

So I'd rather replace stream_test.go:159-161 with the previous version:

Not all cases return artificialError: when the deferred seek fails it returns an artificialError wrapped into a fmt.wrapError.

A more explicit definition could be:

func TestErrorHandlingInStream_Size(t *testing.T) {
	isArtificialError := func(err error) bool {
		_, ok := err.(artificialError)
		return ok
	}

	isWrappedArtificialError := func(err error) bool {
		_, ok := errors.Unwrap(err).(artificialError)
		return ok
	}

	tests := map[string]struct {
		initialPos       int64
		failingCondition func(fr failingReader, offset int64, whence int) bool
		errorCheck       func(err error) bool
		wantFinalPos     int64
	}{
		"fails to get initial position": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekCurrent && offset == 0
			},
			errorCheck:   isArtificialError,
			wantFinalPos: 5,
		},
		"seek to the end fails": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekEnd
			},
			errorCheck:   isArtificialError,
			wantFinalPos: 5,
		},
		"deferred seek to the initial pos fails": {
			initialPos: 5,
			failingCondition: func(fr failingReader, offset int64, whence int) bool {
				return whence == io.SeekStart && fr.pos == -1
			},
			errorCheck:   isWrappedArtificialError,
			wantFinalPos: -1,
		},
	}

Anyway, you can go ahead, pick the version you like the most and modify the PR consequently.

Not all cases return artificialError: when the deferred seek fails it returns an artificialError wrapped into a fmt.wrapError.

Oh, thanks, I didn't notice that. I didn't see this little exclamation mark in !ok here:

The above "check" is totally unacceptable because it only checks that the artificialError is either wrapped somewhere inside or we're dealing with a completely different error that does not have anything in common with artificialError at all. It is also deceptive because it looks so similar to return ok, which would make more sense.

But it turns out that the linter knows about the problem that wrapped errors cannot be simply type asserted, and it suggested a solution:

$ ./golangci-lint-1.35.2-windows-amd64/golangci-lint run kaitai --no-config -E errorlint
...
kaitai\stream_test.go:121:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
                                _, ok := err.(artificialError)
                                         ^

So I believe that this silver bullet errors.As should work for everything:

+	var wantErr artificialError
-	_, isArtificialErr := err.(artificialError)
+	_, isArtificialErr := errors.As(err, &wantErr)
 	if !isArtificialErr {
-		t.Fatalf("Expected error of type %T, got one of type %T", artificialError{}, err)
+		t.Fatalf("Expected error of type %T, got one of type %T", wantErr, err)
 	}

See https://play.golang.org/p/1FN6BVmqRWV for a demo.