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: spec: require call results to be explicitly used or ignored (Go 2) #20803

Open
bcmills opened this Issue Jun 26, 2017 · 26 comments

Comments

Projects
None yet
@bcmills
Member

bcmills commented Jun 26, 2017

Today, if a Go function returns both a value and an error, the user must either assign the error to a variable

v, err := computeTheThing()

or explicitly ignore it

v, _ := computeTheThing()

However, errors that are the only return value (as from io.Closer.Close or proto.Unmarshal) or paired with an often-unneeded return value (as from io.Writer.Write) are easy to accidentally forget, and not obvious in the code when forgotten.

tx.Commit()  // Store the transaction in the database!

The same problem can occur even when errors are not involved, as in functional-style APIs:

t := time.Now()
t.Add(10 * time.Second)  // Should be t = t.Add(…)
strconv.AppendQuote(dst, "suffix")  // Should be dst = strconv.AppendQuote(…)

For the few cases where the user really does intend to ignore the return-values, it's easy to make that explicit in the code:

_, _ = fmt.Fprintln(&buf, v)  // Writes to a bytes.Buffer cannot fail.

go func() { _, _ = fmt.Fprintln(os.Stderr, findTheBug()) }()

And that transformation should be straightforward to apply to an existing code base (e.g. in a Go 1-to-2 conversion).

On the other hand, the consequences of a forgotten error-check or a dropped assignment can be quite severe (e.g., corrupted entries stored to a production database, silent failure to commit user data to long-term storage, crashes due to unvalidated user inputs).


Other modern languages with explicit error propagation avoid this problem by requiring (or allowing API authors to require) return-values to be used.

  • Swift warns about unused return values, but allows the warning to be suppressed if the @discardableResult attribute is set.
    • Prior to a change in Swift 3, return-values could be ignored by default (but a warning could be added with @warn_unused_result)
  • Rust has the #[must_use] attribute.
  • C++17 has the [[nodiscard]] attribute, which standardizes the longstanding __attribute__((warn_unused_result)) GNU extension.
  • The ghc Haskell compiler provides warning flags for unused results (-fwarn-unused-do-bind and -fwarn-wrong-do-bind).
  • OCaml warns about unused return values by default. It provides an ignore function in the standard library.

I believe that the OCaml approach in particular would mesh well with the existing design of the Go language.

I propose that Go should reject unused return-values by default.

If we do so, we may also want to consider an ignore built-in or keyword to ignore any number of values:

go func() { ignore(fmt.Fprintln(os.Stderr, findTheBug())) }()

or

go func() { ignore fmt.Fprintln(os.Stderr, findTheBug()) }()

or

go func() { _ fmt.Fprintln(os.Stderr, findTheBug()) }()

If we use a built-in function, we would probably want a corresponding vet check to avoid subtle eager-evaluation bugs:

go ignore(fmt.Fprintln(os.Stderr, findTheBug()))  // BUG: evaluated immediately

Related proposals
Extending vet checks for dropped errors: #19727, #20148
Making the language more strict about unused variables in general: #20802
Changing error-propagation: #19991

@gopherbot gopherbot added this to the Proposal milestone Jun 26, 2017

@gopherbot gopherbot added the Proposal label Jun 26, 2017

@ianlancetaylor ianlancetaylor added the Go2 label Jun 26, 2017

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 26, 2017

Contributor

A point of clarification: when you wrote

_ = fmt.Fprintln(&buf, v)

did you mean to write

_, _ = fmt.Fprintln(&buf, v)

or did you forget that fmt.Fprintln returns two values? That is, is your proposal intended to cover a function like fmt.Fprintln that returns two values? I can't quite tell.

Contributor

ianlancetaylor commented Jun 26, 2017

A point of clarification: when you wrote

_ = fmt.Fprintln(&buf, v)

did you mean to write

_, _ = fmt.Fprintln(&buf, v)

or did you forget that fmt.Fprintln returns two values? That is, is your proposal intended to cover a function like fmt.Fprintln that returns two values? I can't quite tell.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 26, 2017

Member

That was a typo on my part. Fixed, and added a suggestion for an ignore builtin.

Member

bcmills commented Jun 26, 2017

That was a typo on my part. Fixed, and added a suggestion for an ignore builtin.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Jun 26, 2017

Contributor

While I certainly sympathize with the concerns here, I just want to state that 1) I think that wrapping expressions in ignore makes code harder to read by burying the lede; 2) in a language that values simplicity having the canonical hello, world example start with _, _ = seems to me to be unfortunate.

That is, I'm on board with the problem, but not with the proposed solutions.

Contributor

ianlancetaylor commented Jun 26, 2017

While I certainly sympathize with the concerns here, I just want to state that 1) I think that wrapping expressions in ignore makes code harder to read by burying the lede; 2) in a language that values simplicity having the canonical hello, world example start with _, _ = seems to me to be unfortunate.

That is, I'm on board with the problem, but not with the proposed solutions.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 26, 2017

Member

ignore as a keyword rather than a builtin might address the lede-burying problem somewhat. IMO,

    ignore fmt.Println("Hello, world")

is a bit clearer than

    ignore(fmt.Println("Hello, world"))

But I agree that those are both valid concerns, and I'd be thrilled if we could find some way to address the underlying problems (missed values in general, and missed errors in particular) that resolves those concerns.

Member

bcmills commented Jun 26, 2017

ignore as a keyword rather than a builtin might address the lede-burying problem somewhat. IMO,

    ignore fmt.Println("Hello, world")

is a bit clearer than

    ignore(fmt.Println("Hello, world"))

But I agree that those are both valid concerns, and I'd be thrilled if we could find some way to address the underlying problems (missed values in general, and missed errors in particular) that resolves those concerns.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 26, 2017

Member

I should note a point that I forgot to make in the original post (now edited to add): this isn't just a problem for errors, but also for functional-style APIs such as time.Time.Add or strconv.Append*. The missed return value is sometimes the only clue the reader has as to whether an API is functional or mutative.

Member

bcmills commented Jun 26, 2017

I should note a point that I forgot to make in the original post (now edited to add): this isn't just a problem for errors, but also for functional-style APIs such as time.Time.Add or strconv.Append*. The missed return value is sometimes the only clue the reader has as to whether an API is functional or mutative.

@pborman

This comment has been minimized.

Show comment
Hide comment
@pborman

pborman Jun 29, 2017

Contributor

I know this has been discussed before, multiple times. The concern usually is: OMG, they did defer fd.Close() and there might have been an error. If fd is read only then the close error provides no useful information. Either you read the data you wanted or you didn't. Any meaningful errors are captured during read.

go ignore fd.Close()
ignore go fd.Close()

What about maps? Is it okay to ignore the bool, or would I have to always add the , _ to the left hand side. If that is the case then you can't write if myBoolMap[x] { anymore, it becomes if ok, _ := myBoolMap[x]; ok {.

Just how many programs would this proposal break? This seems like a Go 2.0 feature as it breaks the compatibility guarantee. I guess you could test that by writing a go vet function to do this and then run it first over the standard library, and then over random libraries from github or over the Go code internal at your company.

Contributor

pborman commented Jun 29, 2017

I know this has been discussed before, multiple times. The concern usually is: OMG, they did defer fd.Close() and there might have been an error. If fd is read only then the close error provides no useful information. Either you read the data you wanted or you didn't. Any meaningful errors are captured during read.

go ignore fd.Close()
ignore go fd.Close()

What about maps? Is it okay to ignore the bool, or would I have to always add the , _ to the left hand side. If that is the case then you can't write if myBoolMap[x] { anymore, it becomes if ok, _ := myBoolMap[x]; ok {.

Just how many programs would this proposal break? This seems like a Go 2.0 feature as it breaks the compatibility guarantee. I guess you could test that by writing a go vet function to do this and then run it first over the standard library, and then over random libraries from github or over the Go code internal at your company.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 29, 2017

Member

What about maps?

Map lookups would still support both the one-value and two-value forms, but ignoring the result of a map lookup entirely would (continue to) be an error.

These are ok.

if m[x] {
if _, ok := m[x]; ok {
if y := m[x]; y {
if y, _ := m[x]; y {  // Verbose, but ok.

This one is not ok, because the result is ignored entirely. The compiler already rejects it today (https://play.golang.org/p/0WMEDVzY44).

m[x]
Member

bcmills commented Jun 29, 2017

What about maps?

Map lookups would still support both the one-value and two-value forms, but ignoring the result of a map lookup entirely would (continue to) be an error.

These are ok.

if m[x] {
if _, ok := m[x]; ok {
if y := m[x]; y {
if y, _ := m[x]; y {  // Verbose, but ok.

This one is not ok, because the result is ignored entirely. The compiler already rejects it today (https://play.golang.org/p/0WMEDVzY44).

m[x]
@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 29, 2017

Member

Just how many programs would this proposal break?

Most of them. It is labeled "Go2" for good reason. :)

Member

bcmills commented Jun 29, 2017

Just how many programs would this proposal break?

Most of them. It is labeled "Go2" for good reason. :)

@robpike

This comment has been minimized.

Show comment
Hide comment
@robpike

robpike Jun 29, 2017

Contributor

Having lived through the

(void)printf("hello, world\n");

era, I firmly believe some heuristics in vet are a better way to approach this problem. That's what #20148 is about.

Like Ian, I appreciate the concern but dislike the proposed remedy.

Contributor

robpike commented Jun 29, 2017

Having lived through the

(void)printf("hello, world\n");

era, I firmly believe some heuristics in vet are a better way to approach this problem. That's what #20148 is about.

Like Ian, I appreciate the concern but dislike the proposed remedy.

@nerdatmath

This comment has been minimized.

Show comment
Hide comment
@nerdatmath

nerdatmath Jun 30, 2017

Contributor

ISTM we don't really want to ignore errors; I think in most of these cases we really want to panic in case of error, because the programmer believes an error is not possible, or that a meaningful response to an error doesn't exist. So it seems like we want a generic Must. This might seem weird, but what if the exclamation point (!) could be used as a postfix operator to conditionally panic. If the value is a bool, panic if it's not true, and if it's an error, panic if it's not nil. It could also be used with multiple return values, only looking at the last value. Since it's postfix it avoids burying the lede, but indicates the programmer understands they're ignoring an error value. (!) should always consume one or more values but never return anything, and should fail to compile if the type of the last value passed in is not bool or error.

func main() {
    fmt.Println("Hello, world!")!
}

I guess we would also want go f(x)! and defer f(x)! to be equivalent to go func(z T) { f(z)! }(x) and defer func(z T) { f(z)! }(x).

Contributor

nerdatmath commented Jun 30, 2017

ISTM we don't really want to ignore errors; I think in most of these cases we really want to panic in case of error, because the programmer believes an error is not possible, or that a meaningful response to an error doesn't exist. So it seems like we want a generic Must. This might seem weird, but what if the exclamation point (!) could be used as a postfix operator to conditionally panic. If the value is a bool, panic if it's not true, and if it's an error, panic if it's not nil. It could also be used with multiple return values, only looking at the last value. Since it's postfix it avoids burying the lede, but indicates the programmer understands they're ignoring an error value. (!) should always consume one or more values but never return anything, and should fail to compile if the type of the last value passed in is not bool or error.

func main() {
    fmt.Println("Hello, world!")!
}

I guess we would also want go f(x)! and defer f(x)! to be equivalent to go func(z T) { f(z)! }(x) and defer func(z T) { f(z)! }(x).

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jun 30, 2017

Member

So, what are the cases where it really is appropriate to ignore all of the return values of a function?

These are always appropriate to ignore:

  • Write methods on a bytes.Buffer or hash.Hash (which always return len(p), nil).
  • (ioutil.NopCloser).Close (but in that case why use the NopCloser at all?).
  • (expvar.Map).Init (which returns the receiver).

These are conditionally appropriate:

  • Close on an io.ReadCloser implementation (if it is not also writable).
  • (io.Closer).Close, (exec.Cmd).Wait, and builtin.recover (if the current function is already returning some other error).
  • Transitive writes to a bytes.Buffer (if the bytes.Buffer detail is obvious enough to avoid bugs if the code is refactored).
  • Transitive writes to os.Stdout or os.Stderr, including the fmt.Print functions (if the program is small enough and has few enough dependencies that SIGPIPE is obviously not ignored, and/or the program will obviously exit soon).
  • atomic.Add functions (if overflow is benign or impossible).
  • math/big operator methods (if the receiver is used elsewhere).
  • Container insertions (if the caller is storing an element for later or doesn't care whether it is stored).
    • (*ring.Rink).Link
    • (*list.List).{InsertBefore,InsertAfter}
    • (*sync.Map).LoadOrStore
    • (reflect.Value).TrySend
  • Container deletions (if the caller already has the element or is no longer interested in it)
    • heap.Remove
    • (*ring.Ring).Unlink
    • (*list.List).Remove
  • time.AfterFunc (if the caller will never cancel the operation).
  • runtime.GOMAXPROCS and runtime.SetMutexProfileFraction (if the argument is nonzero and the previous value will not be restored).
  • Slice operations:
    • copy and reflect.Copy (if the number of bytes to be copied is already known)
    • append (if the argument is known to fit in the slice)
  • (reflect.Value).Call and (reflect.Value).CallSlice (if the function is known to have no return-values, or known to be one of the above cases)

Unclear to me:

  • (sql.Tx).Rollback
Member

bcmills commented Jun 30, 2017

So, what are the cases where it really is appropriate to ignore all of the return values of a function?

These are always appropriate to ignore:

  • Write methods on a bytes.Buffer or hash.Hash (which always return len(p), nil).
  • (ioutil.NopCloser).Close (but in that case why use the NopCloser at all?).
  • (expvar.Map).Init (which returns the receiver).

These are conditionally appropriate:

  • Close on an io.ReadCloser implementation (if it is not also writable).
  • (io.Closer).Close, (exec.Cmd).Wait, and builtin.recover (if the current function is already returning some other error).
  • Transitive writes to a bytes.Buffer (if the bytes.Buffer detail is obvious enough to avoid bugs if the code is refactored).
  • Transitive writes to os.Stdout or os.Stderr, including the fmt.Print functions (if the program is small enough and has few enough dependencies that SIGPIPE is obviously not ignored, and/or the program will obviously exit soon).
  • atomic.Add functions (if overflow is benign or impossible).
  • math/big operator methods (if the receiver is used elsewhere).
  • Container insertions (if the caller is storing an element for later or doesn't care whether it is stored).
    • (*ring.Rink).Link
    • (*list.List).{InsertBefore,InsertAfter}
    • (*sync.Map).LoadOrStore
    • (reflect.Value).TrySend
  • Container deletions (if the caller already has the element or is no longer interested in it)
    • heap.Remove
    • (*ring.Ring).Unlink
    • (*list.List).Remove
  • time.AfterFunc (if the caller will never cancel the operation).
  • runtime.GOMAXPROCS and runtime.SetMutexProfileFraction (if the argument is nonzero and the previous value will not be restored).
  • Slice operations:
    • copy and reflect.Copy (if the number of bytes to be copied is already known)
    • append (if the argument is known to fit in the slice)
  • (reflect.Value).Call and (reflect.Value).CallSlice (if the function is known to have no return-values, or known to be one of the above cases)

Unclear to me:

  • (sql.Tx).Rollback
@rodcorsi

This comment has been minimized.

Show comment
Hide comment
@rodcorsi

rodcorsi Jul 1, 2017

As you say, we have few cases where it is appropriate to ignore the return value, and we can do this clearly with an attribute.

func Fprintln(w io.Writer, a ...interface{}) (n int, err error) optional

rodcorsi commented Jul 1, 2017

As you say, we have few cases where it is appropriate to ignore the return value, and we can do this clearly with an attribute.

func Fprintln(w io.Writer, a ...interface{}) (n int, err error) optional
@AlekSi

This comment has been minimized.

Show comment
Hide comment
@AlekSi

AlekSi Jul 5, 2017

Contributor

Personally, I think that linters like https://github.com/kisielk/errcheck solve the problem quite nicely.

Contributor

AlekSi commented Jul 5, 2017

Personally, I think that linters like https://github.com/kisielk/errcheck solve the problem quite nicely.

@romainmenke

This comment has been minimized.

Show comment
Hide comment
@romainmenke

romainmenke Jul 5, 2017

IMHO this is not a problem with the language spec and can be remedied by linters.

One of the reasons I stopped writing in Swift is because they kept introducing breaking changes with things like @discardableResult. It didn't fix a problem with the language but broke a lot of things. First I gave up on using third party libs (because most don't have cross version support) then I gave up on Swift entirely.

Please consider the awesome go community before changing something as fundamental as function returns.

romainmenke commented Jul 5, 2017

IMHO this is not a problem with the language spec and can be remedied by linters.

One of the reasons I stopped writing in Swift is because they kept introducing breaking changes with things like @discardableResult. It didn't fix a problem with the language but broke a lot of things. First I gave up on using third party libs (because most don't have cross version support) then I gave up on Swift entirely.

Please consider the awesome go community before changing something as fundamental as function returns.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jul 5, 2017

Member

@nerdatmath

I think in most of these cases we really want to panic in case of error, because the programmer believes an error is not possible

A generic "must" operator would not help for forgotten return-values from calls that do not return errors (such as (time.Time).Add). Furthermore, in many cases we really do intend to ignore the error, not not panic if it occurs: for example, if we are already returning a non-nil error from writing a file, we typically do not care whether its Close method returns an error.

Member

bcmills commented Jul 5, 2017

@nerdatmath

I think in most of these cases we really want to panic in case of error, because the programmer believes an error is not possible

A generic "must" operator would not help for forgotten return-values from calls that do not return errors (such as (time.Time).Add). Furthermore, in many cases we really do intend to ignore the error, not not panic if it occurs: for example, if we are already returning a non-nil error from writing a file, we typically do not care whether its Close method returns an error.

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jul 6, 2017

Member

Heuristic tools are for the author of the code, not the reader. I want to address both of those users, not just the author.

If there is a consistent rule in the language itself, then the reader can easily see whether a return-value has been discarded, and can know that the author intentionally chose that behavior.

With a heuristic tool, the reader can't distinguish between several possibilities:

  1. The call in question does not return a value.
  2. The call returns a value, but the tool heuristically decided it was probably not relevant.
    a. …and the heuristic was correct.
    b. …and the heuristic was incorrect, and the author didn't notice.
  3. The author of the code ran the tool, but intentionally ignored its warning.
  4. The author of the code didn't run the tool.

Because the tool is not part of the language, its use discards important information which can only be recovered by ad-hoc documentation, and that documentation is (in my opinion) even more unpleasant than a leading _, _ = or ignore:

fmt.Fprintln(w, stuff) // Ignore errors: w always wraps a bytes.Buffer.
Member

bcmills commented Jul 6, 2017

Heuristic tools are for the author of the code, not the reader. I want to address both of those users, not just the author.

If there is a consistent rule in the language itself, then the reader can easily see whether a return-value has been discarded, and can know that the author intentionally chose that behavior.

With a heuristic tool, the reader can't distinguish between several possibilities:

  1. The call in question does not return a value.
  2. The call returns a value, but the tool heuristically decided it was probably not relevant.
    a. …and the heuristic was correct.
    b. …and the heuristic was incorrect, and the author didn't notice.
  3. The author of the code ran the tool, but intentionally ignored its warning.
  4. The author of the code didn't run the tool.

Because the tool is not part of the language, its use discards important information which can only be recovered by ad-hoc documentation, and that documentation is (in my opinion) even more unpleasant than a leading _, _ = or ignore:

fmt.Fprintln(w, stuff) // Ignore errors: w always wraps a bytes.Buffer.
@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jul 6, 2017

Member

The other problem with heuristic tools is that they are unreliable for authors.

If I run lint or vet on my code as I write it, I can't distinguish between:

  1. The call in question does not return a relevant value.
  2. The call returns a relevant value, but the tool's heuristics indicated that it was not relevant (and my tests didn't catch it).

We could bias toward over-reporting in order to reduce the occurrence of case (2), but that would make it much more likely that authors would simply turn off the check, further decreasing the confidence of readers of the code. We could provide a mechanism to suppress false-positives, but if we're going to do that, why not use it uniformly in the first place?

Given that most of the ignorable return values I could find (in #20803 (comment)) are only conditionally ignorable, I'm skeptical that we can find a set of heuristics that are actually reliable in practice.

Member

bcmills commented Jul 6, 2017

The other problem with heuristic tools is that they are unreliable for authors.

If I run lint or vet on my code as I write it, I can't distinguish between:

  1. The call in question does not return a relevant value.
  2. The call returns a relevant value, but the tool's heuristics indicated that it was not relevant (and my tests didn't catch it).

We could bias toward over-reporting in order to reduce the occurrence of case (2), but that would make it much more likely that authors would simply turn off the check, further decreasing the confidence of readers of the code. We could provide a mechanism to suppress false-positives, but if we're going to do that, why not use it uniformly in the first place?

Given that most of the ignorable return values I could find (in #20803 (comment)) are only conditionally ignorable, I'm skeptical that we can find a set of heuristics that are actually reliable in practice.

@jba

This comment has been minimized.

Show comment
Hide comment
@jba

jba Jul 6, 2017

for example, if we are already returning a non-nil error from writing a file, we typically do not care whether its Close method returns an error.

I think that's a bad example: you probably want to check both, in case of delayed writes. (But I agree with your main point.)

jba commented Jul 6, 2017

for example, if we are already returning a non-nil error from writing a file, we typically do not care whether its Close method returns an error.

I think that's a bad example: you probably want to check both, in case of delayed writes. (But I agree with your main point.)

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Jul 6, 2017

Member

This is a really gross idea, but another option:

With type aliases, the author of a package could add

type ignorableError = error //or irrelevantError, unusedError, etc.

and return it when it is always safe to ignore the return error like:

func (T) Close() ignorableError {  //just need to satisfy the interface
  return nil
}

Since it's a type alias it only changes the spelling not the meaning.

Linters could be aware of the idiom and whitelist anything that returned an alias spelled ignorableError to cut down on the noise.

Readers aware of the idiom would know it's safe to ignore from the signature.

It could be promoted from idiom to standard by making it a predeclared identifier.

Of course if you change something so that the error is no longer ignorable and forget to update the signature . . .

Member

jimmyfrasche commented Jul 6, 2017

This is a really gross idea, but another option:

With type aliases, the author of a package could add

type ignorableError = error //or irrelevantError, unusedError, etc.

and return it when it is always safe to ignore the return error like:

func (T) Close() ignorableError {  //just need to satisfy the interface
  return nil
}

Since it's a type alias it only changes the spelling not the meaning.

Linters could be aware of the idiom and whitelist anything that returned an alias spelled ignorableError to cut down on the noise.

Readers aware of the idiom would know it's safe to ignore from the signature.

It could be promoted from idiom to standard by making it a predeclared identifier.

Of course if you change something so that the error is no longer ignorable and forget to update the signature . . .

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Jul 6, 2017

Member

@jimmyfrasche I admire your creativity, but that wouldn't address non-error return types, and as far as I can tell wouldn't help the reader much for conditionally-ignorable return values.

Member

bcmills commented Jul 6, 2017

@jimmyfrasche I admire your creativity, but that wouldn't address non-error return types, and as far as I can tell wouldn't help the reader much for conditionally-ignorable return values.

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Aug 6, 2017

Member

@bcmills I was thinking about this again today and I realized that a less gross variant of my last post would be a convention to use the blank identifier to name always-ignorable return params, like:

func (b *BytesBuffer) Write(p []byte) (n int, _ error) {
  //code omitted
  return n, nil
}

or

func (Something) Close() (_ error) {
  return nil
}

It's

  • perfectly legal today
  • easy to lint †
  • easy to update existing code
  • mirrors the convention of explicitly ignoring a return _ = f() recognized by existing linters
  • covers all return parameters

† it's also easy to lint the case of you used to have an ignorable error but then you made a change and forgot to rename the return param.

It wouldn't help a reader unfamiliar with the convention, but it's easy enough to pick up, especially if it's followed by the stdlib.

I'm not sure there's a way to help with conditionally-ignorable errors. Really if you can only ignore an error in some conditions you:

  • need to be absolutely sure you meet those conditions and use something like _ = f() to express your intent at the call site (comment appreciated, of course)
  • need a linter that can abstractly interpret the code and prove that error is always non-nil
  • hedge your bets and deal with the error at the call site even if you're pretty sure that the path is never hit
Member

jimmyfrasche commented Aug 6, 2017

@bcmills I was thinking about this again today and I realized that a less gross variant of my last post would be a convention to use the blank identifier to name always-ignorable return params, like:

func (b *BytesBuffer) Write(p []byte) (n int, _ error) {
  //code omitted
  return n, nil
}

or

func (Something) Close() (_ error) {
  return nil
}

It's

  • perfectly legal today
  • easy to lint †
  • easy to update existing code
  • mirrors the convention of explicitly ignoring a return _ = f() recognized by existing linters
  • covers all return parameters

† it's also easy to lint the case of you used to have an ignorable error but then you made a change and forgot to rename the return param.

It wouldn't help a reader unfamiliar with the convention, but it's easy enough to pick up, especially if it's followed by the stdlib.

I'm not sure there's a way to help with conditionally-ignorable errors. Really if you can only ignore an error in some conditions you:

  • need to be absolutely sure you meet those conditions and use something like _ = f() to express your intent at the call site (comment appreciated, of course)
  • need a linter that can abstractly interpret the code and prove that error is always non-nil
  • hedge your bets and deal with the error at the call site even if you're pretty sure that the path is never hit
@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Aug 7, 2017

Member

@jimmyfrasche

I'm not sure there's a way to help with conditionally-ignorable errors.

Given the list in #20803 (comment), I think those are the only ones that really matter. Even the canonical example of an intentionally-ignored error, fmt.Printf, is only safe to ignore if the program is small and self-contained. (The special cases for bytes.Buffer and hash.Hash are easy enough to encode in a linting tool either way.)

Member

bcmills commented Aug 7, 2017

@jimmyfrasche

I'm not sure there's a way to help with conditionally-ignorable errors.

Given the list in #20803 (comment), I think those are the only ones that really matter. Even the canonical example of an intentionally-ignored error, fmt.Printf, is only safe to ignore if the program is small and self-contained. (The special cases for bytes.Buffer and hash.Hash are easy enough to encode in a linting tool either way.)

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Aug 7, 2017

Member

@bcmills Special cases for bytes.Buffer in the standard library are easy enough to special case in a linting tool but that doesn't help with the problem of making it clear to someone looking at the docs that it can always be ignored or the problem of discovering such funcs outside the standard library. A convention here would be useful to people and linters.

Let's say there was a way to document that you can sometimes ignore a result, for example an error, however. What does that tell you? That you can sometimes ignore the error. That's not very helpful unless you know when you can ignore the error. Short of a machine-checkable proof that a very sophisticated linter can use to analyze your program and verify that you can indeed safely ignore the error, the only recourse I see is to document the function with the cases when it is safe to ignore the error (and hope that if they change the docs get updated). A less sophisticated linter could see an annotation that a result is optional and that you didn't use it and assume you know what you're doing and not say anything, but that doesn't seem especially helpful.

If there were a way to annotate that you must use a result, it's easy to detect when you didn't. But that's most things so it could infect the majority of func sigs. (Though coupled with the _ convention for always ignorables it would give you the set of optionally ignorables as a by-product).

For the sake of argument say that 90% of results must be checked, 9% must be checked in certain scenarios, and 1% never need to be checked. Let's say we go with annotating the optionally and always ignorables because there are fewer of them (10% vs 90%). Then we can assume that anything that is not annotated must be checked. Then

  • it's always safe to ignore a result that's been marked always ignorable
  • it's never safe to ignore a result that is not annotated
  • it's still not clear whether it's safe to ignore a result marked optionally ignorable so a linter is going to have to say something about it.

The only gain I see is annotating the always ignorables since it at least can safely remove some noise from linter output. The _ = f() notation let's you be explicit at the call site as a signal to readers and linters alike that you're ignoring a result.

Member

jimmyfrasche commented Aug 7, 2017

@bcmills Special cases for bytes.Buffer in the standard library are easy enough to special case in a linting tool but that doesn't help with the problem of making it clear to someone looking at the docs that it can always be ignored or the problem of discovering such funcs outside the standard library. A convention here would be useful to people and linters.

Let's say there was a way to document that you can sometimes ignore a result, for example an error, however. What does that tell you? That you can sometimes ignore the error. That's not very helpful unless you know when you can ignore the error. Short of a machine-checkable proof that a very sophisticated linter can use to analyze your program and verify that you can indeed safely ignore the error, the only recourse I see is to document the function with the cases when it is safe to ignore the error (and hope that if they change the docs get updated). A less sophisticated linter could see an annotation that a result is optional and that you didn't use it and assume you know what you're doing and not say anything, but that doesn't seem especially helpful.

If there were a way to annotate that you must use a result, it's easy to detect when you didn't. But that's most things so it could infect the majority of func sigs. (Though coupled with the _ convention for always ignorables it would give you the set of optionally ignorables as a by-product).

For the sake of argument say that 90% of results must be checked, 9% must be checked in certain scenarios, and 1% never need to be checked. Let's say we go with annotating the optionally and always ignorables because there are fewer of them (10% vs 90%). Then we can assume that anything that is not annotated must be checked. Then

  • it's always safe to ignore a result that's been marked always ignorable
  • it's never safe to ignore a result that is not annotated
  • it's still not clear whether it's safe to ignore a result marked optionally ignorable so a linter is going to have to say something about it.

The only gain I see is annotating the always ignorables since it at least can safely remove some noise from linter output. The _ = f() notation let's you be explicit at the call site as a signal to readers and linters alike that you're ignoring a result.

@gladkikhartem

This comment has been minimized.

Show comment
Hide comment
@gladkikhartem

gladkikhartem Dec 25, 2017

@bcmills

I think there should be no ignorable errors, because they are really confusing from a standpoint of a plain logic.
If you could ignore it - why does the function is returning it in a first place!? If It's something to conform to an interface - you should not ignore an error, because you never know what could be behind that interface in a future. Functions should adapt to strict error handling, rather than error handling should adapt to 1% of functions that return "ignorable" errors.

I think simple handling ALL return parameters is enough to eliminate typos. It's a balance between cognitive load and being error-prone. And if person explicitly ignores error - it's a mental problem, rather than the problem of exception handling.

Not even mentioning the fact that if you introduce new stuff to Go - script-kiddies would use it everywhere.

gladkikhartem commented Dec 25, 2017

@bcmills

I think there should be no ignorable errors, because they are really confusing from a standpoint of a plain logic.
If you could ignore it - why does the function is returning it in a first place!? If It's something to conform to an interface - you should not ignore an error, because you never know what could be behind that interface in a future. Functions should adapt to strict error handling, rather than error handling should adapt to 1% of functions that return "ignorable" errors.

I think simple handling ALL return parameters is enough to eliminate typos. It's a balance between cognitive load and being error-prone. And if person explicitly ignores error - it's a mental problem, rather than the problem of exception handling.

Not even mentioning the fact that if you introduce new stuff to Go - script-kiddies would use it everywhere.

@jba

This comment has been minimized.

Show comment
Hide comment
@jba

jba Jan 4, 2018

If It's something to conform to an interface - you should not ignore an error, because you never know what could be behind that interface in a future.

If you want to talk logic, there is a logical flaw in that argument. You are conflating implementing a method in order to conform to an interface, with invoking that method through an interface type. For example, I will never need to check the error from Write in

var buf bytes.Buffer
buf.Write(...)

jba commented Jan 4, 2018

If It's something to conform to an interface - you should not ignore an error, because you never know what could be behind that interface in a future.

If you want to talk logic, there is a logical flaw in that argument. You are conflating implementing a method in order to conform to an interface, with invoking that method through an interface type. For example, I will never need to check the error from Write in

var buf bytes.Buffer
buf.Write(...)

@rsc rsc changed the title from proposal: spec: require return-values to be explicitly used or ignored (Go 2) to proposal: spec: require call results to be explicitly used or ignored (Go 2) Aug 26, 2018

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills
Member

bcmills commented Aug 27, 2018

CC: @dgryski

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment