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: conversion from int/float typed constant to complex variable changed in 1.14 #38117

Closed
JetSetIlly opened this issue Mar 27, 2020 · 9 comments
Assignees
Milestone

Comments

@JetSetIlly
Copy link

@JetSetIlly JetSetIlly commented Mar 27, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.1 linux/amd64

and

$ go version
go version go1.13.6 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE="auto" GOARCH="amd64" GOBIN="/home/user/Go/go/bin" GOCACHE="/home/user/.cache/go-build" GOENV="/home/user/.config/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="linux" GOINSECURE="" GONOPROXY="" GONOSUMDB="" GOOS="linux" GOPATH="/home/user/Go/go" GOPRIVATE="" GOPROXY="direct" GOROOT="/home/user/Go/go-current" GOSUMDB="off" GOTMPDIR="" GOTOOLDIR="/home/user/Go/go-current/pkg/tool/linux_amd64" GCCGO="gccgo" AR="ar" CC="gcc" CXX="g++" CGO_ENABLED="1" GOMOD="" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build444610156=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Ran the following test program for both specified versions.

package main

import "testing"

// int -> complex

const constInt = 1

func TestConstInt(t *testing.T) {
	_ = complex64(constInt)
}

const constIntExplicit = int(1)

func TestConstIntExplicit(t *testing.T) {
	_ = complex64(constIntExplicit)
}

func TestVarInt(t *testing.T) {
	varInt := 1
	_ = complex64(varInt)
}

func TestVarIntExplicit(t *testing.T) {
	varIntExplicit := int(1)
	_ = complex64(varIntExplicit)
}

// float -> complex

const constFloat = 1.0

func TestConstFloat(t *testing.T) {
	_ = complex64(constFloat)
}

const constFloatExplicit = float64(1.0)

func TestConstFloatExplicit(t *testing.T) {
	_ = complex64(constFloatExplicit)
}

func TestVarFloat(t *testing.T) {
	varFloat := 1.0
	_ = complex64(varFloat)
}

func TestVarFloatExplicit(t *testing.T) {
	varFloatExplicit := float64(1.0)
	_ = complex64(varFloatExplicit)
}

What did you expect to see?

The test results to be the same for go version 1.13.6 and 1.14.1

What did you see instead?

Whereas both versions fail (correctly) when converting variable int/float to complex, they behave differently when converting typed constants to complex.

  1.13.6 1.14.1
const int y y
const int (explicit type) y n
variable int n n
variable int (explicit type) n n
const float y y
const float (explicit type) y n
variable float n n
variable float (explicit type) n n

The results for go version 1.14.1 are the same for v1.14

@randall77
Copy link
Contributor

@randall77 randall77 commented Mar 27, 2020

@andybons andybons added this to the Unplanned milestone Mar 27, 2020
@ianlancetaylor ianlancetaylor changed the title Conversion from int/float typed constant to complex variable cmd/compile: conversion from int/float typed constant to complex variable changed in 1.14 Mar 27, 2020
@ianlancetaylor ianlancetaylor modified the milestones: Unplanned, Go1.15 Mar 27, 2020
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 27, 2020

This is a change in compiler behavior, so I'm marking it as a release blocker for 1.15. If we decide that the change was in error, then we need to backport the fix to the 1.14 release branch. My quick reading of the spec is that it is unclear in this area, but I may have missed something.

CC @mdempsky because this may be due to https://golang.org/cl/187657.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 27, 2020

Minimized test case:

package p

const _ = complex128(int(0))

I think it should be accepted. Conversions says "A constant value x can be converted to type T if x is representable by a value of T." And Representability says "A constant x is representable by a value of type T if one of the following conditions applies: - x is in the set of values determined by T. [...]"

int(0) is a (typed) constant value, and 0 is in the set of values determined by complex128.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Mar 27, 2020

CC @mdempsky because this may be due to https://golang.org/cl/187657.

Yeah. When writing that CL I assumed that whether we can convert (x : T) to U was simply a function of T and U. But in the case of T being an integer or floating point type and U being a complex type (or vice versa), it also depends on whether x is a constant.

@mdempsky mdempsky self-assigned this Mar 27, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 27, 2020

Change https://golang.org/cl/226197 mentions this issue: cmd/compile: fix constant conversion involving complex types

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 28, 2020

Looks like we're going back to the 1.13 behavior. I'll let @griesemer review the CL, but:

@gopherbot Please open a backport to 1.14.

The compiler behavior changed the way it handles constant conversions. This can break existing programs with no workaround other than rewriting the program, which should not be needed.

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 28, 2020

Backport issue(s) opened: #38122 (for 1.13), #38123 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 31, 2020

I agree with the general assessment. Generally speaking, numeric constant conversions are valid if the converted constant can be represented by the new type.

@gopherbot gopherbot closed this in 3431428 Mar 31, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 7, 2020

Change https://golang.org/cl/232719 mentions this issue: [release-branch.go1.14] cmd/compile: fix constant conversion involving complex types

gopherbot pushed a commit that referenced this issue May 8, 2020
…g complex types

In CL 187657, I refactored constant conversion logic without realizing
that conversions between int/float and complex types are allowed for
constants (assuming the constant values are representable by the
destination type), but are never allowed for non-constant expressions.

This CL expands convertop to take an extra srcConstant parameter to
indicate whether the source expression is a constant; and if so, to
allow any numeric-to-numeric conversion. (Conversions of values that
cannot be represented in the destination type are rejected by
evconst.)

Fixes #38123.
For #38117.

Change-Id: Id7077d749a14c8fd910be38da170fa5254819f2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/226197
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
(cherry picked from commit 3431428)
Reviewed-on: https://go-review.googlesource.com/c/go/+/232719
Run-TryBot: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.