Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: eliminate skip argument to Caller #30809

Closed
josharian opened this issue Mar 13, 2019 · 8 comments

Comments

Projects
None yet
6 participants
@josharian
Copy link
Contributor

commented Mar 13, 2019

This discussion has been happening in CLs and the umbrella issue. Moving it to its own issue for focused discussion.

APIs that ask the caller to pass a number of frames to skip are fragile and hard to use. We should eliminate the skip argument and find another way to allow people to get the expressivity they need (if in fact they need it).

cc @jba @neild @mpvl @davecheney

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I agree that skip counts are fragile and hard to use, but before I can support this proposal, I'd love to see a concrete proposal for how they might be eliminated. In my experience, it's pretty common to define a helper function that wraps fmt.Errorf, making the information on the caller of fmt.Errorf mostly noise, but how could the runtime "know" that the helper function is just that, and not important business logic that justifies that caller info?

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

I personally like the testing.Helper approach. That works because we have a testing.T to hang that information off of.

I don't like this idea a whole lot, but we could have a global in errors that works like testing.T. Then you call errors.Helper() in your error helper function. Probably protected by a (now even cheaper) sync.Once.

I'd love to hear other concrete proposals.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

The nuclear option is to just get rid of the stack information, at least initially. It is (imo!) the least interesting/useful part of the proposal. It's also the part with the thorniest API/implementation issues (cf. DeepEquals).

@bcmills

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

The approach taken by testing.Helper is, I believe, extremely hostile to inlining. Ideally, wrappers around errors.New should be small enough to inline, and the compiler should be made smart enough to elide the call to runtime.Callers entirely.

Perhaps some sort of //go: directive to the compiler to tell it to skip the function's frame would be preferable?

@bcmills

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

(I should add: historically this is one of the key differences between macros and functions. Essentially, you're asking for the ability to define a function to have the behavior of a hygienic macro.)

@rogpeppe

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

Perhaps some sort of //go: directive to the compiler to tell it to skip the function's frame would be preferable?

I suppose the question then is: would it be acceptable not to show that frame in a panic traceback stack too, should the error-creating function were to crash?

(for the record, in the context of tests, T.Helper doesn't work well for me in general because you don't get enough info when you have nested helpers; but I don't think that criticism applies here because you really do only want one source location per level in the error chain, because creating an error is almost never itself an error-prone operation).

@neild

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

I don't see how errors.Helper can be made efficient.

I agree that frame skip counts are not the kindest of APIs, but we need a concrete, viable counterproposal if we are to drop them. Having no control over skips seems far too limiting.

One possibility might be to have just two constructor functions, corresponding to Caller(0) and Caller(1), since "this line right here" and "the function that called me" are the most common frames of interest.

@andybons

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Given that there's no Caller function in the package anymore, closing. If it gets re-introduced feel free to reopen.

@andybons andybons closed this May 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.