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

cmd/compile: inconsistent shift operator type check #66071

Closed
hajimehoshi opened this issue Mar 2, 2024 · 19 comments
Closed

cmd/compile: inconsistent shift operator type check #66071

hajimehoshi opened this issue Mar 2, 2024 · 19 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.

Comments

@hajimehoshi
Copy link
Member

Go version

go version go1.22.0 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/hajimehoshi/Library/Caches/go-build'
GOENV='/Users/hajimehoshi/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/hajimehoshi/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/hajimehoshi/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/cj/73zbb35j0qx5t4b6rnqq0__h0000gn/T/go-build2140543503=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Run this:

package main

import "fmt"

func main() {
	s := 1
	a := 2.0 << s
	b := 2.0<<s + s<<3.0
	fmt.Println(a, b)
}

What did you see happen?

./main.go:7:7: invalid operation: shifted operand 2.0 (type float64) must be integer

What did you expect to see?

Both are errors, or both are correct. (I hope both are correct). Actually this program worked:

package main

import "fmt"

func main() {
	s := 1
	b := 2.0<<s + s<<3.0
	fmt.Println(b)
}
@hajimehoshi hajimehoshi changed the title cmd/go: inconsistent shift operator type check cmd/compile: inconsistent shift operator type check Mar 2, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 2, 2024
@hajimehoshi
Copy link
Member Author

https://go.dev/play/p/OHnUA3weL8w might be a more minimized case:

package main

import "fmt"

func main() {
	a := 1
	b := 1.0<<a + 0 // Error
	c := 1.0<<a + int(0) // Not error
	fmt.Println(b, c)
}

@hajimehoshi
Copy link
Member Author

The spec says:

If the left operand of a non-constant shift expression is an untyped constant, it is first implicitly converted to the type it would assume if the shift expression were replaced by its left operand alone.

So this seems an expected behavior. However, I feel like this caused counter-intuitive behaviors. Another example is:

package main

import "fmt"

func main() {
	s := 1
	a := int(1 << s)
	b := float32(1 << s) // Fail
	fmt.Println(a, b)
}

@zigo101
Copy link

zigo101 commented Mar 2, 2024

Yes, it is not a bug.

There is indeed some counter-intuitive sometimes:

package main

const s = "Go101.org" // len(s) == 9
var a byte = 1 << len(s) / 128 // len(s) is a constant
var b byte = 1 << len(s[:]) / 128 // s[:] is not a constant, so not len(s[;]) too.

func main() {
	println(a, b) // 4 0
}

The reason for the design is to make the evaluation of y be consistent between 32-bit and 64-bit archs.

var n = 32
var y = int64(1 << n)

@hajimehoshi
Copy link
Member Author

var n = 32
var y = int64(1 << n)

Hmm, but this varies between 32bits and 64bits. I was wondering why a shift operator is treated in a special way.

var n = 0x40000000
var y = int64(4 * n)

@zigo101
Copy link

zigo101 commented Mar 2, 2024

var n = 32
var y = int64(1 << n)

Hmm, but this varies between 32bits and 64bits.

R u sure?

@hajimehoshi
Copy link
Member Author

hajimehoshi commented Mar 2, 2024

Yes, I am sure. On Windows:

> cat main.go
package main

import "fmt"

func main() {
        var n = 0x40000000
        var y = int64(4 * n)
        fmt.Println(y)
}
> go run main.go
4294967296
> & (Get-Process -Id $PID).Path { $env:GOARCH="386"; go run main.go }
0

@zigo101
Copy link

zigo101 commented Mar 2, 2024

R u sure the '0' is the output of go run main.go?

@hajimehoshi
Copy link
Member Author

Yes.

> cat main.go
package main

import (
       "fmt"
       "runtime"
)

func main() {
        fmt.Printf("GOARCH is %s\n", runtime.GOARCH)
        var n = 0x40000000
        var y = int64(4 * n)
        fmt.Println(y)
}

> go run main.go
GOARCH is arm64
4294967296

> & (Get-Process -Id $PID).Path { $env:GOARCH="386"; go run main.go }
GOARCH is 386
0

@hajimehoshi
Copy link
Member Author

hajimehoshi commented Mar 2, 2024

So, usually a binary operator determines an untyped value's type if the other is a typed value. However, a shift operator behaves in a very special way ignoring the right side value's type and using other contexts.

@ianlancetaylor
Copy link
Contributor

This is how the language works, it is documented in the spec, and we aren't going to change it now.

There have been extensive discussions of this issue in the past on golang-nuts and in issues like #14844.

@ianlancetaylor ianlancetaylor closed this as not planned Won't fix, can't repro, duplicate, stale Mar 2, 2024
@zigo101
Copy link

zigo101 commented Mar 2, 2024

If you are sure Windows 32-bit prints 0, then it might be a bug in my opinion.
(I think so, but @griesemer and @ianlancetaylor can give an authoritative answer.)

However, a shift operator behaves in a very special way ignoring the right side value's type and using other contexts.

Only when the right side is not a constant.

@hajimehoshi
Copy link
Member Author

This is how the language works, it is documented in the spec, and we aren't going to change it now.

Thanks, but I was wondering if it is possible to change the spec to allow s := 1; a := 2.0 << s and s := 1; a := 1.0<<s + 0. The current spec seems pretty tricky.

If you are sure Windows 32-bit prints 0, then it might be a bug in my opinion.

I don't think this is a bug.

Only when the right side is not a constant.

Ah yes, true

@zigo101
Copy link

zigo101 commented Mar 2, 2024

If it is not bug, then the special rule for bit-shift operation is meaningless.

@hajimehoshi
Copy link
Member Author

If it is not bug, then the special rule for bit-shift operation is meaningless.

Are you sure the special rule was introduced for the int64(1 << n) case?

@zigo101
Copy link

zigo101 commented Mar 2, 2024

Yes.

By the spec, int64(1 << n) is equivalent to int64(1) << n.

@hajimehoshi
Copy link
Member Author

I mean I was wondering if you saw the discussion or the background how the spec for shifts was determined.

@zigo101
Copy link

zigo101 commented Mar 2, 2024

Yes, I saw it in a discussion, but it is hard to find it now. If I remember correctly, @ianlancetaylor made the explanation.

@ianlancetaylor
Copy link
Contributor

@hajimehoshi The rules for shifts are indeed tricky. They were developed over many years as we encountered different issues in real code. We aren't going to change them now.

The basic issue is that constants in Go are untyped, but non-constant expressions have to have a type. Let's say we have an operation with an untyped constant on the left and a typed expression on the right. For most binary operators we simply say that the left hand side is converted to the type of the right hand side. For shifts that does not make sense: there is no necessary connection between the types of the left hand and right hand side. So for shifts we ignore the type of the right hand side and apply the general rule for a plain untyped constant: it gets its type from the context in which it appears.

For s := 1; a := 2.0 << s the context for 2.0 << s does not provide any type. Since there is no type from the context, the language chooses the default type for the untyped constant. The default type for 2.0 is float64. A shift of a float64 is invalid.

If we change that, what would we change it to? Experience tells us that the issue will pop up somewhere else. So we've settled the semantics, and we aren't going to change it now.

Fortunately shifts of an untyped constant written as 2.0 are rare. Why would anybody write 2.0 when they really mean 2? So while it's a confusing corner of the language, it's not a very important one.

@hajimehoshi
Copy link
Member Author

Thank you for elaborating!

For most binary operators we simply say that the left hand side is converted to the type of the right hand side. For shifts that does not make sense: there is no necessary connection between the types of the left hand and right hand side.

Thanks. This explanation makes the most sense to me. (My understanding is that there are no 'type' relashipships between a and b in a*(2^b))

Fortunately shifts of an untyped constant written as 2.0 are rare. Why would anybody write 2.0 when they really mean 2? So while it's a confusing corner of the language, it's not a very important one.

I admit this case (2.0 << s) is unrealistic and useless. I think the below can be an acutal usage, but I have no idea how to modify the spec to allow this, and probably it is impossible.

package main

import "fmt"

func main() {
	a := 1
	b := float32(1 << a)
	fmt.Println(b)
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Development

No branches or pull requests

4 participants