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: export format of untyped constants is unnecessarily restricted #45837

Closed
griesemer opened this issue Apr 29, 2021 · 7 comments
Closed
Assignees
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@griesemer
Copy link
Contributor

The compiler exports untyped constants' format to their respective type. types2 and go/types together with the go/constant package don't require a constant representation to match the (untyped) constant type. For instance, it's ok to represent real(0) of type untyped float with a constant of kind constant.Int.

Should remove this restriction from the export format as it may cause loss of precision and also lead to larger than necessary export data. This will require an extra byte for each exported constant, encoding the respective constant kind. This will also require a bump of the export data version.

Marking for go1.18 since we are changing the export format anyway (generics). But it's not crucial to get this in.

See #43891 for details.

@griesemer griesemer added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 29, 2021
@griesemer griesemer added this to the Go1.18 milestone Apr 29, 2021
@griesemer griesemer self-assigned this Apr 29, 2021
@mdempsky
Copy link
Member

Does go/types only ever use more precise representations, or can it use more relaxed representations too? E.g., can an untyped integer/rune constant be represented by a constant.Float, or an untyped float represented by a constant.Complex?

The export data format should also be extended to support rational constants, so they don't need to be truncated to floating point. (We've discussed this before, but I don't see any open issues about it.)

@griesemer
Copy link
Contributor Author

I don't think that the representation can be more relaxed. Untyped constants's kind usually matches the constant kind but when we apply a type (e.g. an untyped 1 is really a float64 1.0) in context we don't necessarily change the constant representation, leading to a 1.0 that is represented as an constant.Int 1.

Agreed on the rational constants.

@gopherbot
Copy link

Change https://golang.org/cl/358035 mentions this issue: go/internal/gcimporter: stub support importing/exporting constant kind

gopherbot pushed a commit to golang/tools that referenced this issue Oct 25, 2021
Add placeholder support for importing/exporting constant kind in the Go
1.18 export data format.

Also eliminate the redundant iimport.iexportVersion field.

For golang/go#45837

Change-Id: I94dbcadde0fae55788ce1a139a2760276f644630
Reviewed-on: https://go-review.googlesource.com/c/tools/+/358035
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 28, 2021
@griesemer
Copy link
Contributor Author

Moving the final fix to 1.19. This has worked so far and will keep working.

@griesemer griesemer added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Dec 16, 2021
@griesemer griesemer modified the milestones: Go1.18, Go1.19 Dec 16, 2021
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.20.
That time is now, so a friendly reminder to look at it again.

@griesemer
Copy link
Contributor Author

With the new unified IR, the import/export data is writing constant type and kind independently. We're not going to further support the old format, so this issue can be closed as fixed.

@golang golang locked and limited conversation to collaborators Oct 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. 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