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

spec: shift string(1 << s) should be invalid #45114

Closed
griesemer opened this issue Mar 18, 2021 · 7 comments
Closed

spec: shift string(1 << s) should be invalid #45114

griesemer opened this issue Mar 18, 2021 · 7 comments

Comments

@griesemer
Copy link
Contributor

griesemer commented Mar 18, 2021

package p
var s uint
var _ = string(1 << s)

is not accepted by the compiler:

bug.go:3:15: invalid operation: 1 << s (shift of type string)

Per the spec (https://golang.org/ref/spec#Constant_expressions):
"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."

In this case, the left operand would be converted to int (not string). This is different from cases such as, say float64(1 << s) where 1 is implicitly converted to 1.0 because it can be done without loss of precision. It seems a bit far-fetched to claim that a 1 can be converted to a string without loss of precision. (That said, the spec is notoriously difficult to decipher with respect to shifts and implicit conversions.)

As an aside, there's still #3939 which is proposing to prohibit string(int_value), and go vet is checking for this. So we're not permitting a new category of programs by making string(1 << s) valid.

Both go/types and types2 accept this code.

The test $GOROOT/test/fixedbugs/bug193.go assumes that this code should fail.

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 18, 2021
@griesemer griesemer added this to the Go1.17 milestone Mar 18, 2021
@griesemer griesemer self-assigned this Mar 18, 2021
@griesemer
Copy link
Contributor Author

cc: @findleyr

@ianlancetaylor
Copy link
Contributor

My vote is to change the spec, since cmd/compile, gccgo, and GoLLVM all reject this. I'm assuming that nobody is clamoring for this to work.

@griesemer
Copy link
Contributor Author

Fair enough. If anything, we need to clarify the spec in that regard.

@griesemer griesemer changed the title cmd/compile: valid shift string(1 << s) not accepted spec: unclear if shift string(1 << s) should be invalid Mar 19, 2021
@griesemer griesemer added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsFix The path to resolution is known, but the work has not been done. labels Mar 19, 2021
@griesemer griesemer modified the milestones: Go1.17, Backlog Mar 19, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/303094 mentions this issue: test: enable fixedbugs/bug193.go for -G compiler option

gopherbot pushed a commit that referenced this issue Mar 23, 2021
Temporarily disable a questionable test case in fixedbugs/bug193.go
and enable the test as a whole. See the issues below for details.

Updates #45114.
Updates #45117.

Change-Id: I1de6f8d79b592eeeec139cd92b6c9cac56a9a74b
Reviewed-on: https://go-review.googlesource.com/c/go/+/303094
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jan 18, 2022
@griesemer griesemer modified the milestones: Backlog, Go1.18 Jan 18, 2022
@griesemer griesemer changed the title spec: unclear if shift string(1 << s) should be invalid spec: shift string(1 << s) should be invalid Jan 18, 2022
@griesemer griesemer changed the title spec: shift string(1 << s) should be invalid go/types, types2, spec: shift string(1 << s) should be invalid Jan 18, 2022
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/379256 mentions this issue: cmd/compile/internal/types2: report error for invalid string(1 << s)

@griesemer griesemer changed the title go/types, types2, spec: shift string(1 << s) should be invalid spec: shift string(1 << s) should be invalid Jan 18, 2022
@griesemer
Copy link
Contributor Author

griesemer commented Jan 18, 2022

See #45117 for the implementation issue.

@griesemer griesemer added Documentation and removed NeedsFix The path to resolution is known, but the work has not been done. labels Jan 18, 2022
gopherbot pushed a commit that referenced this issue Jan 19, 2022
For #45114.
Fixes #45117.

Change-Id: I71d6650ae2c4c06952fce19959120f15f13c08a2
Reviewed-on: https://go-review.googlesource.com/c/go/+/379256
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/379275 mentions this issue: spec: add another example for an invalid shift change

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
For golang#45114.
Fixes golang#45117.

Change-Id: I71d6650ae2c4c06952fce19959120f15f13c08a2
Reviewed-on: https://go-review.googlesource.com/c/go/+/379256
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
Fixes golang#45114.

Change-Id: I969e5f1037254fc0ffbba2fc07a81a3987e6b05f
Reviewed-on: https://go-review.googlesource.com/c/go/+/379275
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants