Skip to content

types2: adjust types reported for shift expressions with constant RHS to match go/types #47410

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.

Sign up for GitHub

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

Jump to bottom
Closed
findleyr opened this issue Jul 26, 2021 · 13 comments
Closed

types2: adjust types reported for shift expressions with constant RHS to match go/types #47410

findleyr opened this issue Jul 26, 2021 · 13 comments
Assignees
@griesemer
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.24

Comments

@findleyr
Copy link
Member

findleyr commented Jul 26, 2021 •
edited
Loading

As discussed in #47243, go/types should preserve the type of untyped constants on the RHS of a shift expression.

However, it does this inconsistently for integral vs non-integral types. Consider

  var s int
  var _ = s << 1
  var _ = s << 2.

In 1.16 (and probably earlier), go/types reports the type of 1 as untyped int, but the type of 2. as uint. It should preserve the type of 2. as untyped float.

https://play.golang.org/p/aZZ48onPW2z

We're going to preserve this behavior for 1.17, but should try to fix it for 1.18.

CC @griesemer
@findleyr findleyr added this to the Go1.18 milestone Jul 26, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/337529 mentions this issue: go/types: preserve untyped constants on the RHS of a shift expression

gopherbot pushed a commit that referenced this issue Jul 27, 2021
@findleyr
go/types: preserve untyped constants on the RHS of a shift expression
33ff155 
CL 291316 fixed go/types to verify that untyped shift counts are
representable by uint, but as a side effect also converted their types
to uint.

Rearrange the logic to keep the check for representability, but not
actually convert untyped integer constants. Untyped non-integer
constants are still converted, to preserve the behavior of 1.16. This
behavior for non-integer types is a bug, filed as #47410.

Updates #47410
Fixes #47243

Change-Id: I5eab4aab35b97f932fccdee2d4a18623ee2ccad5
Reviewed-on: https://go-review.googlesource.com/c/go/+/337529
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@seankhliao seankhliao added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 28, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/338195 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: preserve untyped constants on the RHS of a shift expression

@griesemer griesemer self-assigned this Jan 18, 2022
@ianlancetaylor
Copy link
Member

@griesemer This is in the 1.18 milestone; time to move to 1.19? Thanks.

@griesemer
Copy link
Contributor

There's a pending CL with failures. It's on my list to investigate.

@griesemer
Copy link
Contributor

The issue here is what type is reported through the API but in contrast to go/types the current API works for the compiler. Eventually we want types2 to match go/types in this respect but it doesn't seem important to do this for 1.18.

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Jan 29, 2022
@griesemer griesemer changed the title go/types: inconsistent types reported for shift expressions with constant RHS types2: adjust types reported for shift expressions with constant RHS to match go/types Jan 29, 2022
@griesemer griesemer changed the title types2: adjust types reported for shift expressions with constant RHS to match go/types types2: adjust types reported for shift expressions with constant RHS to match go/types Jan 29, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/396775 mentions this issue: go/types: don't report errors for untyped int shifts on Go < 1.13

gopherbot pushed a commit that referenced this issue Apr 2, 2022
@findleyr
go/types: don't report errors for untyped int shifts on Go < 1.13
8a816d5 
CL 337529 introduced upfront type-checking of constant shift operands,
to avoid converting their type to uint (per the spec). However, it
had an oversight in that the checks intended for non-constant operands
still ran after the explicit checking of constant operands. As a
result, there are at least two bugs:
 - When GoVersion is < 1.13, we report spurious errors for untyped
   constant shift operands.
 - When the operand is an untyped float constant, we still convert to
   uint (this was a known bug reported in #47410).

Looking at this now, it seems clear that we can avoid both of these bugs
by simply not running the additional checks in the case of a constant
operand. However, this should be considered with some care, as shifts
are notoriously tricky.

Updates #47410
Fixes #52031

Change-Id: Ia489cc5470b92a8187d3de0423d05b309daf47bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/396775
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/397680 mentions this issue: [release-branch.go1.18] go/types: don't report errors for untyped int shifts on Go < 1.13

gopherbot pushed a commit that referenced this issue Apr 6, 2022
@findleyr @cherrymui
[release-branch.go1.18] go/types: don't report errors for untyped int…
9535031 
… shifts on Go < 1.13

CL 337529 introduced upfront type-checking of constant shift operands,
to avoid converting their type to uint (per the spec). However, it
had an oversight in that the checks intended for non-constant operands
still ran after the explicit checking of constant operands. As a
result, there are at least two bugs:
 - When GoVersion is < 1.13, we report spurious errors for untyped
   constant shift operands.
 - When the operand is an untyped float constant, we still convert to
   uint (this was a known bug reported in #47410).

Looking at this now, it seems clear that we can avoid both of these bugs
by simply not running the additional checks in the case of a constant
operand. However, this should be considered with some care, as shifts
are notoriously tricky.

While cherry-picking, the new test file is updated to use the go1_12
package name, following our convention for specifying language version
in the release branch.

Fixes #52032

Change-Id: Ia489cc5470b92a8187d3de0423d05b309daf47bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/396775
Reviewed-by: Robert Griesemer <gri@golang.org>
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit 8a816d5)
Reviewed-on: https://go-review.googlesource.com/c/go/+/397680
@griesemer
Copy link
Contributor

Moving to 1.20. Should make the adjustment for use with the unified IR.

@griesemer griesemer modified the milestones: Go1.19, Go1.20 Jun 23, 2022
@griesemer
Copy link
Contributor

This doesn't affect users of Go. It's just a consistency issue between type2 and go/types. Easier to fix once we only have the unified IR. Moving to 1.21.

@findleyr findleyr modified the milestones: Go1.20, Go1.21 Nov 17, 2022
@griesemer
Copy link
Contributor

Too late for 1.21.

@griesemer griesemer modified the milestones: Go1.21, Go1.22 Jun 1, 2023
@griesemer
Copy link
Contributor

Moving to 1.23.

@griesemer griesemer modified the milestones: Go1.22, Go1.23 Nov 30, 2023
@griesemer griesemer modified the milestones: Go1.23, Go1.24 May 14, 2024
@griesemer
Copy link
Contributor

Moving to 1.24.

@griesemer
Copy link
Contributor

It appears that this has been fixed (probably through a combination of the CLs mentioned in this issue). The playground example reports:

Types and Values of each expression:
 5: 8 | int                 | int
 6: 6 | s                   | int
 6: 6 | s << 1              | int
 6:11 | 1                   | untyped int = 1
 7: 6 | s                   | int
 7: 6 | s << 2.             | int
 7:11 | 2.                  | untyped float = 2

i.e., the recorded types of 1 and 2. are as expected.
Also, Checker.shift is identical between go/types and types2 (modulo AST representation differences).

Closing and considered fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
Release Blockers
Status: Done
Milestone
Go1.24
Development

No branches or pull requests
5 participants
@ianlancetaylor @griesemer @gopherbot @seankhliao @findleyr