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: allow implicit conversion of error to bool in if statements, fmt on one line #38151

Closed
bradfitz opened this issue Mar 29, 2020 · 60 comments
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

@bradfitz
Copy link
Contributor

bradfitz commented Mar 29, 2020

A couple weeks back during a proposal review, reviewing the fiftieth or so error handling proposal, I made a somewhat flippant remark that if we made if err != nil {\n return nil, err \n}\n shorter, most of the error handling proposals would go away. (At least the more incomplete ones)

I then mentioned two things we could do pretty easily:

  1. allow, in if statements only, just:
if err {

... as syntactic sugar instead of:

if err != nil {

That is, if the static type of a variable is exactly and only error, allow conversion to a boolean.

"But that's not orthogonal, why would you make error special?" Yes. But errors are already special. They're the only interface predefined in the language. And they're incredibly common.

Even if somebody is momentarily confused why it works, it's at least not ambiguous.

IIRC, we discussed a bit whether to allow it in all boolean contexts, just in boolean expressions inside an if condition (so if !err { would work), or only when it's the entire expression in an if (only if err {).

  1. modify gofmt to format those on a single line (previously proposed and rejected in proposal: gofmt should allow short inlined if-return statements #27135):
if err { return err }

Later, if we do #21182, that extends to:

if err { return ..., err }

But if you have any non-return statement in the if body, then it formats as multiple lines.

Taken together, this makes error handling much lighter looking (the common motivation for new error handling proposals), while keeping flow control unchanged and explicit (a common argument against most error handling proposals).

I made these comments mostly as a joke at first, but the more we talked about, the more we didn't outright hate the idea. I reluctantly agreed to file an issue, so here it is.

@bradfitz bradfitz added LanguageChange Proposal error-handling Language & library change proposals that are about error handling. labels Mar 29, 2020
@gopherbot gopherbot added this to the Proposal milestone Mar 29, 2020
@ianlancetaylor ianlancetaylor added the v2 A language change or incompatible library change label Mar 29, 2020
@peterbourgon
Copy link

  1. allow just if err { instead of if err != nil {

Only after if and only if it's the whole expression? If so, sounds good.

  1. modify gofmt to format those on a single line: if err { return err }

Would this also work with annotated (wrapped) errors, e.g. fmt.Errorf("annotation: %w", err)? If so, sounds good; if not, strong -1.

@Jacalz
Copy link
Contributor

Jacalz commented Mar 29, 2020

This actually makes a lot of sense to do in my opinion. Both saves time writing and makes error handling look a lot cleaner. Sounds good to me 👍

@swdee

This comment has been minimized.

@seebs
Copy link
Contributor

seebs commented Mar 29, 2020

As a C programmer, I often miss if ptr constructs, but I recognize the confusion they can introduce.

Would you want this to work also for if a, err := foo(); err {?

I have actually been thinking about this same thing, because I frequently end up with, roughly:

a, err := f1(...)
if err != nil {
    return a, err
}
b, err := f2()
if err != nil {
    return b, err
}

and what I really want for a lot of these is basically "return these values if [this condition involving these values]". But in 99% of cases, the condition is "the last one, which is an error, is non-nil".

the ugly answer:
erreturn a, err

@justinfx
Copy link

I like the idea of being able to test for errors this way, but I really feel like if you were going to make this change, that you would just make the bigger change for truthy testing any value. I get that error is a special case interface but I don't think users are likely to think of that when they find they can only truthy test an error interface and not any type. It will feel like a quirk where the language is unbalanced.
But I do remember the reasoning for not having it for all types being something like it leads to confusion or bugs when you mistake the type you are testing? I don't know about that. You still have static typing and no-implicit-conversion to protect you later when you use the value in other ways.
What is the downside of this example?

var s []string
if s {
  ... 
} 

@flibustenet
Copy link

It sound like a good compromise when we can still choose to keep if err != nil on 3 lines if we want.

It should be easy to test and revert if needed. That could be pretty fun at the moment !

@creker
Copy link

creker commented Mar 29, 2020

@peterbourgon would be very strange that this

err := foo()
if err {}

is allowed but this

if err := foo(); err {}

is not.

@justinfx the problem is that empty slice and nil slice are both useful and practically identical in most of the cases (len, cap, append all treat them equally). But your example would probably handle them differently and cause mistakes and confusion.

@justinfx
Copy link

@creker , honestly I would have just thought of an empty and nil slice to both be false, if I were to use truthy testing instead of explicitely checking a length or comparing to nil. The language lets you call len on a nil slice to get a length of 0 anyways, so one could say it already allows nil and empty slice to be treated the same.
If the truthy behaviour is defined then its just something to know about the language.

@bradfitz
Copy link
Contributor Author

@swdee, there are plenty of proposals already open for new keywords like that. This is specifically what this issue is trying to avoid.

@bradfitz
Copy link
Contributor Author

@peterbourgon, returning wrapped errors seems reasonable, yes.

@bradfitz
Copy link
Contributor Author

@creker, writing:

if err := foo(); err {}

... separates the word "if" from the word "err" (error). Since it no longer reads "if error", I think it's totally reasonable to require that be written the normal err != nil way, with no fmt changes.

@bradfitz
Copy link
Contributor Author

@justinfx, many downsides. Like others said: it's ambiguous whether that means len > 0 or != nil.

@creker
Copy link

creker commented Mar 29, 2020

@bradfitz it's fine if gofmt doesn't format it as a one liner but it doesn't make sense disallowing

if err := foo(); err {
}

That just means language spec would have some strange rule about completely identical code (apart from err scope which is irrelevant here) being treated differently by the compiler. I would expect Go to be consistent in its syntax.

@peterbourgon
Copy link

@creker

it doesn't make sense disallowing . . .

Yes, it does.

@creker
Copy link

creker commented Mar 29, 2020

@peterbourgon what is the objective reasoning? I understand if you personally don't like how it looks but that shouldn't affect language spec if it introduces inconsistencies in it. Either allow both or don't implement the proposal at all.
Gofmt is where subjective reasoning should live and there formatting such cases as one line could be disallowed. There it actually makes sense because one liner could become too wide.

@seebs
Copy link
Contributor

seebs commented Mar 29, 2020

What is the downside of this example?

var s []string
if s {
  ... 
} 

That I don't know what it means. You might mean "if s is non-nil", you might mean "if s has a length other than zero". Heck, you might mean "if s contains at least one string which isn't an empty string".

I think it's pretty reasonable to only accept the exact form if err and not the longer form with the prior assignment. It's syntactic sugar, it can be narrowly-focused, and I'd definitely prefer the != nil to still be present in the longer lines so I have an easier time reading them.

if a, b, err := threeReturns(...); err

makes it a bit too easy for me to miss the nature of the condition.

if a, b, err := threeReturns(...); err != nil

is clearer.

The argument here is just that, for the very specific case of if err != nil, allowing the != nil to be omitted doesn't obscure things, because there's no other expressions nearby that can visually subsume the err.

I don't think this breaks the syntax much. The proposal here isn't to allow single error values to be truthy expressions; it's to allow the special case if errorValue as an alternative to requiring if statements to take truthy expressions.

@creker
Copy link

creker commented Mar 29, 2020

The proposal here isn't to allow single error values to be truthy expressions;

That's not decided per the proposal

IIRC, we discussed a bit whether to allow it in all boolean contexts, just in boolean expressions inside an if condition (so if !err { would work), or only when it's the entire expression in an if (only if err {).

It does introduce inconsistencies in the language if we start disallowing syntactic constructs just because we don't like them. If we make if err a special case and not treat error types as false in all possible cases (at least inside an if statement but ideally it should include all types of expressions) it makes the proposal even worse. That would make whole thing confusing and unintuitive. Image someone just starting to learn the language. How would you justify it? Right now the reasoning is "some people didn't like how it looks". If that's the price to save a few strokes then I vote against such proposal. Either implement it properly or don't implement it at all.

@peterbourgon
Copy link

@creker

It does introduce inconsistencies in the language if we start disallowing syntactic constructs just because we don't like them.

There's nothing wrong with this.

@creker
Copy link

creker commented Mar 29, 2020

@peterbourgon there is and it's called bad language design. Language should be consistent and intuitive. If someone sees if err then the only logical conclusion is that error types can be implicitly converted to booleans (and the proposal clearly says "allow conversion to a boolean"). That means all the following cases should also work

func foo() error {
}

if err := foo(); err {
}

err := foo()
var result bool
result = err

The last one is unfortunate consequence of the proposal but it must work, otherwise the whole thing becomes a mess of special case syntactic sugar that doesn't mean anything (in other words, it no longer means implicit conversion to boolean. It now means "we like how this looks, let's allow it").

@mvdan
Copy link
Member

mvdan commented Mar 29, 2020

Overall, I like how refreshingly simple this proposal is, compared to most other error handling proposals. It deals with the verbosity without adding too much magic.

But if you have any non-return statement in the if body, then it formats as multiple lines.

I think this rule could be simplified to: this kind of if statement is allowed to fit in a single line if its body is exactly one statement, and its length is short enough.

This rule is already in place in gofmt, for single-line functions. The heuristic is just not explicitly defined, because it might change in the future. See it in practice - click "format" on https://play.golang.org/p/rEyBUh5rj7V

this makes error handling much lighter looking (the common motivation for new error handling proposals)

I think another point in favor of the handle proposal was to consistently wrap errors in a function:

// go2 draft
func process(...) error {
    handle err { return fmt.Errorf("process: %w", err) }
    check doFoo()
    check doBar()
}

// this proposal
func process(...) error {
    if err := doFoo(); err { return fmt.Errorf("process: %w", err) }
    if err := doBar(); err { return fmt.Errorf("process: %w", err) }
    return nil
}

Note how we repeat the error wrapping/decoration. This is good if we want the call site to decorate an error with the function that was called, but I thought the point of the go2 draft was also to encourage functions to decorate errors with their own information when returning.

@seebs
Copy link
Contributor

seebs commented Mar 29, 2020

I think it's good to be consistent and logical up to a point, but syntactic sugar is frequently special-case exceptions. We already have unusual rules on the behavior of if statements; the "if a := expr; a != 23" construct isn't a way you can spell an expression in any context other than an if statement, and it's not quite equivalent to having the first expression simply before the if statement, because it has different scoping.

I would prefer "an if statement can take an expression which is either any expression of type bool, or a single identifier referring to an object of type error" to "automatic conversion of some things to bool". I don't think the proposed change here would have much impact on my daily life, but I'd probably use it. A change allowing error in general to be used as a conditional expression, or pointers or interfaces in general, would be a major change, and I'd guess that I'd end up being hurt by it more than I'd be helped by it. (Although I'm not sure; I got very used to "if (p)" in C, and sort of miss it sometimes.)

I would be pretty strongly opposed to a general "implicit conversion to boolean". I would not object to conditional operations, like for or if loops, being able to take interfaces, or pointers, or possibly just error in particular.

As it gets more general, it gets more confusing. Consider:

for n, err := r.Read(buf); !err; n, err = r.Read(buf) { ... }

I would be thrown off by this even given the assumption that "err" could be used as a control expression for an if statement. It wouldn't confuse me in C, but in C, we're already used to pointers being usable as truth values.

That said... What about cases where we'd return early on success?

At the risk that it will make this proposal seem dangerous, consider:

err := happyPath()
if !err {
  return nil
}
// handle the complicated path

This, coupled with the above, makes me think that this absolutely should not imply a general conversion of error to bool, but rather, should be a special case of the if statement.

@jimmyfrasche
Copy link
Member

I'm fine with 2. I'd prefer if it's further limited only to if's that contain a return/break/continue statement. That's not especially important to me.

1 makes me uneasy.

If it's just limited to error, does it mean I can't use it with a value of net.Error? I can see why not, but overall the definition seems a little too special. I don't think it's going too far to just extend it to any interface value even if the target is specifically error.

Other boolean contexts seems like it could get convoluted. I could see if !err { as a single special case but I think disallowing it wouldn't provide any problems. Writing if err == nil happens but it's fairly rare, ime.

I don't have any problem with if x := f(); x {}. It looks exactly as clear to me. It can be avoided and linted against by parties that find it a concern.

@zeebo
Copy link
Contributor

zeebo commented Mar 30, 2020

Limiting the if to only contain a return/break/continue makes it not apply for testing, like

if err != nil { t.Fatal("unexpected error:", err) }
if a != b { t.Fatal(a, "does not equal", b) }

which that style I think would help remove the desire and proliferation of incompatible assertion style testing libraries.

For those situations, one could even consider going further and adding brace alignment to aid visual scanning:

if err != nil { t.Fatal("unexpected error:", err) }
if a != b     { t.Fatal(a, "does not equal", b) }

But, there's no existing precedent to do that alignment with function or struct literals like there is for allowing the whole thing to exist on a single line.


During the try proposal, I also brought the single line if statement idea up (#32437 (comment)) and many of the points I describe there also apply here:

  • It requires no language changes.
  • The formatting rule is simple, clear, and has precedent with function and struct literals.
  • It doesn't cause any existing code to need any changes, and it would all still be in canonical gofmt form.
  • It is hard to imagine that a formatting only change could cause any negative interactions with any future extensions to error handling that may be considered.
  • Applies to testing and not just error handling.

In my estimation, the amount of win you can get by allowing the discerning author to place if statements on a single line easily pays for the complexity and implementation cost. It has no negative interactions on existing or future code, greatly reducing the risk. And since there have already been changes to gofmt that changed existing code's formatting, the path to reverting it in the future is open and minimally expensive.

As far as worries about abuse, there are already a multitude of places where gofmt allows the author to choose between one line or multiple: functions (literals and declarations), struct/map/slice literals, type definitions, allowing semicolons on a single line (https://play.golang.org/p/Yr0bfdQPTrp), vertical spacing between imports, and I'm sure more. Yet we rarely see any of those absued.

I think it's a very nice gradual step forward to reducing the page weight of error handling, even if it doesn't reduce the amount of ink on the page.

@jimmyfrasche
Copy link
Member

@zeebo that's a fair point about testing

@sschiz

This comment has been minimized.

@adg
Copy link
Contributor

adg commented Apr 1, 2020

I had actually explored this same idea myself, around the time when the other error proposal was being circulated. My conclusions/worries in respect to this specific proposal:

  1. The special form of if err { might make other error handling constructs that do not fit the exact if err != nil { form seem out of place. This may lead programmers to refactor such code such that they can write if err { instead, which seems counterproductive.

  2. Having the error handling on one line isn't desirable IMO. Within the context of a function a return statement does something profound: it terminates execution. For that reason I think a return statement deserves its own line—with indentation should the return only happen conditionally—as it does today.

  3. I think the consistency of having one way to check errors—arising naturally from the orthogonal concepts of expressions, if statements, blocks, and return statements—is a benefit higher than saving a few keystrokes and line breaks.

@alanfo
Copy link

alanfo commented Apr 1, 2020

Although I promised myself that I wouldn't bother commenting on any more error handling proposals, this one may be unique in that (as I post this) it hasn't yet been heavily voted down! So here FWIW are my two cents.

Firstly, several people (including myself) have long advocated that go fmt should permit single statement blocks to be written on the same line as the if clause itself, where those statements are just return, break, continue or goto statements. It's always seemed anomalous to me that you can write a function block on the same line as the function declaration itself but are not allowed to do this. I therefore welcome the return part of the proposal even if it's not going as far as I would personally like.

I realize, of course, that there are many people who won't agree with this but as it would presumably be an optional part of the proposal, they could carry on writing the return statement on a different line as they do now.

As far as the if part is concerned, I would support it if and only if an expression of type error could be implicitly converted to bool. I think if you also support conversion of other nilable types and/or declaration of temporary variables within the if clause, then the essential simplicity of the proposal is lost and you may even find that the result is too long to write on a single line anyway :)

@networkimprov
Copy link

I'm not in favor of implicit error-to-bool, but if bool(err) isn't helpful. And the lion's share of this proposal can already be achieved by an alternative to go fmt.

I'd be disappointed to see another error handling discussion engulf a lot of Go team time. We were told the topic had been shelved after the community voted overwhelmingly against making any change. That was a well-considered decision.

@acnologia000

This comment has been minimized.

@vtopc

This comment has been minimized.

@acnologia000

This comment has been minimized.

@creker
Copy link

creker commented Apr 7, 2020

I think both of these look even worse than if err != nil.

@vtopc

This comment has been minimized.

@acnologia000

This comment has been minimized.

@themeeman
Copy link

C++ defines explicit conversions to bool as being special in the language, as they can occur implicitly in certain contexts. For example, if x is explicitly convertible to bool, then if x will call that conversion. However, trying to pass it into a function that accepts a bool won't work, because its not implicit.

I believe a similar approach would work in Go. Allow the error type to be explicitly converted to bool using the conversion operator as in bool(err), but also allow the conversion to occur implicitly in certain contexts, specifically inside an if statement condition. It would have to allow the ! operator to convert it to a bool for consistency.

This creates a small set of rules that to me seem very Go-like, as they are simple and can't be leveraged and overloaded by the user; it's just a special case.

On the other hand, if we were to simply allow the specific syntax if err, this will inevitably lead to confusion, as users will wonder why other constructs like if !err, if err := f(); err and if err1 && err2 don't seem to work. This kind of unexpected behavior is what I think a language feature should avoid.

It is similar to PHP's (int) and (float) constructs, designed to look like cast operators from C like languages to be familiar to users. However, they are misleading because there are no int or float types in PHP - the casts are just special syntactic features. It is very much like this, because a user will assume some conversion to bool is happening, where none is at all.

@ianlancetaylor
Copy link
Contributor

Based on the discussion above, this does not seem to be a clear win. And the emoji voting is not strongly in favor. Therefore, this is a likely decline. Leaving open for four weeks for final comments.

@tooolbox
Copy link

Neat idea, but IMO it reduces clarity. Something like ? to do the conversion would at least let the reader know what's happening. if err? { return nil } (Probably already several proposals involving a question mark floating around...)

@blakewatters
Copy link

I am new to the Golang party and aware that there is a ton of past proposals that I have not read, but might as well wade into the arena:

This proposal is nice in that it 1) accepts the reality that errors are special and unavoidable and 2) makes a minimal change for ergonomics that while are admittedly special-cased, are of such commonality as to fade into the background once adopted. I think of the semantics for how make operates on specific types. Why are slices and channels sufficiently special so as to demand language affordances that errors (which are far more common) do not?

I think when combined with an affordance that enables the low ceremony return of underlying errors, we can address 80% of the pain without introducing new keywords, flow control, etc.

To highlight what I mean, consider the introduction of a new error specific sentinel value (and maybe this should be a parallel proposal but this is where I am right now) of ^ that wires in the semantics of return error if not nil -- very similar to how _ throws away a value you don't care about.

Consider something like:

func readSomething() (string, error) {
        file, ^ := os.OpenFile("some_file", os.O_TRUNC|os.O_CREATE|os.O_WRONLY, 0666)
	defer file.Close()
        result, ^ := doSomethingWithFile(file)
        output, err := processResult(result)
        if err {
             // Try to recover from the error and return
        }
        return output, nil
}

IMO boilerplate check and return of errors and coercion to boolean is the essence of the error handling ergonomic issues

@flibustenet
Copy link

@blakewatters i like this idea for simple apps. But most of the time i prefer to wrap the error to add context. I think this idea was already discussed in the try proposal, but maybe you should still make a new proposal ?

@golang golang deleted a comment May 14, 2020
@ianlancetaylor
Copy link
Contributor

No change in consensus.

@andig
Copy link
Contributor

andig commented Jun 11, 2020

@blakewatters I really like your idea for its simplicity and resemblance to _. You could go fancy and even add error wrapping:

file, fmt.Errorf("..", ^) := os.OpenFile("some_file", os.O_TRUNC, 0666)

which is a bit hard to read with a later-called function on the left hand side.
We could also do something similar to extended if statement syntax:

file, ^ := os.OpenFile("some_file", os.O_TRUNC, 0666); fmt.Errorf("..", ^) 

or, to make the return case explicit:

file, ^ := os.OpenFile("some_file", os.O_TRUNC, 0666); return fmt.Errorf("..", ^) 

The last two variants could be combined with non-zero values to the non-error returns, too:

file, ^ := os.OpenFile("some_file", os.O_TRUNC, 0666); return "foo", fmt.Errorf("..", ^) 

Different from this proposal this would allow to simplify a lot of error return statements to concise one-liners. Would it make sense to open yet another error proposal for this?

@ydnar
Copy link

ydnar commented Jun 11, 2020

The guard statement in Swift assigns variables in the outer scope, as syntactic sugar for an if statement:

guard let foo = bar() else {
    return “error”
}

// Use foo here

If Go adopted it:

guard foo, err := bar() else {
    return err
}

// Use foo here

@flibustenet
Copy link

@andig @blakewatters.
+1 for a proposal to discuss your two proposals. But i'm pretty sure we already had this kind of proposal, if somebody remember ?

@andig
Copy link
Contributor

andig commented Jun 12, 2020

But i'm pretty sure we already had this kind of proposal, if somebody remember ?

Will wait some more to not raise this twice. A connection of @ydnar‘s suggestion of try/guard statement that is able to elevate variables out of block scope (which was my biggest pain) allows for using syntax similar to the if statement plus introduce a short form that entirely avoids the multi-line if err checking. I wouldn‘t want to raise it and waste everybody‘s time though if this has been seen before- I couldn‘t find it in the error handling-tagged proposals.

@creker
Copy link

creker commented Jun 12, 2020

The main benefit of guard in Swift is not saving a few lines but optional binding. Other than that I always considered it as a pretty strange construct. It's an if statement and Go doesn't need two types of if statements. I'm sure the proposal would be quickly downvoted and declined.

As for extending an if statement, there were multiple such proposals and all of them were declined either because people don't want another special magic symbol to do all kinds of things (like ^ in the above example) or it doesn't address all the requirements (like allowing for errors to be augmented with some contextual information). The example above have the same problem many other such proposals had - it makes it difficult to see the error path. You have to scan whole line to get to it. It also makes lines much longer and will require you to split them in multiple lines possibly making code even harder to read. The benefit of current error handling is it's completely linear - you can scan the function from top to bottom only checking a few starting symbols on each line and get a glimpse on every return path.

@candlerb
Copy link

I don't support special-casing the error type. I think it would be a wart in the language: something extra that people have to learn to read other people's code, but not a general-purpose feature you can apply to your own types.

However, if the proposal were to special-case all interface values in this way, then I think it's more acceptable. That is: any interface variable can be used in a boolean context; a nil interface is treated as false, and any non-nil interface is treated as true (even if it contains a nil value, e.g. a nil pointer)

It could be used as a guard for a possibly-nil interface:

if v && v.Foo() { ... }

However, this may lead to requests for similar treatment of non-interface types.

Aside: I do sometimes miss Python's truthiness values, which each type can define using __bool__(): e.g. strings and lists implement them as len > 0. The equivalent in Go would be a booler interface with Bool() method. But it would still have to be defined in the language that a nil interface is false, since a nil interface has no concrete type to call a method on. Therefore, the starting point would be "nil interfaces are false and non-nil interfaces are true".

@networkimprov
Copy link

As this proposal is declined, could folks pls continue the discussion re error handling in a new proposal, or golang-nuts? Thanks!

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