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: builtin print treat arguments differently from normal functions #49042

Closed
go101 opened this issue Oct 18, 2021 · 13 comments
Closed

cmd/compile: builtin print treat arguments differently from normal functions #49042

go101 opened this issue Oct 18, 2021 · 13 comments

Comments

@go101
Copy link

@go101 go101 commented Oct 18, 2021

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

$ go version
go version go1.17.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

package main

import "fmt"

const X = '\x61' // 'a'
const Y = 0x62
const A = Y - X

func main() {
	fmt.Printf("%T \n", A << 32) // constant 4294967296 overflows rune
	println(A << 32) // okay
}

[Edit]: A simpler one:

package main

import "fmt"

const A = '\x01'

func main() {
	fmt.Println(A << 32) // constant 4294967296 overflows rune
	println(A << 32) // okay
}

What did you expect to see?

Same behavior.

What did you see instead?

Different behavior.

(Maybe this is not a bug, but it should be mentioned somewhere in docs.)

[Edit]: this has been broken since Go toolchain 1.14.

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 18, 2021

At tip, we get the correct errors (one at each line).
Not sure why we didn't 1.17 and earlier, but I'd say this is fixed. Probably not worth backporting.

Loading

@randall77 randall77 closed this Oct 18, 2021
@cuonglm
Copy link
Member

@cuonglm cuonglm commented Oct 18, 2021

@randall77 I think we should re-open this for discussion.

The compiler typechecker has special case for int constant argument to print/println, it treats int constant as int64. While types2 and go/types will report overflow. That's why we got this fixed at tip.

cc @mdempsky @griesemer

@go101 At least, go vet will warn you!

Loading

@go101
Copy link
Author

@go101 go101 commented Oct 18, 2021

@cuonglm Here, by the spec, the default type of A is rune instead of int. So do you mean print/println treats int32 constant as int64?

Loading

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 18, 2021

@cuonglm I'll reopen, but I'm not sure why. Do you have an example where tip misbehaves?

Loading

@randall77 randall77 reopened this Oct 18, 2021
@go101
Copy link
Author

@go101 go101 commented Oct 18, 2021

It is okay before Go 1.14.

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Oct 18, 2021

@cuonglm I'll reopen, but I'm not sure why. Do you have an example where tip misbehaves?

I don't say tip misbehaves, but I mean the old typechecker have special case for int constant as argument to print/println, the int constant will be converted to int64. I don't know off-hand what's the reason the old typecheker had this special case. And print/println is not covered by Go 1 compatibility, though.

Here, by the spec, the default type of A is rune instead of int. So do you mean print/println treats int32 constant as int64?

@go101 No, A is constant kind int.

Loading

@go101
Copy link
Author

@go101 go101 commented Oct 18, 2021

package main

import "fmt"

const X = '\x61' // 'a'
const Y = 0x62
const A = Y - X

func main() {
	fmt.Printf("%T \n", A << 30) // int32
}

If the untyped operands of a binary operation (other than a shift) are of different kinds, the result is of the operand's kind that appears later in this list: integer, rune, floating-point, complex ...

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Oct 18, 2021

@go101 A is an untyped rune constant, and also is numeric constant. Other than float and complex, numeric constants have kind Int https://pkg.go.dev/go/constant#Kind. Sorry for not being clear about this. But I'm talking about the constant kind, not constant type.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 18, 2021

Up through Go 1.17, the predeclared print and println functions are magic: if an untyped integer constant doesn't fit in the type int, they uses the type int64. That has changed on tip.

We can regard this as a bug fix, but I suspect that it may be an accident. In fact the code to use int64 is still there on tip: https://go.googlesource.com/go/+/refs/heads/master/src/cmd/compile/internal/walk/builtin.go#547. Something else is generating the new error.

If we start reporting the error we may break existing working programs. We can do that on the argument that this behavior was never documented and is arguably buggy, but let's at least do it intentionally.

CC @griesemer @findleyr

Loading

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Oct 18, 2021

@ianlancetaylor Yeah, the code you pointed out is for the old typchecker, at tip, we're using types2 instead. That's why I commented above that we should re-open this issue for more discussion.

Also, here's the relevant part in old typechecker:

if ir.IsConst(n1, constant.Int) {

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 18, 2021

The spec says:

Current implementations provide several built-in functions useful during bootstrapping. These functions are documented for completeness but are not guaranteed to stay in the language. They do not return a result.

and:

print      prints all arguments; formatting of arguments is implementation-specific
println    like print but prints spaces between arguments and a newline at the end

So arguably this change is ok. Also, I wouldn't so much call it an "accident" but a consequence of handling the arguments here no different from anywhere else. (I suspect it might be rather annoying to make the code accept the arguably incorrect argument, but I haven't tried yet).

TBD.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 29, 2021

Filed proposal #49216 to decide if we can make this change.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 3, 2021

Per #49216 we will accept the correct behavior and document it in the release notes. If this ends up breaking a lot of code, we can re-open.

Loading

@griesemer griesemer closed this Nov 3, 2021
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
5 participants