get first stack trace info.

[wangsai-silence]

For this issue #173

[puellanivis]

Not condoning the adoption of this PR, just suggesting code design changes to match other code in the repo, and pointing out issues with how the test is written that will cause it to erroneously pass.

[puellanivis]

Suggested change

	return
	continue

Otherwise, we break out of the test after the first test, because definitively tt.want == nil.

Also, as written, by contract of the code design, we should expect all the other table tests to fail after this change.

[wangsai-silence]

You are right. I will rewrite it.

[wangsai-silence]

Thank you for your comments

[puellanivis]

More detailed review now that the essential functionality is good.

[puellanivis]

Using the phrase “the first stack trace” is ambiguous, depending upon which direction you are orienting the Cause chain. If you look at it going down the Cause chain, the most recent StackTrace is the first stack trace encountered, in which case the stack trace being returned by this function would be the last stack trace.

Meanwhile, from the context that you appear to be intending, you are saying that the bottom-most StackTrace, which is the earliest one attached, is the first, and therefore the last one would be the most-recently attached.

And now even I have reversed my original top/bottom distinction from the last code review, by reorienting the Cause chain. And thus, the difficulty in writing good documentation that explains accurately the contract of behavior.

Please attempt to rework this documentation so that the functionality is unambiguous. Such as, “… returns the earliest/deepest StackTrace attached to any of the errors in the chain of Causes.” Potentially requiring an explanation of what a “Cause chain” is or even how it should be orient for the purpose of understanding the behavior of this function.

Basically, I should be able to read this documentation only, and know the essential functional/behavioral elements of this black box, without having to read the code in order to obtain definitions or context.

Also, this documentation does not indicate what it should return if it is not possible to find a StackTrace. This should be documented so that the user is fully aware of the contract. c.f. the documentation in Cause: If the error does not implement Cause, the original error will be returned. If the error is nil, nil will be returned without further investigation.

[wangsai-silence]

Thank you so much for your patience. Your advice has been invaluable, and I have followed the suggest expect the last one. I don't know how to deal with this documentation. And I just wrote your words on it. So could you help to make it more clear?

[puellanivis]

The docstrings should not have a space between them an the function. I don’t know for sure this would break godoc, but convention suggest we strictly enforce it.

[wangsai-silence]

OK. I'd remove it. Thank you.

[flimzy]

I think you mean "the stacktrace from the original error will be returned"?

[hanzei]

@wangsai-silence Could you please take a look at the comments above?

[wangsai-silence]

@wangsai-silence Could you please take a look at the comments above?

OK. I've done. Thank you for remind me about that.

[mightyguava]

Oh I was just looking for something like this. @wangsai-silence @puellanivis what is left to be done for this PR?

[wangsai-silence]

Oh I was just looking for something like this. @wangsai-silence @puellanivis what is left to be done for this PR?

Nobody answered. I have used it in my project.

[puellanivis]

🤷 I don’t have collaborator rights to the repo, so I cannot approve and merge. Also, I was going to leaving the approval/denial of the mere design up to other parties anyways.

The branch is however now out-of-date with the base branch, and likely would end up with a merge conflict or something.

Fortunately, the code works both inside of and outside of this package. So there is not much stopping anyone from replicating it. But, with proper pkg/error use sanitation, you can avoid the need to use it drastically.

[flimzy]

FWIW, I proposed something like this back in 2016. While it wasn't outright rejected, I closed the issue because I agreed with Dave Cheney's cautious approach.

Since then, I've written this functionality into practically every Go application I've written, and I don't mind doing that.

In fact, I expect I would continue to do so, even if this PR is merged, because, as Dave predicted, I seem to want subtly different things in each app.

And now with the advent of Go 1.13's new error handling, it's even less likely I would write an application to depend on this feature. I'd tend to prefer a version based on errors.As.

[mightyguava]

@flimzy ah, asking each package to implement their own version makes sense with that link. I don't understand what errors.As has to do with this though. I want to print a stacktrace for debugging.

[flimzy]

@flimzy ah, asking each package to implement their own version makes sense with that link. I don't understand what errors.As has to do with this though. I want to print a stacktrace for debugging.

Two things:

1. I want a version that honors Go 1.13's wrapper interface. errors.As is the easiest way to address that.
2. errors.As makes it trivial to implement this functionality, making the need for supporting it in a library like this less urgent, IMO. Example (untested "whiteboard" code):

stackErr := new(interface{ StackTrace() errors.StackTrace })
for errors.As(err, stackErr) {
    err = stackErr
}
if stackErr != nil {
    stack = stackErr.StackTrace()
}

[mightyguava]

errors.As doesn't unwrap unless necessary, so I think you have an infinite loop there.

[flimzy]

errors.As doesn't unwrap unless necessary, so I think you have an infinite loop there.

Possible. As I said, the code is back-of-napkin quality, untested. I don't think it affects the substance of what I was saying.

[puellanivis]

You are correct, errors.As does not unwrap if the given error is already assignable to the second argument. That’s easy enough to fix with an errors.Unwrap call.

But I would additionally recommend against using new() for an interface. Because an interface is already reference semantics, it is almost universally an error to make a direct datatype that is a pointer to an interface. The only time a pointer to interface is not an error, is when it is a derived type, for instance when you have a subfunction that would like to assign into an interface value, rather than just use it, where it should be derefed at the subfunction call. So, these changes get us:

func Stack(err error) StackTrace {
	var stackErr interface{ StackTrace() errors.StackTrace }
	for errors.As(err, &stackErr) {
		err = errors.Unwrap(stackErr) // err will be assignable, and infinite loop otherwise.
	}
	if stackErr != nil {
		return stackErr.StackTrace()
	}
	return nil
}

[flimzy]

But I would additionally recommend against using new() for an interface. ... snip ...

All good advice. Please don't get hung up on the minutiae of a throw-away coding example. I should have just left the trivial example as a mental exercise to avoid derailing the conversation. 🤦

[puellanivis]

I don’t think we derailed things. We ended up with a corrected version, that people can use. It’s the good-ol’ “say a wrong answer, and you will immediately get 50 other people to do the work of finding the correct answer for you.”