Comments (20)
Sorry, I'm not going to do this. I agree that in a green field situation Wrap(nil) could panic, but in your example
err := stuff()
if err != nil {
return err
}
result := moreStuff()
if len(result.Failures) > 0 {
// ... do stuff with failures ...
return err // not wrapped errors.Wrap(err, "Oops")
}
You still have the same scoping mistake. This is unrelated to the errors package.
from errors.
@davecheney just meet this issue, it's very tricky to find out what goes wrong when error gone silently when you accidentally warp an nil error
maybe you can bump a version and denote the incompatible changes?
if panic is a breaking change, at least warp should always return a error
from errors.
@Ehekatl the issue here is that returning a nil err instead of return Wrap(err, …)
would present the same issues of difficulty in tracking down such a bug.
I once found a shadowed err
bug in some RPC retry code, and it took a lot of work to figure out why the code was returning a non-error when we expected an error condition.
i.e. having Wrap
panic on a nil error will not actually solve the root cause of returning a nil error when an error is expected.
Also, consider the following code:
n, err := w.Write(data)
return n, errors.Wrap(err, "error while writing data")
which is comparable to:
return w.Write(data)
vs.
n, err := w.Write(data)
if err != nil {
return n, errors.Wrap(err, "error while writing data")
}
return n, nil
from errors.
I mean Wrap(err) should either panic or return a non-nil error, an error library should always report error, not eat it.
But the error library is not eating any errors. It is strictly keeping the case that a non-error will remain a non-error. Wrapping a non-error, should remain a non-error, and the only way to keep this condition within golang, is: if nil is passed in, nil must come out.
…the point of panic here is to make a equivalent effect of accessing a nil reference.
But we are not accessing a nil reference here. A nil error is a defined and expected state, and using and passing it around as a value should therefore not cause a panic.
from errors.
I see the justification, but it is too late to make this backwards incompatible change. Sorry.
from errors.
That is what semantic versioning is for. Just bump the major version, problem solved.
On Oct 28, 2016, at 22:07, Dave Cheney [email protected] wrote:
I see the justification, but it is too late to make this backwards incompatible change. Sorry.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
from errors.
But that scoping mistake is much easier to notice, and is semantically not a problem, because err
is not being passed on to anything except the caller.
If Wrap()
doesn't receive an error, it's accidentally throwing away semantic information in a manner which can be quite difficult to notice. The contract of Wrap()
should be to wrap an error, and fail on input that isn't an error. This follows the same good design principles behind fmt.Sprintf()
and friends, for example: If Sprintf()
receives bad input (such as superfluous arguments), it doesn't ignore it, but signals the problem in its result value. That's just good design.
The likelihood of the scoping issue happening increases due to how Go encourages reusing error variables — protecting it with if x, err := ...
often results in quite awkward, overly branching code, and in practice, relatively few err
vars can be limited in scope that way. Conversely, I very much dobut that there's much code in the wild that relies on the fact that Wrap(nil)
returns nil.
I don't know what greenfield has to do with anything. Semver makes the problem of introducing backwards-incompatible changes a moot point.
from errors.
I'm sorry, my decision is final.
This package is released under a permissive open source licence, I suggest you fork it and modify it for your requirements.
Thanks
Dave
On 29 Oct. 2016, at 13:35, Alexander Staubo [email protected] wrote:
But that scoping mistake is much easier to notice, and is semantically not a problem, because err is not being passed on to anything except the caller.
If Wrap() doesn't receive an error, it's accidentally throwing away semantic information in a manner which can be quite difficult to notice. The contract of Wrap() should be to wrap an error, and fail on input that isn't an error. This follows the same good design principles behind fmt.Sprintf() and friends, for example: If Sprintf() receives bad input (such as superfluous arguments), it doesn't ignore it, but signals the problem in its result value. That's just good design.
The likelihood of the scoping issue happening increases due to how Go encourages reusing error variables — protecting it with if x, err := ... often results in quite awkward, overly branching code, and in practice, relatively few err vars can be limited in scope that way. Conversely, I very much dobut that there's much code in the wild that relies on the fact that Wrap(nil) returns nil.
I don't know what greenfield has to do with anything. Semver makes the problem of introducing backwards-incompatible changes a moot point.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.
from errors.
I just ran into this exact bug and it swallowed a good hour. I think it would have been better to disallow wrap(nil...)
as suggested by @atombender.
from errors.
from errors.
Understood. I wanted to just contribute some experience. I remember loving that Wrap nil returned nil, but now it reaffirms my obsession with magicless code.
But errors
isn't v1 yet is it?
from errors.
A) It would be bad form for Wrap to panic. https://github.com/golang/go/wiki/CodeReviewComments#dont-panic
B) As for Wrap returning any value other than nil, that is also not a good idea, because then errors.Wrap(nil) != nil
will be true.
This would require anyone calling into your library to use pkg/errors.Cause(err) != nil
which is a poor choice that forces different error handling code on anyone using your code.
Yes, there is a loss of semantic information, but it is simply not possible to attach any semantic information to a nil error and still ensure that standard Go error checking would continue to work.
from errors.
from errors.
for someone else run into this, I made a simple fork version which add MaybeWarp and Wrap on nil will panic: https://github.com/Ehekatl/errors
from errors.
I consider @puellanivis 's opinions more reasonable. Please allow me to add more reasons against doing so.
- panicing on error handling library is against the whole reason of having an error handling library.
The caller then have to do
defer func() {
recover()...
}()
which makes the code less readable.
Panic recovery should be used in specific cases. panic
on Wrap(nil)
makes recover
a general requirement.
- I would consider
n, err := w.Write(data)
return n, errors.Wrap(err, "error while writing data")
without panicing a nice feature to have. err == nil
should always be explicitly checked.
If you don't, the semantic, for me at least, is always "I don't care if it's nil."
from errors.
Sorry I didn't quite get your idea, I mean Wrap(err) should either panic or return a non-nil error, an error library should always report error, not eat it.
I think that certainly help with tracking down bugs.
your comments are reasonable, of course err == nil
should always be explicitly checked, but the problem is how could I possibly know if I forget doing so?
recover should not be a general requirement in such case, because you should never Wrap on anything nil, it should crash. I do agree it's better not to panic in error handling, but this situation is different. Wrap never expecting nil
, the point of panic here is to make a equivalent effect of accessing a nil reference.
from errors.
@puellanivis well, let me reformat my statement, I think Wrap shouldn't accept nil as valid input, Wrap(nil) doesn't make any sense to me. I can't imagine anyone would rely on errors.Wrap(nil, msg) == nil
is true in real world.
from errors.
I understand the argument, but it does make sense to do something like, return errors.Wrap(json.Unmarshal(data, &v), "unmarshaling response failed")
The idea was that one could wrap an arbitrary error without needing to additionally care if it is nil or not.
The pkg/errors
package was never designed as a stop-gap to help people avoid returning a nil error, where they are actually expected/thinking that they should be returning a non-nil error.
This is a fundamental issue/feature/bug of the language itself, and is entirely outside of the scope or ability of pkg/errors
to solve. If we “fix“ this behavior, maybe we catch one or two accidental oversights, but we also risk causing panics in code that is otherwise correctly handling errors.
from errors.
You are right, the problem isn't about the behavior.
It's more of an literal problem, MustX/MaybeY make a perfect sense to avoid misunderstanding.
from errors.
@Ehekatl The best solution for you at the moment might be writing your own / fork this pkg.
As long as you implement .Cause(err) -> err
interface, it should work with this pkg seamlessly
from errors.
Related Issues (20)
- Proposal: add errors.Is and errors.As
- RFC: Allow custom StackTraces
- func Is() does not works with errors.Wrap or errors.Wrapf HOT 3
- Cause() behavior has changed HOT 11
- respect -trimpath build flag HOT 3
- Unwrap doesn't return the base error HOT 7
- Questions about extending annotations HOT 5
- undefined errors.Wrap HOT 2
- How to change format return wrapped error message HOT 2
- Stacktrace don't print all stack HOT 6
- errors.Wrap() created bugs in our codebase. HOT 3
- StackTrace.Format with "%+v" includes a leading newline HOT 1
- A new error caused by another error HOT 7
- Demo
- Change Wrap() to return an error even if we provide nil HOT 3
- return *stack instead of stack HOT 2
- Wrap() duplicates call stack HOT 6
- Elegant way to check error is wrapped HOT 4
- `Errorf` should be able to wrap errors HOT 4
- Package errors looking for new maintainers HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from errors.