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: Go 2: use ? during variable assignment as implicit goto statement #64953

Closed
bsoudan opened this issue Jan 4, 2024 · 36 comments
Closed
Labels
error-handling Language & library change proposals that are about error handling. LanguageChange Suggested changes to the Go language Proposal Proposal-FinalCommentPeriod v2 An incompatible library change
Milestone

Comments

@bsoudan
Copy link

bsoudan commented Jan 4, 2024

Proposal Details

Throwing my hat into the already crowded ring for Go error handling with an extremely simple proposal: suffixing ? to a variable name in an assignment statement adds an implicit if (var != zeroval) { goto var-as-label; }.

Go proposal template attached at end.

Code Sample

@rsc's CopyFile example from https://go.googlesource.com/proposal/+/master/design/go2draft-error-handling-overview.md becomes:

func CopyFile(src, dst string) error {
	r, err? := os.Open(src)
	defer r.Close()

	w, err? := os.Create(dst)
	defer w.Close()

	_, dstErr? := io.Copy(w, r)
        dstErr? = w.Close()
        
	return nil

dstErr:
	os.Remove(dst)
	err = dstErr 

err:
	return fmt.Errorf("copy %s %s: %v", src, dst, err)
}

Notes

  • Go has separate namespaces for labels and variables, so there's no conflict between the two already. Try it on the playground!

  • Allows arbitrary composition of error handling logic using normal golang control flow. Re-use error variable names to re-use error handling logic, use different variable names to support unique error handling logic, order them appropriately to chain handlers together, short circuit fallthrough with a return, use goto for more complex ordering.

  • No support for if (var == zeroval) { goto var-as-label; }. Could potentially allow ! prefix in labels to support this form, for instance, to support the common golang ok idiom:

        value, ok? := <-ch
        return value, nil
    
    !ok:
        return value, errors.New("channel closed")
  • More than one ? test in a multiple assignment statement should be a compiler error. Use standard if err tests when multiple tests are needed in one assignment so control flow is clear.

    err1?, err2? := io.ErrShortWrite, errors.New("some other error")
    
        // ...
    
    err1:
        // should control jump here?
    
    err2:
        // or here?
       
  • I considered a switch/case-like version:

    func CopyFile(src, dst string) (int, error) {
        r, err? := os.Open(src)
        defer r.Close()
    
        w, err? := os.Create(dst)
        defer w.Close()
    
        _, dstErr? := io.Copy(w, r)
        dstErr? = w.Close()
            
        return bytes, nil
    
    when dstErr != nil:
        os.Remove(dst)
        err = dstErr 
        fallthrough
    
    when err != nil
        return 0, fmt.Errorf("copy %s %s: %v", src, dst, err)
    }

    But I don't see many advantages over the goto based version to justify the additional language spec/implementation complexity. (2024/01/08 -- broke this out into a separate proposal anyway: proposal: spec: support new form of switch statement during variable assignment which jumps to function-wide case blocks #65019)

Updates

  • 2024/01/04: After I wrote this, it came to my attention that golang doesn't support goto jumping over variable declarations. I've used goto in several golang programs but somehow never stumbled across that issue, so I was unaware. However, commenters pointed out that there is an open proposal to address that issue: proposal: spec: permit goto over declaration if variable is not used after goto label #26058. This would need implemented in order to properly support this feature.

  • 2024/01/04: @seankhliao pointed out there is a gist with an essentially identical proposal ( https://gist.github.com/dpremus/3b141157e7e47418ca6ccb1fc0210fc7) that was never submitted.

  • 2024/01/04: @seankhliao also pointed out there was a proposal (proposal: Go 2: extend "goto" - statement #53074) that also re-used variable names as labels. @ianlancetaylor didn't like overloading variable names with labels, though golang currently permits it.

    I do like the brevity of re-using the error variable as the label, less typing and cleaner looking code. Using typical go conventions, this would always be 'err' or '*Err' so it would be clear that this was an 'error goto' destination without having to use any special keywords. Could even enforce that with a 'go vet' check. It also means the error labels can be used for regular goto statements, which is nice for complex error handling chaining. If we need to distinguish the implicit error jump labels from standard goto labels, though, I propose this version for symmetry:

    func CopyFile(src, dst string) error {
        r, err? := os.Open(src)
        defer r.Close()
    
        w, err? := os.Create(dst)
        defer w.Close()
    
        _, dstErr? := io.Copy(w, r)
            dstErr? = w.Close()
            
        return nil
    
    ?dstErr:
        os.Remove(dst)
        err = dstErr 
    
    ?err:
        return fmt.Errorf("copy %s %s: %v", src, dst, err)
    }

    If this version is preferred, then I would also propose that we add the ability for the regular goto statement to jump to ? labels, as in 'goto ?err', to support complex error chaining.

  • 2024/01/04: @carlmjohnson asked if the label can appear before the assignment. I don't see any reason to use different rules for the implicit ? goto labels than standard goto and labels. Consider using the feature to implement error retry:

            attempts := 0
            var err error
    
        err:
            attempts++
            if attempts > 5 {
                return nil, errors.New("failed after 5 attempts with: %w", err)
            }
            if err == context.Canceled {
                return nil, err
            }
    
            resource, err? := getResource()
            result, err? := getData(resource)
            
            return result, nil

    If you tried something like this with a for loop, you'd need checks after each function to either break or continue. The more functions in the retry chain the more boilerplate you'd save using the ? feature.

  • 2024/01/12 -- @apparentlymart and @findleyr brought up cases like this:

    func _() {
      if cond {
        err? := "hi"
      }
    
      err := 1
    
    ?err:
      // What is the type of err here?
    }  

    but if we apply the 'it's just a normal goto` test, you can see that even today, the compiler already gives you a good error, as tested on the playground. Couldn't put a link for some reason, getting a server error when I click the share button.

    func main() {
      if true {
      	// err? := "hi", translated to:
      	err := "hi"
      	goto err
      }
    
      err := 1
    
    err:
      fmt.Println("Hello,", err)
    }
    
    // compilation error: ./prog.go:10:8: goto err jumps over declaration of err at ./prog.go:13:6

    Which is great, because you're writing confusing code. But it's a simple fix. a) don't do that because you wouldn't be able to with a normal goto, and b) just declare err as a variable in the outer scope.

    func main() {
      var err string
    
      if true {
      	// err? = "hi", translated to:
      	err = "hi"
      	goto err
      }
    
      // can't do this anymore, but that's probably a good thing for code clarity
      // err = 1
    
      err = "everything's fine now"
    
    err:
      fmt.Println("Hello,", err)
    }
    
    // Prints 'Hello, hi'.

    What I like so much about this proposal is the simplicity of it, which is very appealing to me. It's not inventing any new forms of flow control a new developer has to learn. After thinking through all these different cases, I keep coming back to 'it's just a normal goto'.

Go Proposal Template

  • Would you consider yourself a novice, intermediate, or experienced Go programmer?
    Experienced.

  • What other languages do you have experience with?
    Assembly, C, C++, Python, Erlang, several others

  • Would this change make Go easier or harder to learn, and why?
    A bit more control flow to learn with new ? suffix, but by moving all the error handling boilerplate to separate blocks, the main logic would be easier to follow, so could argue either way.

  • Has this idea, or one like it, been proposed before?
    I read through as many error handling proposals as I could and did my best with the github search feature but couldn't find anything as simple and straightforward.

  • Who does this proposal help, and why?
    All gophers that would like more concise error handling.

  • Is this change backward compatible?
    Yes, existing code does not need to be changed.

  • What is the cost of this proposal? (Every language change has a cost).
    More complex grammar,

  • How many tools (such as vet, gopls, gofmt, goimports, etc.) would be affected?
    Probably many, since this would be a change in language syntax.

  • What is the compile time cost?
    Not familiar enough with the compiler internals to answer this.

  • What is the run time cost?
    Probably negligible if the compiler can optimize out unnecessary case tests.

  • Do you have a prototype? (This is not required.)
    No, but would be happy to put one together with a little guidance.

  • How would the language spec change?
    Only adding support for a ? variable suffix.

  • Orthogonality: how does this change interact or overlap with existing features?
    Re-uses existing golang control flow so minimal interactions.

  • Does this affect error handling?
    This change is intended to provide a new control flow mechanism during variable assignment that can be used to make error handling less verbose, but is not exclusive to error handling.

  • If so, how does this differ from previous error handling proposals?
    From spec: error handling meta issue #40432, seems similar to those with this classification: 'Special characters, often ! or ?, that insert an error check in a function call or assignment.', but none of the ones I could find are as dead simple.

@gopherbot gopherbot added this to the Proposal milestone Jan 4, 2024
@bsoudan
Copy link
Author

bsoudan commented Jan 4, 2024

Did a bit more digging and testing after writing this up and came across this: https://go.dev/ref/spec#Goto_statements. Looks like goto can't skip over variable initializations, so looks like this proposal would have to create a separate construct anyway unless it's possible to eliminate the restriction on goto. Perhaps the switch/case-like version would work better after all, so it's clear the ? is something different than a standard goto, and that the cases are not standard labels:

func CopyFile(src, dst string) error {
	r, err? := os.Open(src)
	defer r.Close()

	w, err? := os.Create(dst)
	defer w.Close()

	_, dstErr? := io.Copy(w, r)
        dstErr? = w.Close()
        
	return nil

when dstErr:
	os.Remove(dst)
	err? = dstErr 

when err:
	return fmt.Errorf("copy %s %s: %v", src, dst, err)
}

@fzipp
Copy link
Contributor

fzipp commented Jan 4, 2024

Looks like goto can't skip over variable initializations, so looks like this proposal would have to create a separate construct anyway unless it's possible to eliminate the restriction on goto.

If one were to accept this proposal, then the restriction should be eliminated (refer to #27165 and #26058, with the former closed in favor of the latter) rather than introducing a separate construct.

@seankhliao seankhliao added LanguageChange Suggested changes to the Go language v2 An incompatible library change error-handling Language & library change proposals that are about error handling. labels Jan 4, 2024
@seankhliao
Copy link
Member

this looks similar to (not a proposal, but we did see this): https://gist.github.com/dpremus/3b141157e7e47418ca6ccb1fc0210fc7

also #53074 where mixing label and variable was rejected

@bsoudan
Copy link
Author

bsoudan commented Jan 4, 2024

Looks like goto can't skip over variable initializations, so looks like this proposal would have to create a separate construct anyway unless it's possible to eliminate the restriction on goto.

If one were to accept this proposal, then the restriction should be eliminated (refer to #27165 and #26058, with the former closed in favor of the latter) rather than introducing a separate construct.

Ah, great, so some thought is already being given to lifting the restriction, ty for the reference.

@fzipp
Copy link
Contributor

fzipp commented Jan 4, 2024

There have been many error handling proposals with ?, but I think this is one of the better ones:

  • it doesn't hide away the error variable
  • it reuses an existing control flow construct (goto labels)
  • it doesn't simply promote a bare return err as many of the other proposals do

@bsoudan
Copy link
Author

bsoudan commented Jan 4, 2024

this looks similar to (not a proposal, but we did see this): https://gist.github.com/dpremus/3b141157e7e47418ca6ccb1fc0210fc7

also #53074 where mixing label and variable was rejected

Ah, I hadn't seen that, basically exactly the same as my proposal. If mixing label and variable is an issue, then seems easily solvable with a keyword in front of the label, that just adds a bit more work.

@fzipp
Copy link
Contributor

fzipp commented Jan 4, 2024

then seems easily solvable with a keyword in front of the label

Introducing a new keyword is usually a non-starter for proposals due to backward-compatibility considerations.

@bsoudan
Copy link
Author

bsoudan commented Jan 4, 2024

Introducing a new keyword is usually a non-starter for proposals due to backward-compatibility considerations.

Doesn't have to be a new keyword, could re-use an existing one. Maybe else? case? Or even re-use the ? in the label name. I personally like the brevity of just using a label, and they're rarely used anyway, but seems like we have plenty of options here if the concept is good:

   err? := file.Close()
   return nil

else err:
   return fmt.Errorf("copy %s %s: %w", src, dst, err)

// or

case err:
   return fmt.Errorf("copy %s %s: %w", src, dst, err)

// or
   
?err:
   return fmt.Errorf("copy %s %s: %w", src, dst, err)

@earthboundkid
Copy link
Contributor

As a veteran of the Try Wars, I like this more than I expected to. Reusing the variable name as a label is clever. It requires #26058, but that's probably a good idea anyway.

What happens if the label is above the ??

@bsoudan
Copy link
Author

bsoudan commented Jan 4, 2024

What happens if the label is above the ??

I don't see any reason to do anything different than a normal goto. Looping back would be a legit use case. Consider using it to implement error retry:

        attempts := 0
        var err error

    err:
        attempts++
        if attempts > 5 {
            return nil, errors.New("failed after 5 attempts with: %w", err)
        }
        if err == context.Canceled {
            return nil, err
        }

        resource, err? := getResource()
        result, err? := getData(resource)
        
        return result, nil

If you tried something like this with a for loop, you'd need checks after each function to either break or continue. The more functions in the retry chain the more boilerplate you'd save using the ? feature.

It's interesting how I've circled wrt goto -- I started using goto/jmp in basic/assembly back when I started programming, moved on to higher level languages where I drank the kool-aid that it was evil and went to great lengths to avoid it, and now I'm back to a more balanced stance. Sometimes goto is just the right thing.

@apparentlymart
Copy link

I do broadly like this, to the extent that I like any proposals for shorthands for error handling. 😀 (I'm in the camp of "I don't actually have any problem with Go's current approach, but I'm not opposed to a good proposal to improve it.")

The only small concern I have is that it seems likely that some functions will have multiple different error-handling labels, as shown in the proposal's current example:

dstErr:
	os.Remove(dst)
	err = dstErr 

err:
	return fmt.Errorf("copy %s %s: %v", src, dst, err)

...but I suspect that falling through from one to another would more often be a bug rather than intention, even though I can see that in this specific example it was intentional.

My reason for suspecting that is that I very often write functions that have multiple error return points that each have their own error message, which under this proposal suggests that they would each have their own label. As long as I remember to write return at the end of each handler then there's no problem of course, but Go already decided to make fallthrough in switch an opt-in rather than an opt-out because that was a common bug in other C-derived languages, and so I'd worry about recreating that common bug with error handling in Go.

The alternative in the proposal that uses a when keyword to distinguish these from plain labels solves that, and I think I'd be inclined to say that the ability to require the explicit fallthrough keyword is good reason to reconsider that alternative, though as noted above it would probably need to reuse an existing keyword/symbol rather than introduce a new one.

@fzipp
Copy link
Contributor

fzipp commented Jan 5, 2024

My reason for suspecting that is that I very often write functions that have multiple error return points that each have their own error message, which under this proposal suggests that they would each have their own label.

If each have their own message then there's no point in factoring them out. You'd just write the ifs like today.

@bsoudan
Copy link
Author

bsoudan commented Jan 5, 2024

...but I suspect that falling through from one to another would more often be a bug rather than intention, even though I can see that in this specific example it was intentional.

Yeah, definitely a good point here -- but go's normal labels fall through, even if select and switch do not. So it just depends on whether your mental model when you see the error goto labels is 'labels' or 'switch'. I suspect a large number of go developers rarely use labels, so I agree with your concern.

For me, I think if we make them look like go labels, they should behave like go labels, and if we make them look like case statements, we should make them behave like case statements.

@bsoudan
Copy link
Author

bsoudan commented Jan 5, 2024

If each have their own message then there's no point in factoring them out. You'd just write the ifs like today.

I think there's a proposal floating out there to unpack function return arguments with three dots... if that gets implemented as well as my proposal, maybe something like this would be possible:

    resource, ?err, msg := getResource()..., "get resource"
    result, ?err, msg := getData()..., "get data"
    return result

err:
    return fmt.Errorf("during %s: %w", msg, err)

@apparentlymart
Copy link

For me, I think if we make them look like go labels, they should behave like go labels, and if we make them look like case statements, we should make them behave like case statements.

Agreed, although I think of it from the other perspective: we should choose which of the two it should look like based on how they are most likely to be used.

If the most likely is that there's only one error label for the entire function then I think the plain label style is defensible, but if this proposal seems likely to cause functions to end with a series of different error handling labels that are mutually-exclusive then that suggests a switch-like formulation (with explicit fallthrough where needed) for me.

Of course, I don't have any data either way, so I can't actually say whether my hunch is right. It would probably help to collect some more examples of code that would benefit from either shape of this improvement to see if one possibility stands out as more helpful than the other.

@apparentlymart
Copy link

apparentlymart commented Jan 5, 2024

If each have their own message then there's no point in factoring them out. You'd just write the ifs like today.

That's a fair objection, but with the way I tend to write things I would tend not to add a prefix at all if the prefixes would all be the same, because the prefixes would not be adding any new information. The caller might potentially add a prefix to distinguish its call to my function from other functions, but then the caller's back in the mode of using a separate prefix for each return again, and so the advantage is diminished.

I suppose now that you mention it I am essentially somewhat objecting to the motivating example from the proposal:

func CopyFile(src, dst string) error {
    // ....

    return fmt.Errorf("copy %s %s: %v", src, dst, err)
}

Personally, I would not have added the prefix here, because I'd consider it the caller's responsibility to report that the error occurred while it was copying a file, assuming that's interesting information. The caller might even choose to describe what it was doing in terms more appropriate for its own level of abstraction.

It's implied that any return value from CopyFile is an error while copying the file, and so this specific example feels totally redundant to me. (If the function were distinguishing between multiple different failure paths in its own implementation then that would be different, of course. It would be interesting to see an example of using this proposal to distinguish between errors from the os.Create call vs. errors in the io.Copy call.)

However, I don't really wish to derail this proposal by arguing about what error style is more appropriate. If there's evidence that I'm the outlier here, and that it's more common elsewhere to add a single prefix in each callee rather than multiple prefixes in different parts of the caller, then I concede this point.

@bsoudan
Copy link
Author

bsoudan commented Jan 5, 2024

However, I don't really wish to derail this proposal by arguing about what error style is more appropriate. If there's evidence that I'm the outlier here, and that it's more common elsewhere to add a single prefix in each callee rather than multiple prefixes in different parts of the caller, then I concede this point.

No matter which error style you prefer, I think there is still a lot of value in moving all the error handling logic out of the main body of the function -- it will make the logic easier to read and follow, as it won't have 3 lines of boilerplate after every function call that returns an error.

@atdiar
Copy link

atdiar commented Jan 5, 2024

My feedback is perhaps a bit subjective but I'm a bit annoyed by the addition of the? sigil.

I don't find it very legible overall if we compare it to the try proposal that is linked.

On the other hand (!!!), it is interesting that it allows to create named error handlers and apply them.

I would perhaps do it differently so that it remains closer to common go code and work as a kind of macro expansion.

handler1: (err error){
    return err
} 

handler2: (err error, f *os.File){
    f.Close()
    return err
} 

v, err := fn()
check(handler1, err) 
// happy path

But even that I don't really like either, even disregarding the fact that it's difficult to find the correct way to declare error handlers out of the flow and difficult to find a correct syntax for their application.

What I'm not sure about it is when the error handling needs to do more than simply returning an error. It means that the error handler needs to be aware of variables that look like they are out of its scope. Such is the case with handler2 above.

I still think that there is not much better than the way we currently handle errors.

@apparentlymart
Copy link

I thought it would be interesting to try this proposal with a slightly more complex situation, with the following requirements:

  • Each of the three main fallible steps of this function gets its own error prefix, so that there's more context about what failed.
  • All of the Close calls get error-checked too, with the close errors getting joined with any other errors that might've occurred.

Here's what I came up with. I'm not meaning to suggest that this is the only way or the best way to write this under the proposal, just trying to see what it might feel like to use in a more complex situation with lots of different error paths to handle.

func CopyFile(src, dst string) (err error) {
    r, openSrcErr? := os.Open(src)
    defer func () {
        if closeErr := r.Close(); closeErr != nil {
            err = errors.Join(err, fmt.Errorf("closing source file: %w", closeErr))
        }
    }()

    w, createDstErr? := os.Create(dst)
    defer func () {
        if err != nil {
            // If anything fails after we've created the file
            // then we'll remove the possibly-incomplete file
            // before returning.
            if removeErr := os.Remove(dst); removeErr != nil {
                err = errors.Join(err, fmt.Errorf("removing failed %q: %w", dst, removeErr))
            }
        }
        if closeErr := w.Close(); closeErr != nil {
            err = errors.Join(err, fmt.Errorf("closing destination file: %w", closeErr))
        }
    }()

    _, copyErr? := io.Copy(w, r)
        
    return nil

openSrcErr:
    return fmt.Errorf("opening source file %q: %w", src, openSrcErr)
    
createDstErr:
    return fmt.Errorf("creating destination %q: %w", dst, createDstErr)
    
copyErr:
    return fmt.Errorf("copying file contents: %w", copyErr)
}

It's debatable whether handling the errors from r.Close() is actually important for this function's abstraction, and also some of these specific error messages would be a little redundant in practice due to prefixes added by the os package functions themselves. I included these details anyway because my goal here was to see how this proposal might scale to more complicated cases. It could be reasonable to argue that one shouldn't actually use this proposal in these complex cases, and should reserve it only for simple cases like the original motivating example.

One specific papercut I ran into while writing this is that I'm accustomed to typically just repeatedly assigning to a single err variable most of the time, rather than using a separate variable for each distinct error path, and so I caught myself writing err out of habit instead of referring to openSrcErr, createDstErr, and copyErr separately. Because err is in scope, that would be a runtime nil rather than a compile-time error and so perhaps would be easy to miss during development and code-review. Perhaps that's something that would improve with practice, though, since of course I also wasn't born with the instinct to use err as the error variable.

Since I handled the os.Remove call as part of the defer rather than in the jump-and-fallthough style of the original example, this made all of the error-handling labels end with return. If that ends up being the norm then perhaps the fallthough concern I raised earlier is less significant. However, getting there did require using a named return value, which I know from previous discussions is quite offputting to some and might cause them to dislike how I formulated this.

@bsoudan
Copy link
Author

bsoudan commented Jan 5, 2024

I thought it would be interesting to try this proposal with a slightly more complex situation, with the following requirements:

  • Each of the three main fallible steps of this function gets its own error prefix, so that there's more context about what failed.
  • All of the Close calls get error-checked too, with the close errors getting joined with any other errors that might've occurred.

I think your example shows exactly why the functions themselves should add context to their error returns, i.e. if close returned an error that reported 'closing file $filename', then copyfile could just do a single error wrap as @rsc's original example showed and everything would make sense. 'copy $sourcefile name to $destfilename: closing $filename' would be the final composed result, and it would be clear that the destination file was the one with the problem.

@bsoudan
Copy link
Author

bsoudan commented Jan 5, 2024

My feedback is perhaps a bit subjective but I'm a bit annoyed by the addition of the? sigil.

I don't find it very legible overall if we compare it to the try proposal that is linked.

On the other hand (!!!), it is interesting that it allows to create named error handlers and apply them.

I would perhaps do it differently so that it remains closer to common go code and work as a kind of macro expansion.

handler1: (err error){
    return err
} 

handler2: (err error, f *os.File){
    f.Close()
    return err
} 

v, err := fn()
check(handler1, err) 
// happy path

I think we have different styles -- I definitely prefer conciseness, I find yours hard to read because it has so much extra noise. Though, I admittedly don't love the ? either, but for something as common as error checking I like being concise and for me ? means a test in most languages (consider C++'s ternary operator) so it maps nicely.

@atdiar
Copy link

atdiar commented Jan 6, 2024

I understand.
The thing is, since error variables can be reused, using the name of the variable as a label may be problematic if it has to be handled in different ways along the course of the program that is written.

It is also easy to forget to put a ? sigil and shadow an error because it's been reused a few lines below.

Having apparent error checking statements gives at least more visibility.

Or is this concern addressed somehow and I've overlooked it?

@psnilesh
Copy link

psnilesh commented Jan 8, 2024

As much as I'd like to see some changes in error handling, I'm finding it hard to get onboard this proposal.

My main concern is whether this approach will bring back the horrors of goto. I also suspect a lot devs will be tempted to consolidate error handling at the end of the function in a manner that will make it difficult to understand where the error originated from. For e.g

func foo() error {
   _, err? := doX();
   _, err? = doY();
   _, err? = doZ();
   return nil
err:
     // Was ErrOne returned by doX(), doY() or doZ() ?
     if errors.Is(err, ErrOne) { 
       ...
     } else if errors.Is(err, ErrTwo) {

     }
     ...
}

@bsoudan
Copy link
Author

bsoudan commented Jan 8, 2024

My main concern is whether this approach will bring back the horrors of goto.

Can you elaborate here?

I also suspect a lot devs will be tempted to consolidate error handling at the end of the function in a manner that will make it difficult to understand where the error originated from. For e.g

func foo() error {
   _, err? := doX();
   _, err? = doY();
   _, err? = doZ();
   return nil
err:
     // Was ErrOne returned by doX(), doY() or doZ() ?
     if errors.Is(err, ErrOne) { 
       ...
     } else if errors.Is(err, ErrTwo) {

     }
     ...
}

I think the 'more difficult to understand where the error originated from' is an issue with many of the error handling proposals though, even @rsc's original proposal I linked in my original message. That's just a tradeoff I think we're making to make error handling more concise -- for me at least, I find 90% (maybe even more) of the time I want it to go to the same place because I just need to wrap the error in some context and return.

I'm having a hard time thinking of a way to alleviate your concern without making the 90% case less convenient. For instance, we could disallow more than one ? test per variable, forcing each to jump to a different label, but then the typical case of 'wrap and return' means we have to duplicate the code in each label. For instance:

func foo(someContext aStruct) error {
   _, xErr? := doX();
   _, yErr? = doY();
   _, zErr? = doZ();
   return nil
xErr:
   return fooErr{someContext, xErr};
yErr:
   return fooErr{someContext, yErr};
zErr:
   return fooErr{someCnotext, zErr};
}

when I would much rather write (and read, because it makes it very clear these are all the same case without having to manually scan and compare each case):

func foo(someContext aStruct) error {
   _, err? := doX();
   _, err? = doY();
   _, err? = doZ();
   return nil
err:
   return fooErr{someContext, err};
}

@bsoudan
Copy link
Author

bsoudan commented Jan 8, 2024

I understand. The thing is, since error variables can be reused, using the name of the variable as a label may be problematic if it has to be handled in different ways along the course of the program that is written.

With this proposal, if you need to handle an error in a different manner, then you should simply use a different error variable name. This is illustrated in the original example. Unless I'm missing your point?

It is also easy to forget to put a ? sigil and shadow an error because it's been reused a few lines below.

I don't understand this statement?

Having apparent error checking statements gives at least more visibility.

Yeah, I think this is maybe the fundamental style thing here for those in the 'error checking is fine' and 'error checking isn't fine' camps. I'm in the latter, because I think including the error handling in the main control flow is noisy and obscures the main logic. I still want it to be visible of course, but moving it the error handling to the end of the function using labels consolidates it in one place and makes both (the main logic, and the error handling logic) easier to read and understand for me.

I wonder how people code too -- for me, first I write the main logic, not thinking too carefully about the error cases or handling them trivially because I'm focusing on making sure the function works. Then, once I'm done and happy with the logic, I do a second pass through the function to properly implement error handling. So this proposal maps nicely for me because I can just put a ? identifier after each err while I'm coding the main logic, implement a basic wrap-and-return at the end, and then during my second pass I can split out special error handling cases where needed by changing the variable name and adding a new label.

@atdiar
Copy link

atdiar commented Jan 8, 2024

@bsoudan

But it is not enforced is it? What if someone employs the traditional go style and reuses the same error name mistakenly?

Also re shadowing:

v, err? := f() // handled
v, err = g() //not handled
// ... 
w, err? := h() // handled

err: // handler definition

It makes the lack of handling of the middle error a bit easier to go unnoticed.
To be fair, we can have the problem with regular code but at least the lack of if err != nil{...} right where the error is produced is more conspicuous.

@bsoudan
Copy link
Author

bsoudan commented Jan 8, 2024

But it is not enforced is it? What if someone employs the traditional go style and reuses the same error name mistakenly?

But being able to re-use the same error name is a feature, because then it's clear both these checks use the same error handling logic.

Also re shadowing:

v, err? := f() // handled
v, err = g() //not handled
// ... 
w, err? := h() // handled

err: // handler definition

It makes the lack of handling of the middle error a bit easier to go unnoticed. To be fair, we can have the problem with regular code but at least the lack of if err != nil{...} right where the error is produced is more conspicuous.

Ah I see what you mean. The ? token stands out to me personally since it's not used in any other context, so I don't think for me it's any less conspicuous than the normal if boilerplate. But I know everyone reads code differently.

@apparentlymart
Copy link

For this concern of forgetting the ? on a subset of the statements, perhaps it's worth considering a rule (either in the compiler or in vet, depending on what seems most appropriate) that if a particular symbol/label is used with ? at least once then it should not be directly assigned without ? anywhere else in its lifetime.


This question did also make me think about the fact that labels are function-scoped while variables are block-scoped, and so the following is a little challenging to describe the rules for:

func example() error {
    err? := Something()
    if somethingElse {
      // This declares a new "err" available only in
      // this block.
      err? := SomethingElse()
    }

    err:
        // Which of the two "err" variables is live here?
}

Normal scoping rules would mean that the nested err would not be available in the err:-labelled statement, and so err there would presumably be nil due to it being bound to the function-level err that presumably got nil assigned to it after executing the first statement in the function.

The connection I see between this and the other point is that this situation seems like it might warrant another special rule constraining how the ? symbol can be used when it is applied to a symbol that's declared in a narrower scope than the label it refers to.

The existing rule that you can't jump over a variable declaration would already block this today, but this proposal seems to imply somehow loosening that rule, and so we'd presumably need to make sure we don't loosen it so far that this ambiguous situation becomes possible, or at least find a way to loosen it that seems useful.

@bsoudan
Copy link
Author

bsoudan commented Jan 8, 2024

For this concern of forgetting the ? on a subset of the statements, perhaps it's worth considering a rule (either in the compiler or in vet, depending on what seems most appropriate) that if a particular symbol/label is used with ? at least once then it should not be directly assigned without ? anywhere else in its lifetime.

Yeah, we could vet that case for a warning or even make it outright illegal. I don't have a strong opinion here.

The existing rule that you can't jump over a variable declaration would already block this today, but this proposal seems to imply somehow loosening that rule, and so we'd presumably need to make sure we don't loosen it so far that this ambiguous situation becomes possible, or at least find a way to loosen it that seems useful.

Very good point here about nested variable scoping -- though the loosening the 'goto-can't-skip-over-variable-declaration' rule is a separate proposal #26058, independent of this one.

To keep this proposal straightforward which is one of the whole points, my initial thought is ideally we'd just do whatever goto does today (or, in the future, with #26058 implemented), and if one needs something more complicated, fall back to regular if err != nil error handling. But I will think about it more when I have some time!

@findleyr
Copy link
Member

Borrowing from #53074 (comment), which discusses a similar proposal to overload variable names and label names:

This proposal overloads variable names and label names, such that the name of a label corresponds to the name of a variable. That is confusing, and would seem to work poorly if one wants to assign to a variable more than once.

Therefore, this is a likely decline. Leaving open for four weeks for final comments.
— rfindley for the language proposal review group

@bsoudan
Copy link
Author

bsoudan commented Jan 11, 2024

Borrowing from #53074 (comment), which discusses a similar proposal to overload variable names and label names:

This proposal overloads variable names and label names, such that the name of a label corresponds to the name of a variable. That is confusing, and would seem to work poorly if one wants to assign to a variable more than once.

Therefore, this is a likely decline. Leaving open for four weeks for final comments. — rfindley for the language proposal review group

We discussed this in the comment chain and I added an update that we could disambiguate labels by using a keyword or a prefix. (highlight added)

  • 2024/01/04: @seankhliao also pointed out there was a proposal (proposal: Go 2: extend "goto" - statement #53074) that also re-used variable names as labels. @ianlancetaylor didn't like overloading variable names with labels, though golang currently permits it.

    I do like the brevity of re-using the error variable as the label, less typing and cleaner looking code. Using typical go conventions, this would always be 'err' or '*Err' so it would be clear that this was an 'error goto' destination without having to use any special keywords. Could even enforce that with a 'go vet' check. It also means the error labels can be used for regular goto statements, which is nice for complex error handling chaining. If we need to distinguish the implicit error jump labels from standard goto labels, though, I propose this version for symmetry:

    func CopyFile(src, dst string) error {
        r, err? := os.Open(src)
        defer r.Close()
    
        w, err? := os.Create(dst)
        defer w.Close()
    
        _, dstErr? := io.Copy(w, r)
            dstErr? = w.Close()
            
        return nil
    
    ?dstErr:
        os.Remove(dst)
        err = dstErr 
    
    ?err:
        return fmt.Errorf("copy %s %s: %v", src, dst, err)
    }
    

    If this version is preferred, then I would also propose that we add the ability for the regular goto statement to jump to ? labels, as in 'goto ?err', to support complex error chaining.

@findleyr
Copy link
Member

findleyr commented Jan 12, 2024

We discussed this in the comment chain and I added an update that we could disambiguate labels by using a keyword or a prefix. (highlight added)

Thanks for pointing that out, @bsoudan, and for the thorough updates to the issue description.

That does make it clearer, though it still mixes the semantics of variables and labels. In particular, as noted by @apparentlymart:

Normal scoping rules would mean that the nested err would not be available in the err:-labelled statement, and so err there would presumably be nil due to it being bound to the function-level err that presumably got nil assigned to it after executing the first statement in the function.

Consider this related example (apologies if this has already been brought up):

func _() {
  if cond {
    err? := "hi"
  }

  err := 1

?err:
  // What is the type of err here?
}

Do we not allow the err? form to jump out of a given block? But then it is inconsistent with goto, and has less utility. Or do we could continue to disallow intervening declarations? But as others have pointed out, that also makes this form significantly less useful.

It's interesting how I've circled wrt goto -- I started using goto/jmp in basic/assembly back when I started programming, moved on to higher level languages where I drank the kool-aid that it was evil and went to great lengths to avoid it, and now I'm back to a more balanced stance. Sometimes goto is just the right thing.

I can relate to this, but I worry that this new form of error handling would encourage goto-like control flow in many places where it's not the right thing, and would impact readability. Sometimes goto is right, but it usually requires extra scrutiny.

@bsoudan
Copy link
Author

bsoudan commented Jan 12, 2024

That does make it clearer, though it still mixes the semantics of variables and labels.

Great, just don't want this proposal to be rejected for that reason because I think it is a very solvable problem. We could completely disambiguate with something like err? label := at the cost of some conciseness. But, this would give developers back the ability to just re-use the error variable, which would be nice. @rsc's CopyFile example becomes:

func CopyFile(src, dst string) error {
	r, err? defaultError := os.Open(src)
	defer r.Close()

	w, err? defaultError := os.Create(dst)
	defer w.Close()

	_, err? closeError := io.Copy(w, r)
        err ? closeError = w.Close()
        
	return nil

closeError:
	os.Remove(dst)

defaultError:
	return fmt.Errorf("copy %s %s: %v", src, dst, err)
}

Consider this related example (apologies if this has already been brought up):

func _() {
  if cond {
    err? := "hi"
  }

  err := 1

?err:
  // What is the type of err here?
}

Do we not allow the err? form to jump out of a given block? But then it is inconsistent with goto, and has less utility. Or do we could continue to disallow intervening declarations? But as others have pointed out, that also makes this form significantly less useful.

I noted in a response above I think we should just follow the normal goto rules (hopefully, eventually, with #26058 implemented), but I did promise to think about it more. I did spend more brainpower on it, and I still think the same thing. Just follow the normal goto rules, whatever they are.

Today, for your example, the compiler already gives you a good error, as tested on the playground. Couldn't put a link for some reason, getting a server error when I click the share button.

func main() {
	if true {
		// err? := "hi", translated to:
		err := "hi"
		goto err
	}

	err := 1

err:
	fmt.Println("Hello,", err)
}

// compilation error: ./prog.go:10:8: goto err jumps over declaration of err at ./prog.go:13:6

Which is great, because you're writing confusing code. But it's a simple fix. a) don't do that and b) just declare err as a variable in the outer scope.

func main() {
	var err string

	if true {
		// err? = "hi", translated to:
		err = "hi"
		goto err
	}

	// can't do this anymore, but that's probably a good thing for code clarity
	// err = 1

	err = "everything's fine now"

err:
	fmt.Println("Hello,", err)
}

// Prints 'Hello, hi'.

And to directly answer your questions:

Do we not allow the err? form to jump out of a given block? But then it is inconsistent with goto, and has less utility.

We should allow the err? form to jump out of a block if and when goto allows it. If the developer needs to use the scoped variable at the jump target but the jump target in is the outer scope, they can solve that problem easily by just declaring the error variable in the outer scope.

Or do we could continue to disallow intervening declarations? But as others have pointed out, that also makes this form significantly less useful.

That is a separate proposal that seems to be getting traction, #26058, that would allow jumping across intervening declarations. If implemented alongside this proposal I think your concern would go away.

I worry that this new form of error handling would encourage goto-like control flow in many places where it's not the right thing, and would impact readability.

I thought about this but I don't share the same concern -- goto exists now and it's not abused. Yes, this proposal would mean it was used more, but I think we should trust developers to make good judgement calls and write good code. I think the positive impact on readability (getting rid of err != nil boilerplate and moving it to the end of a function, which I think would be the majority of uses for this) would be worth the small number of abuses we'd be likely to see. And don't forget code can just still do it the old-fashioned way for complicated error handling, which would be more clear than a rat's nest of goto statements.

I think what I like so much about this proposal is the simplicity of it, which is very appealing to me. It's not inventing any new forms of flow control a new developer has to learn. After thinking through all these different cases, I keep coming back to 'it's just a normal goto'.

@hauntedness
Copy link

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

Is there any chance to figure out from which line comes the err? If no, I prefer inplace return rather than implicit goto.

@bambuo
Copy link

bambuo commented Jan 29, 2024

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

有没有机会弄清楚哪条线是错误的?如果没有,我更喜欢就地返回而不是隐式 goto。

Is it important to have a clear understanding of which os.Open error is causing when an error occurs?
If that's the case, but you use the same err?, it's indistinguishable.
Unless this is possible, it would add an additional symbol ! and two keywords when to, while also reusing the mechanism of the implicit goto label in ? ! needs to be interpreted as a trigger like effect, while when is used in conjunction with !. When the variable calibrated by ! is triggered, it should be executed. The result value of the execution can be reused with to and the err name, so that the same err name can be used for receiving and processing, The above are some divergent thinking, and whether it is reasonable also needs to be scrutinized
Of course, you can also use different names to indicate different errors, such as the second one

The first type

func CopyFile(src) error {
	r, err! when errors.Warp(err,"What is this") to err? := os.Open(src)
	defer r.Close()
	w, err? := os.Open(src)
	defer w.Close()
	return nil
err:
	return fmt.Errorf("copy %s: %v", src, err)
}

The second type

func CopyFile(src) error {
	r, err1? := os.Open(src)
	defer r.Close()
	w, err2? := os.Open(src)
	defer w.Close()
	return nil
err1:
	return fmt.Errorf("copy %s: %v", src, err)
err2:
	return fmt.Errorf("copy %s: %v", src, err)
}

@findleyr
Copy link
Member

findleyr commented Feb 7, 2024

As described in #64953 (comment), the overloading of variable names and label names is confusing, in part because variables and labels have different scoping rules.

No change in consensus, so declined.
— rfindley for the language proposal review group

@findleyr findleyr closed this as completed Feb 7, 2024
@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error-handling Language & library change proposals that are about error handling. LanguageChange Suggested changes to the Go language Proposal Proposal-FinalCommentPeriod v2 An incompatible library change
Projects
None yet
Development

No branches or pull requests