[amluto]

Somewhat off topic, but I find a different part of this to be quite ugly:

    if match || err != nil {
        return rule, err
    }

Translating this code to actual logic takes too much thought and is too fragile. Is that an error path or a success path? It’s both! The logic is “if we found a rule or if there was an error then return a tuple that hopefully indicates the outcome”. If any further code were to be added in this block, it would have to be validated for the success and the error case.

But this only makes any sense at all if one is okay with reading Go result returns in their full generality. A canonical Go function returns either Success(value) or Error(err not nil, meaningless auxiliary value). And this code has “meaningless auxiliary value” != nil! In fact, it’s a pointer that likely escapes further into unrelated error handling code and thus complicates and kind of lifetime or escape analysis.

I don’t use Go, but if I did, I think this part of the language would be my biggest peeve. Go has very little explicit error handling; fine, that’s a reasonable design decision. But Go’s error handing is incorrectly typed, and that is IMO not a reasonable design.

[ericbarrett]

I write a lot of Go, and I agree that this is a big wart in its error handling that would be served by a proper Result type.

Nevertheless, the convention is that if a function returns (value, err), and err != nil, the value is discarded (I think of it as "undefined"). So the code is conventional.

[amluto]

In C, “discarding” a pointer in a way that leaves the value visible is quite common. At least if one doesn’t accidentally use the pointer, it’s harmless. (In the way that all manner of unsafeness is harmless in C as long as no actual UB occurs, which is to say it’s not great.)

But Go is a garbage collected language, and there is so such thing as “discarding” a pointer. Either it’s there or it isn’t, and this kind of leak has side effects. I find it baffling that the language designers and the community consider this acceptable.

(One thing I really like about Rust is that you can’t play fast and loose with lifetimes like this. If you have function taking &'a Vec<T> and you return &'a T, you can’t arbitrarily “discard” and leak that reference up the call chain. You need to genuinely get rid of it by the time the vector is gone.)

[foldr]

>and this kind of leak has side effects

Only if the calling function does something with the pointer (which it generally won't, if err is non-nil). If the calling code does do something with the pointer even when err is non-nil, then either

(a) the value of the pointer is meaningful, and this is fine; or

(b) there's a logic error in the calling code, which is a far more serious bug than a potential memory leak.

So I don't really see the problem here.

[amluto]

I should have been more explicit. Go is garbage collected. If you keep a pointer live, then it’s not garbage, which prevents it from being collected.

At least Go doesn’t use RAII, so inadvertently pinning a pointer won’t keep a socket open or a lock held.

[foldr]

Just returning the pointer doesn't keep the value that it points to live, unless the calling function does something with the pointer.

The following toy example doesn't leak memory. Are you thinking of some other pattern that would leak memory? If so, what is it? I can only think of patterns where a logic error is also involved. That is, where the meaningless value of 'val' is used in code paths where err != nil.

    func alwaysError() (*int, error) {
        var dummy int

        // We return &dummy even though it's a meaningless value.
        // Will this cause a memory leak?

        return &dummy, fmt.Errorf("An error")
    }

    func caller() {
        val, err := alwaysError()
        if err != nil {
            fmt.Printf("Error\n")

            // It won't! Because the value pointed to by 'val'
            // can be GCed from this point on.
            return
        }

        // never get here        

        fmt.Printf("Value %v\n", val)
        //
        // ...lots of code that uses *val...
        //
    }

You might think that you'd get a leak if the error branch was also long-lived, but Go's GC seems to be precise with respect to conditional branching (as far as I can tell by experimenting with runtime.SetFinalizer). That is, as long as the error branch doesn't refer to 'val', then the value pointed to by 'val' can be collected once the error branch is entered (and before 'caller' returns).

[amluto]

I haven't actually played with this, but if the same pattern of lazily (sloppily?) returning arbitrary pointers along with errors continues, the lifetime ought to be extended arbitrarily. The caller could itself return val, err, and so on up the chain.

[za3faran]

What I found frustrating that there is nothing stopping a function from returning both a non nil value and a non nil error. I've seen weird code that did that and it could have easily resulted in incorrect behavior.

[klodolph]

The one place where you’ll actually see this is in Read().

If you try to read 1000 bytes, you might get “800 bytes read, EOF” as a result. The worst part is that os.File won’t ever do this!

[ok_dad]

I really hate when people use "errors" for signals. Or, alternately, use the signals mechanisms for actual errors. An error should be "something went wrong", and reading a file to the EOF is not "going wrong", that's just what the "read()" should do if you tell it so!

I like Go's multiple returns and error checking by default, but it definitely should have been implemented with some sort of "Result/Error" type union instead. Go made so many good decisions, that I am frankly amazed they made this one really bad one.

[robotresearcher]

If you take the position that the semantics of read() are 'read data from this endless stream', then EOF is an error indicating that this model no longer applies.

Doesn't bother me at all.

[account42]

Except in that model it would be fine to return an error as soon as soon as you know there is an EOF while in the real world you do want to read right up to the EOF because EOF is usually not an error condition but expected, even with streams of indeterminate length.

[bouncycastle]

But that's what "errors" are, the're signals.

[nemo1618]

Presumably this is for performance reasons: if a `Read` call involves, say, a network request, then returning "800, EOF" in one roundtrip is certainly nicer than "800, nil" -> "0, EOF" in two roundtrips. In practice... I suspect this happens rarely enough that it's not worth the cost. The performance edge case could be handled with an alternative API, e.g. an optional EOF() method.

[slcjordan]

They should probably replace that with 2 if statements to make the error path and the non-error path obvious:

    if err != nil {
        return nil, err
    }
    if match {
        return rule, nil
    }

[foldr]

The regular return value doesn’t have to be meaningless just because the error is non-nil. In this case the function returns the rule that triggered the error, which is potentially useful information. I don’t think it is an official Go convention that err being non-nil entails that the other return value should be meaningless.

[za3faran]

It's quite error prone, most golang code is if err != nil { return nil, err }

Now of course it's important to read the documentation, but a language with sum types (or exceptions) would have used a separate type to indicate an error condition plus useful information on that error.

[foldr]

Right, but Go doesn't have sum types, and I think it's a mistake to interpret Go's error handling conventions through that lens. It's perfectly fine to return an error together with a meaningful value. At least, I haven't been able to find any official Go docs suggesting otherwise. You might wish that Go had sum types, but that doesn't mean that it's an actual Go coding convention to pretend that (err, X) tuples are sum types.

[adamwk]

That only works if a function is returning a pointer though, so I don't think it's right to say "most google code." Any function returning a string or any struct by value that can fail doesn't return nil