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/internal/types2: "real(0)" and "imag(0)" have type "untyped float", but value of kind constant.Int #43891

Closed
mdempsky opened this issue Jan 25, 2021 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

On dev.typeparams, go tool compile -G=3 $(go env GOROOT)/test/fixedbugs/issue11945.go currently fails because the real(0) and imag(0) expressions have type "untyped float", but then their go/constant representation has kind constant.Int, whereas irgen expects it to be constant.Float.

My intuition is that the TypeAndValue.Value's constant.Kind should always match the TypeAndValue.Type's Underlying's BasicInfo's Is{Boolean,Integer,Float,Complex,String}.

However, if that expectation is incorrect/unreasonable, I think this would be reasonable to handle in irgen instead of types2. (With the recent types2 cleanup, untyped numeric values are only seen in constant declarations. We also already have the constant's type before we set the value, so it wouldn't be hard to coerce to the expected constant.Kind.)

/cc @griesemer @findleyr @danscales

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 25, 2021
@mdempsky
Copy link
Member Author

FWIW, after CLs 286176, 286214, and 286215, this is the only remaining failure for "GO_GCFLAGS=-G=3 go run run.go". (Note though: errorcheck tests don't use GO_GCFLAGS.)

@mdempsky
Copy link
Member Author

It looks like this may actually be a go/constant bug: the Go spec say the real and imag functions always return an untyped float when given an untyped constant argument, so I think constant.Real and constant.Imag should make the same guarantee.

@mdempsky
Copy link
Member Author

mdempsky commented Jan 25, 2021

Incidentally, I'm suspicious of the constant.ToFloat calls in go/types and types2's handling of complex (also copied to typecheck/const.go:makeComplex). I'd think 2i and complex(0, 2) should be interchangeable, yet the program below prints two different values (with or without -G=3).

package main

import "fmt"

const ulp1 = imag(1i + 2i / 3 - 5i / 3)
const ulp2 = imag(1i + complex(0, 2) / 3 - 5i / 3)

func main() {
	fmt.Printf("%g %g\n", ulp1, ulp2)
}

This discrepancy is because constant.ToFloat forces integer constants to big.Float representation, but constant.MakeImag (used to convert 2 into 2i in ulp1) does not.

@griesemer griesemer self-assigned this Jan 25, 2021
@griesemer griesemer added this to the Go1.17 milestone Jan 25, 2021
@griesemer
Copy link
Contributor

Thanks for tracking this down. There appear to be two separate issues at stake here:

  1. Internally, go/constant is supposed to use the most "efficient" or accurate representation for a value of a specific kind. constant.ToFloat seems incorrect: The "default" representation for a constant.Float value is the rational representation, and if we change that in ToFloat, the above mentioned discrepancy disappears. So I think this is clearly a bug in ToFloat.

  2. I believe originally, go/constant value kinds didn't have to match the type used by the type checker (go/types or types2). The kind simply described the value "as is". But I am not sure if this is still supposed to be true, or if we happen to have an implicit match between types and kinds at this point. That said, if I remove AssertValidTypeForConst(n.Type(), v) from ir/name.go:328 the test $(go env GOROOT)/test/fixedbugs/issue11945.go passes.

I need to think a bit more on both of these to convince myself of the correct solution.

@griesemer
Copy link
Contributor

Factored out problem 1) into #43908.

@mdempsky
Copy link
Member Author

That said, if I remove AssertValidTypeForConst(n.Type(), v) from ir/name.go:328 the test $(go env GOROOT)/test/fixedbugs/issue11945.go passes.

Without actually trying it, I think that's only because the declared constants aren't exported. If they were renamed from "_" to "ExportedConstant" or something, I expect it would cause another crash in the exporter, because the value representation wouldn't match the untyped kind.

I need to think a bit more on both of these to convince myself of the correct solution.

Ack. In the mean time, I'll look into adding a workaround/fix in irgen.constDecl, so we can enable GO_GCFLAGS=-G=3 go run run.go. In should be pretty minimal and easy to back out again if we decide to fix this within types2 instead.

@gopherbot
Copy link

Change https://golang.org/cl/286652 mentions this issue: [dev.typeparams] cmd/compile: force untyped constants from types2 to expected kind

gopherbot pushed a commit that referenced this issue Jan 26, 2021
…expected kind

Currently, types2 sometimes produces constant.Values with a Kind
different than the untyped constant type's Is{Integer,Float,Complex}
info, which irgen expects to always match.

While we mull how best to proceed in #43891, this CL adapts irgen to
types2's current behavior. In particular, fixedbugs/issue11945.go now
passes with -G=3.

Updates #43891.

Change-Id: I24823a32ff49af6045a032d3903dbb55cbec6bef
Reviewed-on: https://go-review.googlesource.com/c/go/+/286652
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

I can confirm that the exporter forces a particular constant representation based on type. However, go/types (and it's importers) do not expect this, nor was this the original intent for go/constant: The representation of a value can be independent of its type.

This was not an issue for the compiler before the use of go/constant for constant evaluation. But now that we use it, e.g. forcing an exported floating-point constant to "float" representation often will convert an exactly represented value (a fraction) into an arbitrary-precision float, causing a loss of precision.

I think we should try to avoid this, but it will require an extra byte of information (the constant.Kind) in exported values, and with that a version bump. We may want to consider adding the byte (only required for numerical values) when we extend the export format for generics. (We just need to make the space, we don't need to use the byte right away.)

@gopherbot
Copy link

Change https://golang.org/cl/292351 mentions this issue: go/internal/gcimporter: fix reexporting compiler data

gopherbot pushed a commit to golang/tools that referenced this issue Feb 17, 2021
The current versions of the indexed export format encode constant
values based on their type. However, IExportData was encoding
constants values based on their own kind instead. While cmd/compile
internally keeps these matched, go/types does not guarantee the
same (see also golang/go#43891).

This CL fixes this issue, and also extends testing to check reading in
the compiler's export data and that imported packages can be exported
again.

Change-Id: I08f1845f371edd87773df0ef85bcd4e28f5db2f5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/292351
gopls-CI: kokoro <noreply+kokoro@google.com>
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@griesemer
Copy link
Contributor

Confirming that go tool compile -G=3 $(go env GOROOT)/test/fixedbugs/issue11945.go now passes, so this is not an immediate issue anymore. The same is true when running this test through types2 and go/types.

The go/types constant representation is more flexible than what the compiler export data permits. Filed issue #45837 to cover that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants