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: Go2: introduce check keyword for errors on LHS of assignment #46655

Closed
smasher164 opened this issue Jun 9, 2021 · 29 comments
Closed
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Milestone

Comments

@smasher164
Copy link
Member

smasher164 commented Jun 9, 2021

Edit:

  1. I have relaxed the requirement on excluding underscores in named return values. We can simply return the zero value of that type when there's an underscore.
  2. Referenced other similar issues.
  3. Added playground links for the "After" examples.
  4. Explain why named return values are necessary.
  5. Mention that check being a context-dependent keyword is different from the way identifiers work today, and could potentially be confusing to work with.
@smasher164 smasher164 added LanguageChange v2 A language change or incompatible library change error-handling Language & library change proposals that are about error handling. labels Jun 9, 2021
@smasher164 smasher164 added this to the Proposal milestone Jun 9, 2021
@earthboundkid
Copy link
Contributor

This seems substantively the same as the last check proposal, except on the LHS instead of a magic expression on RHS. The reason the last proposal was rejected AFAICT was that it turned out that check messed up code coverage percentages deeply (ie a line will count as covered even with no testing of error handling!), and no one wanted to bite that bullet. This proposal is not sufficiently different to get past that limitation, so I don't think it has much chance of being accepted.

@smasher164
Copy link
Member Author

@carlmjohnson If line coverage is a blocker for error handling proposals, then I think it's fundamentally impossible for a language change that simultaneously removes the error handling boilerplate while making independent lines show up in a coverage report.

That being said, I'm sympathetic to the idea that coverage tools should show if an error path is exercised. Any coverage tool/instrumentation/report would have to accommodate operations within a single line (or at least special-case the check).


Also when you refer to the previous check proposal, do you mean the check/handle one, or the try one? Unlike either of those, this proposal does not rely on a handler block and keeps the imperative style by annotating the LHS. It eliminates ambiguity around what happens when invoking a function that doesn't return an error as its last parameter.

func f() (error, error, int)

func g() (i int) {
    check e1, check e2, v := f()
    return v
}

desugars into

func g() (i int) {
    e1, e2, v := f()
    if e1 != nil {
        return i
    }
    if e2 != nil {
        return i
    }
    return v
}

@smasher164
Copy link
Member Author

@gptankit Feel free to draft a separate proposal for that idea, but it has been brought up before: #33233

Defining built-in function carries the risks that it looks like a regular function call. Inspecting code that redefines returnOnError would make it unclear that it's changing control flow.

One of the consequences behind this proposal is that an editor can highlight the check keyword differently, signifying during an assignment that something different is going on.

@ianlancetaylor
Copy link
Contributor

This seems similar to #32500 #32601 #32884 #33150.

@ianlancetaylor
Copy link
Contributor

I don't understand why this requires named result variables.

Purely as a matter of style, the new use of check seems to emphasize the error result, which tends to obscure the more important non-error code.

@ibudisteanu

This comment has been minimized.

@ianlancetaylor

This comment has been minimized.

@smasher164
Copy link
Member Author

smasher164 commented Jun 10, 2021

@ianlancetaylor

This seems similar to #32500 #32601 #32884 #33150.

I can update my proposal to mention these. However,


I don't understand why this requires named result variables.

I should have explained this more in the original proposal, but when annotating the error result, the natural question to ask is "what happens when the error is non-nil?"

  1. Some proposals argue that we return zero values for everything except the error value, but that forces us to adopt the convention that there is only a single error value returned as the last. If we went this route, then named return values aren't necessary, but we would be enforcing a convention.
  2. We could do a naked return, and was my original plan. But the assignment would fail in a nested scope where the err variable is shadowed. If we went this route, then named return values are still necessary, since they permit the naked return.
  3. We could return the values of the currently shadowed names for the named return values. This permits the user to redeclare err in a new scope. This is the approach I preferred, since it provides the most flexibility around where the assignment can happen.

Purely as a matter of style, the new use of check seems to emphasize the error result, which tends to obscure the more important non-error code.

Edit: This is a tradeoff made for readability. Anything less than a keyword, and the code might be too cryptic because the function exits early.

@sergiokessler
Copy link

Purely as a matter of style, the new use of check seems to emphasize the error result, which tends to obscure the more important non-error code.

if you look at the code:

func printSum(a, b string) error {
	x, err := strconv.Atoi(a)
	if err != nil {
		return err
	}
	y, err := strconv.Atoi(b)
	if err != nil {
		return err
	}
	fmt.Println("result:", x + y)
	return nil
}

the error handling code tends to obscure the more important non-error code.

@qingtao
Copy link

qingtao commented Jun 11, 2021

I think better error-handling

expr? // return if err

@smasher164
Copy link
Member Author

@qingtao One of the goals of this proposal is to make it more obvious to the reader that there are changes to control flow. This is why I chose a keyword over an operator. If you have an argument for using a ? operator, please file separate proposal.

@qingtao
Copy link

qingtao commented Jun 11, 2021

make it more obvious to the reader
@smasher164 sorry, add extra keywords check vs? The purpose is the same. Not much help for reading.

@smasher164
Copy link
Member Author

@qingtao Clearly there is a subjective element to this. If you feel that strongly about it, please file a separate proposal.

That being said, on one hand @ianlancetaylor has stated that

check seems to emphasize the error result

while you state that ? and check have the same level of readability. This is something that I presume will be decided through this review process.

  • A keyword takes up more space.
  • It can be syntactically highlighted as a keyword in an editor.
  • It is consistent with the other mechanisms of altering control flow in Go programs, those being return and panic, both of which use an identifier.

Therefore, I would not argue that a keyword and ? are the same.

@deanveloper
Copy link

deanveloper commented Jun 11, 2021

Something nice about Go is that general errors are simply values, the language doesn't give them any special treatment. This feature would contradict that idea. If we are to solve error handling, I would like it to be a solution that doesn't give errors special treatment. I'm alright with this solution, though. It does look a bit strange without syntax highlighting, however. Maybe my eyes just have to get used to it.

@urandom
Copy link

urandom commented Jun 11, 2021

the language doesn't give them any special treatment.

It kind of does though. The error interface is a universal-scope lower-cased exported type.

@deanveloper
Copy link

the language doesn't give them any special treatment.

It kind of does though. The error interface is a universal-scope lower-cased exported type.

Right, when I say “special treatment” I mean “behaves differently from other values”. Such as in Java with being able to throw Throwables and automatically unfurl the stack, or (not quite as drastically) Swift where errors have a special “slot” for functions to return an error.

Currently, the Go spec does not have any “special treatment” for errors besides providing a universal interface for them. Using this interface is entirely optional, and the language itself doesn’t “encourage” its use in any way (ie special language features which only work on the builtin error interface). I would like it to stay that way if possible. The standard library encourages its use, which (in my opinion) is good.

@smasher164
Copy link
Member Author

smasher164 commented Jun 11, 2021

@deanveloper

the language doesn't give them any special treatment. This feature would contradict that idea.

I believe most of the error handling proposals fall into this category, so this flaw is not unique to this proposal.

I would like it to be a solution that doesn't give errors special treatment

There are generalizations of this proposal that could be applied to non-error values, but I want to keep this narrow in scope. For example, check could be made to work with any interface or even nillable type.

However, I think even if errors were given special treatment, this is an example of a language evolving with the needs of users. Rather than introducing a type-level feature, we address the syntactic issue with a special form of assignment.

I suspect some might wonder how this proposal would interact with generics, where functions may start returning Result[T], defined as

type Result[T any] interface {
    T | error
} 

I believe check can be extended when that time comes to apply to Result values as well, as a form of destructuring assignment. But we should cross that bridge when we get to it.

Edit: One way I'm thinking about it is extending the definition of assignment to operate on errors. For pointers we have *p, slices and arrays we have arr[i], check is merely an operator defined on errors during assignment.

@deanveloper
Copy link

I believe most of the error handling proposals fall into this category, so this flaw is not unique to this proposal.

Yeah, pretty much all of them do. Not quite all though, however the ones that don't aren't very elegant... Not sure how possible it would be to do successfully.

There are generalizations of this proposal that could be applied to non-error values, but I want to keep this narrow in scope. For example, check could be made to work with any interface or even nillable type.

I think it would be quite strange (at least with the spelling check) for this to apply outside of errors. I'm not trying to say it should by any means. I just think that a "perfect" feature wouldn't treat errors as special values. Go doesn't need to shoot for perfect, though.

I believe check can be extended when that time comes to apply to Result values as well, as a form of destructuring assignment. But we should cross that bridge when we get to it.

This would be really cool provided that some kind of Result would end up being used. However a benefit of a system which doesn't treat errors specifically would work with Results as well

@nikandfor
Copy link

I almost always wrap my errors with a short message. In the example above I would have

func printSum(a, b string) error {
	x, err := strconv.Atoi(a)
	if err != nil {
		return errors.Wrap(err, "parse a")
	}

	y, err := strconv.Atoi(b)
	if err != nil {
		return errors.Wrap(err, "parse b")
	}

	fmt.Println("result:", x + y)

	return nil
}

check keyword looses that option. So I would have to drop my nice error descriptions or I wouldn't be able to use that proposal.

I don't also see any problem with these ifs. With empty lines it looks pretty clear what's happening here.

@smasher164
Copy link
Member Author

@nikandfor

This proposal isn't meant to replace if err != nil in every context, but in most usages.

That being said, typically the context associated with a returned error is scoped at the function level, and not at each return. In the example you gave above, parse a and parse b provide different context for each parameter name. So while this would be a place where if err != nil makes sense, I question the value gained from referring to the parameter name in the error message here. Why isn't something like parse: strconv.Atoi: parsing "asdf": invalid syntax good enough?

@leighmcculloch
Copy link
Contributor

Would the check return the err unaltered, or would it wrap the error with information about where the error occurred?

@smasher164
Copy link
Member Author

@leighmcculloch
check returns the err unaltered, so location information is lost. However, because deferred functions can modify return values, you can still add context (just not at the granularity of which line the error came from).

@leighmcculloch
Copy link
Contributor

It occurs to me that if something like check is to be added it must handle what for many is the most common case of error handling, where the error is wrapped and appropriately annotated. How to do that though is unclear to me.

Maybe something like the below, or that achieves the same effect as the below, but better because this seems not great:

x, check("annotation %s: %w", someInput, err) := f()

@smasher164
Copy link
Member Author

it must handle what for many is the most common case

I don't know that it's true that adding context to every return is the most common case of error handling. I think if per-return context is needed, one would want to draw attention to it, and the two extra lines of control flow serve that purpose.

Maybe something like the below

#33150 and #32500 suggest something similar. They are not very specific on evaluation order, and overlap with the work that error wrappers like fmt.Errorf do. Can the wrapper be any arbitrary function? Does it need to accept an argument of type error (as the first argument)? I'd be interested to see variants of this proposal that allow one to wrap at the assignment.

Otherwise, writing the function so that in most cases, the context can be provided at the function-level would allow one to reuse mechanisms like defer for adding context.

@ianlancetaylor
Copy link
Contributor

As noted above, this is similar to previous proposals that were not accepted. It adds a new keyword, so it is not backward compatible. Based on that and the discussion above this is a likely decline. Leaving open for four weeks for final comments.

@smasher164
Copy link
Member Author

Based on the upvote/downvote ratio, I can understand that this proposal is likely to be declined. However, for future reference, I'd like more clarity on why this language change is not considered backwards compatible. Although a new keyword is introduced, the keyword is not usable in a context where it is ambiguous. Typically, keywords are used in statements, and are not available on the left-hand side of an assignment. Moreover, there is no ambiguity with other identifiers on the left-hand side of an assignment, because currently it is not legal to have two identifiers next to each other on the left-hand side that are only separated by whitespace.

@ianlancetaylor
Copy link
Contributor

By definition, a new keyword is not backward compatible. As the language spec says, a keyword may not be used as an identifier.

Reading more closely, I see that although you say that check is a keyword, you mean something else: it is an identifier that has a special meaning in certain circumstances. Sorry for missing that point.

There isn't anything like that in Go today. In Go identifiers are always defined, or not, in a given scope. The approach you describe raises a different kind of concern: identifiers that only sometimes have a special meaning can be confusing for programmers, because minor changes can have surprising repercussions.

@smasher164
Copy link
Member Author

smasher164 commented Aug 24, 2021

identifiers that only sometimes have a special meaning can be confusing for programmers, because minor changes can have surprising repercussions

Understandable. I can imagine it being confusing for a programmer to see that

x, check err := foo()

and

x, check := foo()

have different semantics, although an editor highlighting check as a keyword would help. I will mention this flaw in the proposal text.

@ianlancetaylor
Copy link
Contributor

No change in consensus.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
error-handling Language & library change proposals that are about error handling. FrozenDueToAge LanguageChange Proposal Proposal-FinalCommentPeriod v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests