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

Proposal: A built-in Go error check function, "try" #32437

Open
griesemer opened this issue Jun 4, 2019 · 400 comments

Comments

Projects
None yet
@griesemer
Copy link
Contributor

commented Jun 4, 2019

Proposal: A built-in Go error check function, try

Before commenting, please read the detailed design doc and see the discussion summary as of June 6, the summary as of June 10, and specifically the advice on staying focussed. Your question or suggestion may have already been answered or made. Thanks.

We propose a new built-in function called try, designed specifically to eliminate the boilerplate if statements typically associated with error handling in Go. No other language changes are suggested. We advocate using the existing defer statement and standard library functions to help with augmenting or wrapping of errors. This minimal approach addresses most common scenarios while adding very little complexity to the language. The try built-in is easy to explain, straightforward to implement, orthogonal to other language constructs, and fully backward-compatible. It also leaves open a path to extending the mechanism, should we wish to do so in the future.

The try built-in function takes n+1 arguments (where n may be zero) where the last argument must be of type error. It returns the first n arguments (if any) if the (final) error argument is nil, otherwise it returns from the enclosing function with that error. For instance, code such as

f, err := os.Open(filename)
if err != nil {
	return …, err  // zero values for other results, if any
}

can be simplified to

f := try(os.Open(filename))

try can only be used in a function which itself returns an error result, and that result must be the last result parameter of the enclosing function.

This proposal reduces the original draft design presented at last year's GopherCon to its essence. If error augmentation or wrapping is desired there are two approaches: Stick with the tried-and-true if statement, or, alternatively, “declare” an error handler with a defer statement:

defer func() {
	if err != nil {	// no error may have occurred - check for it
		err = …	// wrap/augment error
	}
}()

Here, err is the name of the error result of the enclosing function. In practice, suitable helper functions will reduce the declaration of an error handler to a one-liner. For instance

defer fmt.HandleErrorf(&err, "copy %s %s", src, dst)

(where fmt.HandleErrorf decorates *err) reads well and can be implemented without the need for new language features.

The main drawback of this approach is that the error result parameter needs to be named, possibly leading to less pretty APIs. Ultimately this is a matter of style, and we believe we will adapt to expecting the new style, much as we adapted to not having semicolons.

In summary, try may seem unusual at first, but it is simply syntactic sugar tailor-made for one specific task, error handling with less boilerplate, and to handle that task well enough. As such it fits nicely into the philosophy of Go. try is not designed to address all error handling situations; it is designed to handle the most common case well, to keep the design simple and clear.

Credits

This proposal is strongly influenced by the feedback we have received so far. Specifically, it borrows ideas from:

Detailed design doc

https://github.com/golang/proposal/blob/master/design/32437-try-builtin.md

@gopherbot gopherbot added this to the Proposal milestone Jun 4, 2019

@gopherbot gopherbot added the Proposal label Jun 4, 2019

@rasky

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

I agree this is the best way forward: fixing the most common issue with a simple design.

I don't want to bikeshed (feel free to postpone this conversation), but Rust went there and eventually settled with the ? postfix operator rather than a builtin function, for increased readability.

The gophercon proposal cites ? in the considered ideas and gives three reason why it was discarded: the first ("control flow transfers are as a general rule accompanied by keywords") and the third ("handlers are more naturally defined with a keyword, so checks should too") do not apply anymore. The second is stylistic: it says that, even if the postfix operator works better for chaining, it can still read worse in some cases like:

check io.Copy(w, check newReader(foo))

rather than:

io.Copy(w, newReader(foo)?)?

but now we would have:

try(io.Copy(w, try(newReader(foo))))

which I think it's clearly the worse of the three, as it's not even obvious anymore which is the main function being called.

So the gist of my comment is that all three reasons cited in the gophercon proposal for not using ? do not apply to this try proposal; ? is concise, very readable, it does not obscure the statement structure (with its internal function call hierarchy), and it is chainable. It removes even more clutter from the view, while not obscuring the control flow more than the proposed try() already does.

@jimmyfrasche

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

To clarify:

Does

func f() (n int, err error) {
  n = 7
  try(errors.New("x"))
  // ...
}

return (0, "x") or (7, "x")? I'd assume the latter.

Does the error return have to be named in the case where there's no decoration or handling (like in an internal helper function)? I'd assume not.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Your example returns 7, errors.New("x"). This should be clear in the full doc that will soon be submitted (https://golang.org/cl/180557).

The error result parameter does not need to be named in order to use try. It only needs to be named if the function needs to refer to it in a deferred function or elsewhere.

@dominikh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I am really unhappy with a built-in function affecting control flow of the caller. This is very unintuitive and a first for Go. I appreciate the impossibility of adding new keywords in Go 1, but working around that issue with magic built-in functions just seems wrong to me. It's worsened by the fact that built-ins can be shadowed, which drastically changes the way try(foo) behaves. Shadowing of other built-ins doesn't have results as unpredictable as control flow changing. It makes reading snippets of code without all of the context much harder.

I don't like the way postfix ? looks, but I think it still beats try(). As such, I agree with @rasky .

Edit: Well, I managed to completely forget that panic exists and isn't a keyword.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

The detailed proposal is now here (pending formatting improvements, to come shortly) and will hopefully answer a lot of questions.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@dominikh The detailed proposal discusses this at length, but please note that panic and recover are two built-ins that affect control flow as well.

@nictuku

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

One clarification / suggestion for improvement:

if the last argument supplied to try, of type error, is not nil, the enclosing function’s error result variable (...) is set to that non-nil error value before the enclosing function returns

Could this instead say is set to that non-nil error value and the enclosing function returns? (s/before/and)

On first reading, before the enclosing function returns seemed like it would eventually set the error value at some point in the future right before the function returned - possibly in a later line. The correct interpretation is that try may cause the current function to return. That's a surprising behavior for the current language, so a clearer text would be welcomed.

@purpleidea

This comment has been minimized.

Copy link

commented Jun 5, 2019

I think this is just sugar, and a small number of vocal opponents teased golang about the repeated use of typing if err != nil ... and someone took it seriously. I don't think it's a problem. The only missing things are these two built-ins:

https://github.com/purpleidea/mgmt/blob/a235b760dc3047a0d66bb0b9d63c25bc746ed274/util/errwrap/errwrap.go#L26

@webermaster

This comment has been minimized.

Copy link

commented Jun 5, 2019

Not sure why anyone ever would write a function like this but what would be the envisioned output for

try(foobar())

If foobar returned (error, error)

@dominikh

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I retract my previous concerns about control flow and I no longer suggest using ?. I apologize for the knee-jerk response (though I'd like to point out this wouldn't have happened had the issue been filed after the full proposal was available).

I disagree with the necessity for simplified error handling, but I'm sure that is a losing battle. try as laid out in the proposal seems to be the least bad way of doing it.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@webermaster Only the last error result is special for the expression passed to try, as described in the proposal doc.

@akyoto

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

Like @dominikh, I also disagree with the necessity of simplified error handling.

It moves vertical complexity into horizontal complexity which is rarely a good idea.

If I absolutely had to choose between simplifying error handling proposals, though, this would be my preferred proposal.

@cespare

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

It would be helpful if this could be accompanied (at some stage of accepted-ness) by a tool to transform Go code to use try in some subset of error-returning functions where such a transformation can be easily performed without changing semantics. Three benefits occur to me:

  • When evaluating this proposal, it would allow people to quickly get a sense for how try could be used in their codebase.
  • If try lands in a future version of Go, people will likely want to change their code to make use of it. Having a tool to automate the easy cases will help a lot.
  • Having a way to quickly transform a large codebase to use try will make it easy to examine the effects of the implementation at scale. (Correctness, performance, and code size, say.) The implementation may be simple enough to make this a negligible consideration, though.
@lestrrat

This comment has been minimized.

Copy link

commented Jun 5, 2019

I just would like to express that I think a bare try(foo()) actually bailing out of the calling function takes away from us the visual cue that function flow may change depending on the result.

I feel I can work with try given enough getting used, but I also do feel we will need extra IDE support (or some such) to highlight try to efficiently recognize the implicit flow in code reviews/debugging sessions

@Goodwine

This comment has been minimized.

Copy link

commented Jun 5, 2019

The thing I'm most concerned about is the need to have named return values just so that the defer statement is happy.

I think the overall error handling issue that the community complains about is a combination of the boilerplate of if err != nil AND adding context to errors. The FAQ clearly states that the latter is left out intentionally as a separate problem, but I feel like then this becomes an incomplete solution, but I'll be willing to give it a chance after thinking on these 2 things:

  1. Declare err at the beginning of the function.
    Does this work? I recall issues with defer & unnamed results. If it doesn't the proposal needs to consider this.
func sample() (string, error) {
  var err error
  defer fmt.HandleErrorf(&err, "whatever")
  s := try(f())
  return s, nil
}
  1. Assign values like we did in the past, but use a helper wrapf function that has the if err != nil boilerplate.
func sample() (string, error) {
  s, err := f()
  try(wrapf(err, "whatever"))
  return s, nil
}
func wrapf(err error, format string, ...v interface{}) error {
  if err != nil {
    // err = wrapped error
  }
  return err
}

If either work, I can deal with it.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

func sample() (string, error) {
  var err error
  defer fmt.HandleErrorf(&err, "whatever")
  s := try(f())
  return s, nil
}

This will not work. The defer will update the local err variable, which is unrelated to the return value.

func sample() (string, error) {
  s, err := f()
  try(wrapf(err, "whatever"))
  return s, nil
}
func wrapf(err error, format string, ...v interface{}) error {
  if err != nil {
    // err = wrapped error
  }
  return err
}

That should work. It will call wrapf even on a nil error, though.
This will also (continue to) work, and is IMO a lot clearer:

func sample() (string, error) {
  s, err := f()
  if err != nil {
      return "", wrap(err)
  }
  return s, nil
}

No one is going to make you use try.

@singhpradeep

This comment has been minimized.

Copy link

commented Jun 5, 2019

Not sure why anyone ever would write a function like this but what would be the envisioned output for

try(foobar())

If foobar returned (error, error)

Why would you return more than one error from a function? If you are returning more than one error from function, perhaps function should be split into two separate ones in the first place, each returning just one error.

Could you elaborate with an example?

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@cespare: It should be possible for somebody to write a go fix that rewrites existing code suitable for try such that it uses try. It may be useful to get a feel for how existing code could be simplified. We don't expect any significant changes in code size or performance, since try is just syntactic sugar, replacing a common pattern by a shorter piece of source code that produces essentially the same output code. Note also that code that uses try will be bound to use a Go version that's at least the version at which try was introduced.

@lestrrat: Agreed that one will have to learn that try can change control flow. We suspect that IDE's could highlight that easily enough.

@Goodwine: As @randall77 already pointed out, your first suggestion won't work. One option we have thought about (but not discussed in the doc) is the possibility of having some predeclared variable that denotes the error result (if one is present in the first place). That would eliminate the need for naming that result just so it can be used in a defer. But that would be even more magic; it doesn't seem justified. The problem with naming the return result is essentially cosmetic, and where that matters most is in the auto-generated APIs served by go doc and friends. It would be easy to address this in those tools (see also the detailed design doc's FAQ on this subject).

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@nictuku: Regarding your suggestion for clarification (s/before/and/): I think the code immediately before the paragraph you're referring to makes it clear what happens exactly, but I see your point, s/before/and/ may make the prose clearer. I'll make the change.

See CL 180637.

@deanveloper

This comment has been minimized.

Copy link

commented Jun 5, 2019

I actually really like this proposal. However, I do have one criticism. The exit point of functions in Go have always been marked by a return. Panics are also exit points, however those are catastrophic errors that are typically not meant to ever be encountered.

Making an exit point of a function that isn't a return, and is meant to be commonplace, may lead to much less readable code. I had heard about this in a talk and it is hard to unsee the beauty of how this code is structured:

func CopyFile(src, dst string) error {
	r, err := os.Open(src)
	if err != nil {
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}
	defer r.Close()

	w, err := os.Create(dst)
	if err != nil {
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}

	if _, err := io.Copy(w, r); err != nil {
		w.Close()
		os.Remove(dst)
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}

	if err := w.Close(); err != nil {
		os.Remove(dst)
		return fmt.Errorf("copy %s %s: %v", src, dst, err)
	}
}

This code may look like a big mess, and was meant to by the error handling draft, but let's compare it to the same thing with try.

func CopyFile(src, dst string) error {
	defer func() {
		err = fmt.Errorf("copy %s %s: %v", src, dst, err)
	}()
	r, err := try(os.Open(src))
	defer r.Close()

	w, err := try(os.Create(dst))

	defer w.Close()
	defer os.Remove(dst)
	try(io.Copy(w, r))
	try(w.Close())

	return nil
}

You may look at this at first glance and think it looks better, because there is a lot less repeated code. However, it was very easy to spot all of the spots that the function returned in the first example. They were all indented and started with return, followed by a space. This is because of the fact that all conditional returns must be inside of conditional blocks, thereby being indented by gofmt standards. return is also, as previously stated, the only way to leave a function without saying that a catastrophic error occurred. In the second example, there is only a single return, so it looks like the only thing that the function ever should return is nil. The last two try calls are easy to see, but the first two are a bit harder, and would be even harder if they were nested somewhere, ie something like proc := try(os.FindProcess(try(strconv.Atoi(os.Args[1])))).

Returning from a function has seemed to have been a "sacred" thing to do, which is why I personally think that all exit points of a function should be marked by return.

@s4n-gt

This comment has been minimized.

Copy link

commented Jun 5, 2019

Someone has already implemented this 5 years ago. If you are interested, you can
try this feature

https://news.ycombinator.com/item?id=20101417

I implemented try() in Go five years ago with an AST preprocessor and used it in real projects, it was pretty nice: https://github.com/lunixbochs/og

Here are some examples of me using it in error-check-heavy functions: https://github.com/lunixbochs/poxd/blob/master/tls.go#L13

@jasonmoo

This comment has been minimized.

Copy link

commented Jun 5, 2019

I appreciate the effort that went into this. I think it's the most go-ey solution I've seen so far. But I think it introduces a bunch of work when debugging. Unwrapping try and adding an if block every time I debug and rewrapping it when I'm done is tedious. And I also have some cringe about the magical err variable that I need to consider. I've never been bothered by the explicit error checking so perhaps I'm the wrong person to ask. It always struck me as "ready to debug".

@Goodwine

This comment has been minimized.

Copy link

commented Jun 5, 2019

@griesemer
My problem with your proposed use of defer as a way to handle the error wrapping is that the behavior from the snippet I showed (repeated below) is not very common AFAICT, and because it's very rare then I can imagine people writing this thinking it works when it doesn't.

Like.. a beginner wouldn't know this, if they have a bug because of this they won't go "of course, I need a named return", they would get stressed out because it should work and it doesn't.

var err error
defer fmt.HandleErrorf(err);

try is already too magic so you may as well go all the way and add that implicit error value. Think on the beginners, not on those who know all the nuances of Go. If it's not clear enough, I don't think it's the right solution.

Or... Don't suggest using defer like this, try another way that's safer but still readable.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@deanveloper It is true that this proposal (and for that matter, any proposal trying to attempt the same thing) will remove explicitly visible return statements from the source code - that is the whole point of the proposal after all, isn't it? To remove the boilerplate of if statements and returns that are all the same. If you want to keep the return's, don't use try.

We are used to immediately recognize return statements (and panic's) because that's how this kind of control flow is expressed in Go (and many other languages). It seems not far fetched that we will also recognize try as changing control flow after some getting used to it, just like we do for return. I have no doubt that good IDE support will help with this as well.

@buchanae

This comment has been minimized.

Copy link

commented Jun 5, 2019

I have two concerns:

  • named returns have been very confusing, and this encourages them with a new and important use case
  • this will discourage adding context to errors

In my experience, adding context to errors immediately after each call site is critical to having code that can be easily debugged. And named returns have caused confusion for nearly every Go developer I know at some point.

A more minor, stylistic concern is that it's unfortunate how many lines of code will now be wrapped in try(actualThing()). I can imagine seeing most lines in a codebase wrapped in try(). That feels unfortunate.

I think these concerns would be addressed with a tweak:

a, b, err := myFunc()
check(err, "calling myFunc on %v and %v", a, b)

check() would behave much like try(), but would drop the behavior of passing through function return values generically, and instead would provide the ability to add context. It would still trigger a return.

This would retain many of the advantages of try():

  • it's a built-in
  • it follows the existing control flow WRT to defer
  • it aligns with existing practice of adding context to errors well
  • it aligns with current proposals and libraries for error wrapping, such as errors.Wrap(err, "context message")
  • it results in a clean call site: there's no boilerplate on the a, b, err := myFunc() line
  • describing errors with defer fmt.HandleError(&err, "msg") is still possible, but doesn't need to be encouraged.
  • the signature of check is slightly simpler, because it doesn't need to return an arbitrary number of arguments from the function it is wrapping.
@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@s4n-gt Thanks for this link. I was not aware of it.

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@Goodwine Point taken. The reason for not providing more direct error handling support is discussed in the design doc in detail. It is also a fact that over the course of a year or so (since the draft designs published at last year's Gophercon) no satisfying solution for explicit error handling has come up. Which is why this proposal leaves this out on purpose (and instead suggests to use a defer). This proposal still leaves the door open for future improvements in that regard.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

The proposal mentions changing package testing to allow tests and benchmarks to return an error. Though it wouldn’t be “a modest library change”, we could consider accepting func main() error as well. It’d make writing little scripts much nicer. The semantics would be equivalent to:

func main() {
  if err := newmain(); err != nil {
    println(err.Error())
    os.Exit(1)
  }
}
@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@mishak87 We address this in the detailed proposal. Note that we have other built-ins (try, make, unsafe.Offsetof, etc.) that are "irregular" - that's what built-ins are for.

@networkimprov

This comment has been minimized.

Copy link

commented Jun 14, 2019

@rsc, super useful! If you're still revising it, maybe linkify the #id issue refs? And font-style sans-serif?

@owais

This comment has been minimized.

Copy link

commented Jun 14, 2019

This probably has been covered before so I apologize for adding even more noise but just wanted to make a point about try builtin vs the try ... else idea.

I think try builtin function can be a bit frustrating during development. We might occasionally want to add debug symbols or add more error specific context before returning. One would have to re-write a line like

user := try(getUser(userID))

to

user, err := getUser(userID)
if err != nil {  
    // inspect error here
    return err
}

Adding a defer statement can help but it's still not the best experience when a function throws multiple errors as it would trigger for every try() call.

Re-writing multiple nested try() calls in the same function would be even more annoying.

On the other hand, adding context or inspection code to

user := try getUser(userID)

would be as simple as adding a catch statement at end followed by the code

user := try getUser(userID) catch {
   // inspect error here
}

Removing or temporarily disabling a handler would be as simple as breaking the line before catch and commenting it out.

Switching between try() and if err != nil feels a lot more annoying IMO.

This also applies to adding or removing error context. One can write try func() while prototyping something very quickly and then add context to specific errors as needed as the program matures as opposed to try() as a built-in where one would have to re-write the lines to add context or add extra inspection code during debugging.

I'm sure try() would be useful but as I imagine using it in my day to day work, I can't help but imagine how try ... catch would be so much more helpful and much less annoying when I'd need to add/remove extra code specific to some errors.


Also, I feel that adding try() and then recommending to use if err != nil to add context is very similar to having make() vs new() vs := vs var. These features are useful in different scenarios but wouldn't it be nice if we had less ways or even a single way to initialize variables? Of course no one is forcing anyone to use try and people can continue to use if err != nil but I feel this will split error handling in Go just like the multiple ways to assign new variables. I think whatever method is added to the language should also provide a way to easily add/remove error handlers instead of forcing people to rewrite entire lines to add/remove handlers. That doesn't feel like a good outcome to me.

Sorry again for the noise but wanted to point it out in case someone wanted to write a separate detailed proposal for the try ... else idea.

//cc @brynbellomy

@griesemer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

Thanks, @owais, for bringing this up again - it's a fair point (and the debugging issue has indeed been mentioned before). try does leave the door open for extensions, such as a 2nd argument, which could be a handler function. But it is true that a try function doesn't make debugging easier - one may have to rewrite the code a bit more than a try-catch or try - else.

@alanfo

This comment has been minimized.

Copy link

commented Jun 14, 2019

@owais

Adding a defer statement can help but it's still not the best experience when a function throws multiple errors as it would trigger for every try() call.

You could always include a type switch in the deferred function which would handle (or not) different types of error in an appropriate way before returning.

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 14, 2019

Given the discussion thus far — specifically the responses from the Go team — I am getting the strong impression the team plans to move forward with the proposal that is on the table. If yes, then a comment and a request:

  1. The as-is proposal IMO will result in a non-insignificant reduction in code quality in the publicly available repos. My expectation is many developers will take the path of least resistance, effectively use exception handling techniques and choose to use try() instead of handling errors at the point they occur. But given the prevailing sentiment on this thread I realize that any grandstanding now would just be fighting a losing battle so I am just registering my objection for posterity.

  2. Assuming that the team does move forward with the proposal as currently written, can you please add a compiler switch that will disable try() for those who do not want any code that ignores errors in this manner and to disallow programmers they hire from using it? (via CI, of course.) Thank you in advance for this consideration.

@Goodwine

This comment has been minimized.

Copy link

commented Jun 14, 2019

can you please add a compiler switch that will disable try()

This would have to be on a linting tool, not on the compiler IMO, but I agree

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 15, 2019

This would have to be on a linting tool, not on the compiler IMO, but I agree

I am explicitly requesting a compiler option and not a linting tool because to disallow compiling such as option. Otherwise it will be too easy to "forget" to lint during local development.

@dpinela

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

@mikeschinkel Wouldn't it be just as easy to forget to turn on the compiler option in that situation?

@deanveloper

This comment has been minimized.

Copy link

commented Jun 15, 2019

Compiler flags should not change the spec of the language. This is much more fit for vet/lint

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 15, 2019

Wouldn't it be just as easy to forget to turn on the compiler option in that situation?

Not when using tools like GoLand where there is no way to force a lint to be run before a compile.

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 15, 2019

Compiler flags should not change the spec of the language.

-nolocalimports changes the spec, and -s warns.

@deanveloper

This comment has been minimized.

Copy link

commented Jun 15, 2019

Compiler flags should not change the spec of the language.

-nolocalimports changes the spec, and -s warns.

No, it doesn't change the spec. Not only does the grammar of the language continue to remain the same, but the spec specifically states:

The interpretation of the ImportPath is implementation-dependent but it is typically a substring of the full file name of the compiled package and may be relative to a repository of installed packages.

@deanveloper

This comment has been minimized.

Copy link

commented Jun 15, 2019

Not when using tools like GoLand where there is no way to force a lint to be run before a compile.

https://github.com/vmware/dispatch/wiki/Configure-GoLand-with-golint

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 15, 2019

@deanveloper

https://github.com/vmware/dispatch/wiki/Configure-GoLand-with-golint

Certainly that exists, but you are comparing apple-to-organges. What you are showing is a file watcher that runs on files changing and since GoLand autosaves files that means it runs constantly which generates far more noise than signal.

The lint always does not and cannot (AFAIK) be configured to as a pre-condition for running the compiler:

image

No, it doesn't change the spec. Not only does the grammar of the language continue to remain the same, but the spec specifically states:

You are playing with semantics here instead of focusing on the outcome. So I will do the same.

I request that a compiler option be added that will disallow compiling code with try(). That is not a request to change the language spec, it is just a request to for the compiler to halt in this special case.

And if it helps, the language spec can be updated to say something like:

The interpretation of try() is implementation-dependent but it is typically a one that triggers a return when the last parameter is an error however it can be implemented to not be allowed.

@networkimprov

This comment has been minimized.

Copy link

commented Jun 15, 2019

The time to ask for a compiler switch or vet check is after the try() prototype lands in 1.14(?) tip. At that point you'd file a new issue for it (and yes I think it's a good idea). We've been asked to restrict comments here to factual input about the current design doc.

@damienfamed75

This comment has been minimized.

Copy link

commented Jun 15, 2019

Hi so just to add on to the whole issue with adding debug statements and such during development.
I think that the second parameter idea is fine for the try() function, but another idea just to throw it out there is by adding an emit clause to be a second part for try().

For instance, I believe when developing and such there could be a case when I want to call fmt for this instant to print the error. So I could go from this:

func writeStuff(filename string) (io.ReadCloser, error) {
    f := try(os.Open(filename))
    try(fmt.Fprintf(f, "stuff\n"))

    return f, nil
}

Can be re-written to something like this for debug statements or general handling or the error before returning.

func writeStuff(filename string) (io.ReadCloser, error) {
    emit err {
        fmt.Printf("something happened [%v]\n", err.Error())
        return nil, err
    }

    f := try(os.Open(filename))
    try(fmt.Fprintf(f, "stuff\n"))

    return f, nil
}

So here I did end up putting a proposal for a new keyword emit which could be a statement or a one liner for immediate returning like the initial try() functionality:

emit return nil, err

What the emit would be is essentially just a clause where you can put any logic you wish in it if the try() gets triggered by an error not equalling nil. Another ability with the emit keyword is that you're able to access the error right there if you add just after the keyword a variable name such as I did in the first example using it.

This proposal does create a little verbosity to the try() function, but I think it's at least a little more clear on what is happening with the error. This way you're able to decorate the errors too without having it jammed all into one line and you can see how the errors are handles immediately when you're reading the function.

@deanveloper

This comment has been minimized.

Copy link

commented Jun 15, 2019

This is a response to @mikeschinkel, I'm putting my response in a detail block so that I don't clutter up the discussion too much. Either way, @networkimprov is correct that this discussion should be tabled until after this proposal gets implemented (if it does).

details about a flag to disable try @mikeschinkel

The lint always does not and cannot (AFAIK) be configured to as a pre-condition for running the compiler:

Reinstalled GoLand just to test this. This seems to work just fine, the only difference being is that if the lint finds something it doesn't like, it doesn't fail the compilation. That could easily be fixed with a custom script though, that runs golint and fails with a nonzero exit code if there is any output.
image

(Edit: I fixed the error that it was trying to tell me at the bottom. It was running fine even while the error was present, but changing "Run Kind" to directory removed the error and it worked fine)

Also another reason why it should NOT be a compiler flag - all Go code is compiled from source. That includes libraries. That means that if you want to turn of try through the compiler, you'd be turning off try for every single one of the libraries you are using as well. It's just a bad idea to have it be a compiler flag.

You are playing with semantics here instead of focusing on the outcome.

No, I am not. Compiler flags should not change the spec of the language. The spec is very well layed-out and in order for something to be "Go", it needs to follow the spec. The compiler flags you have mentioned do change the behavior of the language, but no matter what, they make sure the language still follows the spec. This is an important aspect of Go. As long as you follow the Go spec, your code should compile on any Go compiler.

I request that a compiler option be added that will disallow compiling code with try(). That is not a request to change the language spec, it is just a request to for the compiler to halt in this special case.

It is a request to change the spec. This proposal in itself is a request to change the spec. Builtin functions are very specifically included in the spec.. Asking to have a compiler flag that removes the try builtin would therefore be a compiler flag that would change the spec of the language being compiled.

That being said, I think that ImportPath should be standardized in the spec. I may make a proposal for this.

And if it helps, the language spec can be updated to say something like [...]

While this is true, you would not want the implementation of try to be implementation dependent. It's made to be an important part of the language's error handling, which is something that would need to be the same across every Go compiler.

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 16, 2019

@deanveloper

"Either way, @networkimprov is correct that this discussion should be tabled until after this proposal gets implemented (if it does)."

Then why did you decide to ignore that suggestion and post in this thread anyway instead of waiting for later? You argued your points here while at the same time asserting that I should not challenge your points. Practice what you preach...

Given you choice, I will choose to respond too, also in a detail block

here:

"That could easily be fixed with a custom script though, that runs golint and fails with a nonzero exit code if there is any output."

Yes, with enough coding any problem can be fixed. But we both know from experience that the more complex a solution is the fewer people who want to use it will actually end up using it.

So I was explicitly asking for a simple solution here, not a roll-your-own solution.

"you'd be turning off try for every single one of the libraries you are using as well."

And that is explicitly the reason why I requested it. Because I want to ensure that all code that uses this troublesome "feature" will not make its way into executables we distribute.

"It is a request to change the spec. This proposal in itself is a request to change the spec."

It is ABSOLUTELY not a change to the spec. It is a request for a switch to change the behavior of the build command, not a change in language spec.

If someone asks for the go command to have a switch to display its terminal output in Mandarin, that is not a change to the language spec.

Similarly if go build were to sees this switch then it would simply issue an error message and halt when it comes across a try(). No language spec changes needed.

"It's made to be an important part of the language's error handling, which is something that would need to be the same across every Go compiler."

It will be a problematic part of the language's error handling and making it optional will allow those who want to avoid its problems to be able to do so.

Without the switch it is likely most people will just see as a new feature and embrace it and never ask themselves if in fact it should be used.

With the switch — and articles explaining the new feature that mention the switch — many people will understand that it has problematic potential and thus will allow the Go team to study if it was a good inclusion or not by seeing how much public code avoids using it vs. how public code uses it. That could inform design of Go 3.

"No, I am not. Compiler flags should not change the spec of the language."

Saying you are not playing semantics does not mean you are not playing semantics.

Fine. Then I instead request a new top level command called (something like) build-guard used to disallow problematic features during compilation, starting with disallowing try().

Of course the best outcome is if the try() feature is tabled with a plan to reconsider solving the issue a different way the future, a way in which the vast majority agrees with. But I fear the ship has already sailed on try() so I am hoping to minimize its downside.


So now if you truly agree with @networkimprov then hold your reply until later, as they suggested.

@networkimprov

This comment has been minimized.

Copy link

commented Jun 16, 2019

Sorry to interrupt, but I have facts to report :-)

I'm sure the Go team has already benchmarked defer, but I haven't seen any numbers...

$ go test -bench=.
goos: linux
goarch: amd64
BenchmarkAlways2-2      20000000                72.3 ns/op
BenchmarkAlways4-2      20000000                68.1 ns/op
BenchmarkAlways6-2      20000000                68.0 ns/op

BenchmarkNever2-2       100000000               16.5 ns/op
BenchmarkNever4-2       100000000               13.1 ns/op
BenchmarkNever6-2       100000000               13.5 ns/op

Source

package deferbench

import (
   "fmt"
   "errors"
   "testing"
)

func Always(iM, iN int) (err error) {
   defer func() {
      if err != nil {
         err = fmt.Errorf("d: %v", err)
      }
   }()
   if iN % iM == 0 {
      return errors.New("e")
   }
   return nil
}

func Never(iM, iN int) (err error) {
   if iN % iM == 0 {
      return fmt.Errorf("r: %v", errors.New("e"))
   }
   return nil
}

func BenchmarkAlways2(iB *testing.B) { for a := 0; a < iB.N; a++ { Always(1e2, a) }}
func BenchmarkAlways4(iB *testing.B) { for a := 0; a < iB.N; a++ { Always(1e4, a) }}
func BenchmarkAlways6(iB *testing.B) { for a := 0; a < iB.N; a++ { Always(1e6, a) }}

func BenchmarkNever2(iB *testing.B) { for a := 0; a < iB.N; a++ { Never(1e2, a) }}
func BenchmarkNever4(iB *testing.B) { for a := 0; a < iB.N; a++ { Never(1e4, a) }}
func BenchmarkNever6(iB *testing.B) { for a := 0; a < iB.N; a++ { Never(1e6, a) }}
@ugorji

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

@networkimprov

From https://github.com/golang/proposal/blob/master/design/32437-try-builtin.md#efficiency-of-defer (my emphasis in bold)

Independently, the Go runtime and compiler team has been discussing alternative implementation options and we believe that we can make typical defer uses for error handling about as efficient as existing “manual” code. We hope to make this faster defer implementation available in Go 1.14 (see also ** CL 171758 ** which is a first step in this direction).

i.e. defer is now 30% performance improvement for go1.13 for common usage, and should be faster and just as efficient as non-defer mode in go 1.14

@networkimprov

This comment has been minimized.

Copy link

commented Jun 16, 2019

Maybe someone can post numbers for 1.13 and the 1.14 CL?

Optimizations don't always survive contact with the enemy... er, ecosystem.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

1.13 defers will be about 30% faster:

name     old time/op  new time/op  delta
Defer-4  52.2ns ± 5%  36.2ns ± 3%  -30.70%  (p=0.000 n=10+10)

This is what I get on @networkimprov 's tests above (1.12.5 to tip):

name       old time/op  new time/op  delta
Always2-4  59.8ns ± 1%  47.5ns ± 1%  -20.57%  (p=0.008 n=5+5)
Always4-4  57.9ns ± 2%  43.5ns ± 1%  -24.96%  (p=0.008 n=5+5)
Always6-4  57.6ns ± 2%  44.1ns ± 1%  -23.43%  (p=0.008 n=5+5)
Never2-4   13.7ns ± 8%   3.8ns ± 4%  -72.27%  (p=0.008 n=5+5)
Never4-4   10.5ns ± 6%   1.3ns ± 2%  -87.76%  (p=0.008 n=5+5)
Never6-4   10.8ns ± 6%   1.2ns ± 1%  -88.46%  (p=0.008 n=5+5)

(I'm not sure why Never ones are so much faster. Maybe inlining changes?)

The optimizations for defers for 1.14 are not implemented yet, so we don't know what the performance will be. But we think we should get close to the performance of a regular function call.

@deanveloper

This comment has been minimized.

Copy link

commented Jun 16, 2019

Then why did you decide to ignore that suggestion and post in this thread anyway instead of waiting for later?

The details block was edited in later, after I had read @networkimprov's comment. I'm sorry for making it look like I had understood what he said and ignored it. I'm ending the discussion after this statement, I wanted to explain myself since you had asked me why I posted the comment.


Regarding the optimizations to defer, I'm excited for them. They help this proposal a little bit, making defer HandleErrorf(...) a bit less heavy. I still don't like the idea of abusing named parameters in order for this trick to work, though. How much is it expected to speed up by for 1.14? Should they run at similar speeds?

@thepudds

This comment has been minimized.

Copy link

commented Jun 16, 2019

@griesemer One area that might be worth expanding a bit more is how transitions work in a world with try, perhaps including:

  • The cost of transitioning between error decoration styles.
  • The classes of possible mistakes that might result when transitioning between styles.
  • What classes of mistakes would be (a) caught immediately by a compiler error, vs. (b) caught by vet or staticcheck or similar, vs. (c) might lead to a bug that might not be noticed or would need to be caught via testing.
  • The degree to which tooling might mitigate the cost and chance of error when transitioning between styles, and in particular, whether or not gopls (or another utility) could or should have a role in automating common decoration style transitions.

Stages of error decoration

This is not exhaustive, but a representative set of stages could be something like:

0. No error decoration (e.g., using try without any decoration).
1. Uniform error decoration (e.g., using try + defer for uniform decoration).
2. N-1 exit points have uniform error decoration, but 1 exit point has different decoration (e.g., perhaps a permanent detailed error decoration in just one location, or perhaps a temporary debug log, etc.).
3. All exit points each have unique error decoration, or something approaching unique.

Any given function is not going to have a strict progression through those stages, so maybe "stages" is the wrong word, but some functions will transition from one decoration style to another, and it could be useful to be more explicit about what those transitions are like when or if they happen.

Stage 0 and stage 1 seem to be sweet spots for the current proposal, and also happen to be fairly common use cases. A stage 0->1 transition seems straightforward. If you were using try without any decoration in stage 0, you can add something like defer fmt.HandleErrorf(&err, "foo failed with %s", arg1). You might at that moment also need to introduce named return parameters under the proposal as initially written. However, if the proposal adopts one of the suggestions along the lines of a predefined built-in variable that is an alias for the final error result parameter, then the cost and risk of error here might be small?

On the other hand, a stage 1->2 transition seems awkward (or "annoying" as others have said) if stage 1 was uniform error decoration with a defer. To add one specific bit of decoration at one exit point, first you would need to remove the defer (to avoid double decoration), then it seems one would need to visit all the return points to desugar the try uses into if statements, with N-1 of the errors getting decorated the same way and 1 getting decorated differently.

A stage 1->3 transition also seems awkward if done manually.

Mistakes when transitioning between decoration styles

Some mistakes that might happen as part of a manual desugaring process include accidentally shadowing a variable, or changing how a named return parameter is affected, etc. For example, if you look at the first and largest example in the "Examples" section of the try proposal, the CopyFile function has 4 try uses, including in this section:

        w := try(os.Create(dst))
        defer func() {
                w.Close()
                if err != nil {
                        os.Remove(dst) // only if a “try” fails
                }
        }()

If someone did an "obvious" manual desugaring of w := try(os.Create(dst)), that one line could be expanded to:

        w, err := os.Create(dst)
        if err != nil {
            // do something here
            return err
        }

That looks good at first glance, but depending on what block that change is in, that could also accidentally shadow the named return parameter err and break the error handling in the subsequent defer.

Automating transitioning between decoration styles

To help with the time cost and risk of mistakes, perhaps gopls (or another utility) could have some type of command to desugar a specific try, or a command desugar all uses of try in a given func that could be mistake-free 100% of the time. One approach might be any gopls commands only focus on removing and replacing try, but perhaps a different command could desugar all uses of try while also transforming at least common cases of things like defer fmt.HandleErrorf(&err, "copy %s %s", src, dst) at the top of the function into the equivalent code at each of the former try locations (which would help when transitioning from stage 1->2 or stage 1->3). That is not a fully baked idea, but perhaps worth more thought as to what is possible or desirable or updating the proposal with current thinking.

Idiomatic results?

A related comment is it is not immediately obvious is how frequently a programmatic mistake free transformation of a try would end up looking like normal idiomatic Go code. Adapting one of the examples from the proposal, if for example you wanted to desugar:

x1, x2, x3 = try(f())

In some cases, a programmatic transform that preserves behavior could end up with something like:

t1, t2, t3, te := f()  // visible temporaries
if te != nil {
        return x1, x2, x3, te
}
x1, x2, x3 = t1, t2, t3

The proposal talks about possible behavior differences between if and try due to named result parameters, but in that particular section it seems to be talking mainly about transitioning from if to try (in the section that concludes "While this is a subtle difference, we believe cases like these are rare. If current behavior is expected, keep the if statement."). In contrast, there might be different risks of mistakes worth elaborating when transitioning from try back to if while preserving identical behavior (including taking into account shadowing, named return parameters, := vs =, other uses of err in the same function, etc.).


In any event, sorry for the long comment, but it seems a fear of high transition costs between styles is underlying some of the concern expressed in some of the other comments posted here, and hence the suggestion to be more explicit about those transition costs and potential mitigations.

@mikeschinkel

This comment has been minimized.

Copy link

commented Jun 17, 2019

@thepudds I love you are highlighting the costs and potential bugs associated with how language features can either positive or negatively affect refactoring. It is not a topic I see often discussed, but one that can have a large downstream effect.

a stage 1->2 transition seems awkward if stage 1 was uniform error decoration with a defer. To add one specific bit of decoration at one exit point, first you would need to remove the defer (to avoid double decoration), then it seems one would need to visit all the return points to desugar the try uses into if statements, with N-1 of the errors getting decorated the same way and 1 getting decorated differently.

This is where using break instead of return shines with 1.12. Use it in a for range once { ... } block where once = "1" to demarcate the sequence of code that you might want to exit from and then if you need to decoration just one error you do it at the point of break. And if you need to decorate all errors you do it just before the sole return at the end of the method.

The reason it is such a good pattern is it is resilient to changing requirements; you rarely ever have to break working code to implement new requirements. And it is a cleaner and more obvious approach IMO than jumping back to the beginning of the method before then jumping out of it.

#fwiw

@networkimprov

This comment has been minimized.

Copy link

commented Jun 17, 2019

@randall77's results for my benchmark show a 40+ns per call overhead for both 1.12 & tip. That implies that defer can inhibit optimizations, rendering improvements to defer moot in some cases.

@randall77

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

@networkimprov Defer does currently inhibit optimizations, and that's part of what we'd like to fix. For instance, it would be nice to inline the body of the defer'd function just like we inline regular calls.

I fail to see how any improvements we make would be moot. Where does that assertion come from?

@networkimprov

This comment has been minimized.

Copy link

commented Jun 17, 2019

Where does that assertion come from?

The 40+ns per call overhead for a function with a defer to wrap the error didn't change.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2019

The changes in 1.13 are one part of optimizing defer. There are other improvements planned. This is covered in the design document, and in the part of the design document quoted at some point above.

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.