Go compiler changes code behaviour with optimization enabled

[Ge]

As follows showing,  when `-N` is enabled with compiler, it gives the expected output.

```

➜  grd more nf2.go
package main

import "os"

func main() {
        f, err := os.Open("nonExistFile")
        fname := f.Name()
        if err != nil {
                println("failed to open file, Error:", err.Error())
                return
        }

        println("filename:", fname)
}
➜  grd go run nf2.go
failed to open file, Error: open nonExistFile: no such file or directory

➜  grd go run -gcflags="-N" nf2.go
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x4782e3]

goroutine 1 [running]:
os.(*File).Name(...)
        /usr/local/go/src/os/file.go:62
main.main()
        /root/grd/nf2.go:7 +0x123
exit status 2

➜  grd go version
go version go1.24.4 linux/amd64

```

[Ian Lance Taylor]

This may be https://go.dev/issue/72860. In any case, it works
correctly (that is, it panics) on HEAD and the upcoming Go 1.25
release.

Ian

[Dominik Honnef (dominikh)]

git bisect suggests that this behavior was introduced by the following commit:

commit 962ccbef91057f91518443b648e02fc3afe8c764 (HEAD)
Author: Keith Randall <k...@golang.org>
Date:   Wed Oct 25 13:35:13 2023 -0700

    cmd/compile: ensure pointer arithmetic happens after the nil check
   
    Have nil checks return a pointer that is known non-nil. Users of
    that pointer can use the result, ensuring that they are ordered
    after the nil check itself.
   
    The order dependence goes away after scheduling, when we've fixed
    an order. At that point we move uses back to the original pointer
    so it doesn't change regalloc any.
   
    This prevents pointer arithmetic on nil from being spilled to the
    stack and then observed by a stack scan.
   
    Fixes #63657
   
    Change-Id: I1a5fa4f2e6d9000d672792b4f90dfc1b7b67f6ea
    Reviewed-on: https://go-review.googlesource.com/c/go/+/537775
    Reviewed-by: David Chase <drc...@google.com>
    LUCI-TryBot-Result: Go LUCI <golang...@luci-project-accounts.iam.gserviceaccount.com>
    Reviewed-by: Keith Randall <k...@google.com>

[Dominik Honnef (dominikh)]

Memory beats git bisect, but this was indeed fixed by a1ddbdd3ef8b739aab53f20d6ed0a61c3474cf12.

[Ge]

Thanks Ian and Dominik for the tips. I have checked the issue 72860 and found randall77 posted a comment that

I couldn't confirm from golang spec(go.dev/ref/spec):

//https://github.com/golang/go/issues/72860#issuecomment-2734549770

```

Where f returns a pointer and an error. f expects its callers to only use the ptr result if the error is nil. And in particular, f returns a nil pointer whenever it returns a non-nil error.

This code is incorrect, in that it dereferences the ptr result before checking the error. The Go spec says that it should nil pointer panic if f returns a nil pointer and non-nil error. 

```

I did a search in whole spec and did't find any descriptions about it.

From my understanding, `v, err := f()` is like `v1, v2 := f()` from compiler's view, 

checking error is user code's business logic and couldn't be assumed by compiler.

If it's defined in spec, what about variants like `a, b, err := f()` or `a, b, err1, err2 := f()`？

Ge

[Ian Lance Taylor]

On Sat, Jul 5, 2025 at 9:19 PM Ge <everg...@gmail.com> wrote:
>
> Thanks Ian and Dominik for the tips. I have checked the issue 72860 and found randall77 posted a comment that
> I couldn't confirm from golang spec(go.dev/ref/spec):
>
> //https://github.com/golang/go/issues/72860#issuecomment-2734549770
> ```
>
> Where f returns a pointer and an error. f expects its callers to only use the ptr result if the error is nil. And in particular, f returns a nil pointer whenever it returns a non-nil error.
>
> This code is incorrect, in that it dereferences the ptr result before checking the error. The Go spec says that it should nil pointer panic if f returns a nil pointer and non-nil error.
>
> ```
>
> I did a search in whole spec and did't find any descriptions about it.
> From my understanding, `v, err := f()` is like `v1, v2 := f()` from compiler's view,
> checking error is user code's business logic and couldn't be assumed by compiler.
> If it's defined in spec, what about variants like `a, b, err := f()` or `a, b, err1, err2 := f()`？

It's true that for code like

f, err := os.Open("nonexist")

it's just a convention that if err != nil then f == nil. For example,
Google's Go style guide:
https://google.github.io/styleguide/go/decisions#returning-errors .

os.Open follows that convention.

Ian

[Kurtis Rader]

On Sat, Jul 5, 2025 at 9:19 PM Ge <everg...@gmail.com> wrote:

Thanks Ian and Dominik for the tips. I have checked the issue 72860 and found randall77 posted a comment that

I couldn't confirm from golang spec(go.dev/ref/spec):

//https://github.com/golang/go/issues/72860#issuecomment-2734549770

```

Where f returns a pointer and an error. f expects its callers to only use the ptr result if the error is nil. And in particular, f returns a nil pointer whenever it returns a non-nil error.

This code is incorrect, in that it dereferences the ptr result before checking the error. The Go spec says that it should nil pointer panic if f returns a nil pointer and non-nil error. 

```

I did a search in whole spec and did't find any descriptions about it.

From my understanding, `v, err := f()` is like `v1, v2 := f()` from compiler's view, 

checking error is user code's business logic and couldn't be assumed by compiler.

If it's defined in spec, what about variants like `a, b, err := f()` or `a, b, err1, err2 := f()`？

No, it is not defined in the spec. For the very reasons you mention (and probably others). But I would argue that commonsense suggests that unless a function explicitly says a returned non-error pointer is useful (i.e., non-nil) when an error is returned you should assume that pointer is likely to be nil if an error is indicated. Ideally this would be explicit in the documentation of every function that returns an error and non-error pointer. However, in the absence of the documentation stating that the non-error pointer value is useful if an error is indicated it seems reasonable to assume that pointer is not useful; even if it isn't nil. Even a non-pointer return value is unlikely to be meaningful if an error was indicated unless explicitly documented. If the called function returns the "zero value" when it also returns an error indicator what does it mean to ignore the error and use the "zero value"? That's obviously context dependent but it is unlikely, in my experience, to ignore the error and use the returned "zero value".

 

--

Kurtis Rader

Caretaker of the exceptional canines Junior and Hank

[Brian Candler]

The main example which springs to mind is io.Reader.Read which may return a non-zero number of read bytes *and* and error from the same call. Because this is so unusual, it is carefully emphasised in the docs:

Callers should always process the n > 0 bytes returned before considering the error err. Doing so correctly handles I/O errors that happen after reading some bytes and also both of the allowed EOF behaviors.

Anyway: the point here is that if a function returns (ptr, err) then in the case err = nil it will conventionally guarantee that ptr is valid, so the caller doesn't need to check ptr != nil before dereferencing it. But if err != nil, no such guarantee is implied, and it's highly unlikely that you could safely deference ptr. That convention is not in the spec, but dereferencing a nil pointer causing a panic is:

the pointer indirection *x denotes the variable of type T pointed to by x. If x is nil, an attempt to evaluate *x will cause a run-time panic.

[Keith Randall]

See the second paragraph in the compiler section of the go1.25 release notes: https://tip.golang.org/doc/go1.25#compiler
The example looks strangely familiar...

[Robert Engels]

This change seems wrong just based on the io.Reader returns of valid bytes and an error. An error doesn’t always mean the other return values are invalid - it is decided by the api method. 

On Jul 7, 2025, at 10:43 AM, 'Keith Randall' via golang-nuts <golan...@googlegroups.com> wrote:

See the second paragraph in the compiler section of the go1.25 release notes: https://tip.golang.org/doc/go1.25#compiler

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion visit https://groups.google.com/d/msgid/golang-nuts/61d20eb5-8102-429e-a29c-17a3da0314f3n%40googlegroups.com.

[Ian Lance Taylor]

On Mon, Jul 7, 2025, 8:47 AM Robert Engels <ren...@ix.netcom.com> wrote:

This change seems wrong just based on the io.Reader returns of valid bytes and an error. An error doesn’t always mean the other return values are invalid - it is decided by the api method. 

I think you are misunderstanding the actual change, which is about how nil dereference checks are handled. Previously they were moved later than they should have been.

Ian

To view this discussion visit https://groups.google.com/d/msgid/golang-nuts/7D77D6E6-277A-4014-8F4D-C034B3973526%40ix.netcom.com.

[Keith Randall]

On Monday, July 7, 2025 at 8:48:03 AM UTC-7 Robert Engels wrote:

This change seems wrong just based on the io.Reader returns of valid bytes and an error.

I'm not sure what you are saying. This change (CL 657715) is just about doing (pointer) nil checks where the spec says they should go. There are no nil checks when using the first result of an io.Reader.Read call. If you think there is a problem, please find a program demonstrating the problem.

 

An error doesn’t always mean the other return values are invalid - it is decided by the api method. 

100% agree.

[Robert Engels]



It seems I clearly don’t understand it - based on the example in the documentation for the 1.25 compiler changes - why would this gave to panic? You can call methods on nil pointers - so this call is valid - it all depends on what the method does. 

Eg in this case f.Name() could return a default if f is nil. 

So what is the fix here referring to? Maybe it is just an example that requires the semantics of these api calls?

On Jul 7, 2025, at 10:53 AM, Ian Lance Taylor <ia...@golang.org> wrote:



[Robert Engels]

Or maybe it is how the proposed solution is worded. 

“ If this change is affecting your code, the solution is to put the non-nil error check earlier in your code, preferably immediately after the error-generating statement.”

This is only the case if the api call does not return valid results in addition to returning an error value. 

On Jul 7, 2025, at 11:25 AM, Robert Engels <ren...@ix.netcom.com> wrote:



To view this discussion visit https://groups.google.com/d/msgid/golang-nuts/92C78124-EAF8-4E6C-B239-D888061F634A%40ix.netcom.com.

[Ian Lance Taylor]

On Mon, Jul 7, 2025 at 9:24 AM Robert Engels <ren...@ix.netcom.com> wrote:
>
> 

> It seems I clearly don’t understand it - based on the example in the documentation for the 1.25 compiler changes - why would this gave to panic? You can call methods on nil pointers - so this call is valid - it all depends on what the method does.
>
> Eg in this case f.Name() could return a default if f is nil.
>
> So what is the fix here referring to? Maybe it is just an example that requires the semantics of these api calls?

You can call the method f.Name on a nil pointer f. But in this
specific case f has type *os.File, and the method (*os.File).Name will
dereference f. That is the nil dereference that was incorrectly being
moved later.