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.

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

go/types, types2: audit type assertions for potential Alias bugs #67547

Closed
findleyr opened this issue May 21, 2024 · 15 comments
Closed

go/types, types2: audit type assertions for potential Alias bugs #67547

findleyr opened this issue May 21, 2024 · 15 comments
Assignees
@griesemer @findleyr
Milestone
Go1.23

Comments

@findleyr
Copy link
Member

findleyr commented May 21, 2024 •
edited by gabyhelp
Loading

#67540 fixes a bug where GODEBUG=gotypesalias=1 (the default in go 1.23) results in a type checker bug.

While reviewing the fix (https://go.dev/cl/587075), I noticed that right above that bug, there is another, allowing the following program to compile without error (the error comes from vet, not type checking, which may actually indicate another bug: is vet not honoring GODEBUG?):

https://go.dev/play/p/cy0Jma_6jAA?v=gotip

It appears that we need to more thoroughly audit type assertions within the type checker.

CC @griesemer @adonovan
@findleyr findleyr added this to the Go1.23 milestone May 21, 2024
@griesemer
Copy link
Contributor

There are many assertions with type parameters that seem incorrect. I will take this on.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587156 mentions this issue: go/types, types2: add missing Unalias calls (clarification)

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587157 mentions this issue: go/types, types2: don't panic converting a constant to aliased type parameter

gopherbot pushed a commit that referenced this issue May 21, 2024
@griesemer @gopherbot
go/types, types2: add missing Unalias calls (clarification)
816b603 
This change adds an Unalias call in applyTypeFunc and arrayPtrDeref.
At the moment this doesn't change anything or fix any bugs because
of the way these two functions are invoked, but that could change
in the future.

Also, manually reviewed all type assertions to Type types.

Excluding assertions to type parameters, no obvious issues
were found except for #67540 for which a separate fix is pending.

There are potential issues with assertions type parameters
which will be addressed in a follow-up CL.

For #67547.

Change-Id: I312268dc5e104f95b68f115f00aec3ec4c82e41f
Reviewed-on: https://go-review.googlesource.com/c/go/+/587156
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587176 mentions this issue: go/types, types2: coreType/String must consider Alias types

gopherbot pushed a commit that referenced this issue May 21, 2024
@griesemer @gopherbot
go/types, types2: don't panic converting a constant to aliased type p…
814e72f 
…arameter

For #67547.

Change-Id: I1b2118a311dce906327ae6e29e582da539c60b2b
Reviewed-on: https://go-review.googlesource.com/c/go/+/587157
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587158 mentions this issue: go/types, types2: underIs must consider Alias types

gopherbot pushed a commit that referenced this issue May 21, 2024
@griesemer @gopherbot
go/types, types2: coreType/String must consider Alias types
4b77847 
Fixes regression from Go 1.22.

For #67547.

Change-Id: Idd319b9d2a73c824caa2c821df0e2fcd4f58cb08
Reviewed-on: https://go-review.googlesource.com/c/go/+/587176
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
gopherbot pushed a commit that referenced this issue May 21, 2024
@griesemer @gopherbot
go/types, types2: underIs must consider Alias types
ca17bda 
Fixes regression from Go 1.22.

For #67547.

Change-Id: I012681c7b8b01b02018b313dd3804690bc7aeed1
Reviewed-on: https://go-review.googlesource.com/c/go/+/587158
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587159 mentions this issue: go/types, types2: operand.convertibleTo must consider alias types

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587161 mentions this issue: go/types, types2: operand.AssignableTo must consider Alias types

@griesemer
Copy link
Contributor

With these most recent CLs I believe the major issues here are covered.
There are still a few cases (in type inference, in the mono checker) which need attention, but I am not yet sure they are wrong. I will try to create test cases for these as well.

But I think for the freeze we're ok on this front. We may find more regressions but once found, they are easy to address.

gopherbot pushed a commit that referenced this issue May 22, 2024
@griesemer @gopherbot
go/types, types2: operand.convertibleTo must consider Alias types
5ab8f90 
Fixes regression from Go 1.22.

Fixes #67540.
For #67547.

Change-Id: I61f642970c6a9bd8567654bb5ecf645ae77b3bcc
Reviewed-on: https://go-review.googlesource.com/c/go/+/587159
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Auto-Submit: Robert Griesemer <gri@google.com>
gopherbot pushed a commit that referenced this issue May 22, 2024
@griesemer @gopherbot
go/types, types2: operand.AssignableTo must consider Alias types
8d464e4 
Fixes regression from Go 1.22.

For #67547.

Change-Id: Id0d07d6b24e1eab6ed1c7476d9d9b82d28aee80a
Reviewed-on: https://go-review.googlesource.com/c/go/+/587161
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587939 mentions this issue: go/types, types2: add missing Unalias call to mono checker

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587940 mentions this issue: go/types, types2: add missing Unalias call to type string functionality

gopherbot pushed a commit that referenced this issue May 23, 2024
@griesemer
go/types, types2: add missing Unalias call to type string functionality
13e3068 
For #67547.

Change-Id: I999cd31f9a01f91e7984b4e7012c81e8bd9c6b06
Reviewed-on: https://go-review.googlesource.com/c/go/+/587940
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/587820 mentions this issue: go/types, types2: document why Unalias is not needed in some places

gopherbot pushed a commit that referenced this issue May 23, 2024
@griesemer
go/types, types2: document why Unalias is not needed in some places
2fe2e4d 
Documentation change only.

For #67547.

Change-Id: I0da480299c33239bcd1e059f8b9c6d48d8f26609
Reviewed-on: https://go-review.googlesource.com/c/go/+/587820
TryBot-Bypass: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588117 mentions this issue: go/types, types2: pull up Unalias call to cover all of cycleFinder.typ

@griesemer griesemer mentioned this issue May 24, 2024
Closed
gopherbot pushed a commit that referenced this issue May 24, 2024
@griesemer @gopherbot
go/types, types2: pull up Unalias call to cover all of cycleFinder.typ
3ea2be1 
Without a test because it's unclear the situation can actually occur,
but the code is correct because it now mimics the behavior without
explicit Alias nodes.

For #67547.

Change-Id: I21a31af28880ca6d599fe465563d9574c26ed1f1
Reviewed-on: https://go-review.googlesource.com/c/go/+/588117
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588797 mentions this issue: go/types, types2: don't lose alias information during unification

@griesemer griesemer mentioned this issue May 28, 2024
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/588855 mentions this issue: go/types, types2: report error when using uninstantiated signature alias

gopherbot pushed a commit that referenced this issue May 29, 2024
@griesemer @gopherbot
go/types, types2: don't lose alias information during unification
ee29dbe 
While at it, rename asTypeParam to asBoundTypeParam for clarity.

For #67547.
Fixes #67628.

Change-Id: I2f447c4cd4d72f5315fe9323d82fcb9bf33657c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/588797
Reviewed-by: Robert Griesemer <gri@google.com>
Auto-Submit: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Commit-Queue: Robert Griesemer <gri@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
gopherbot pushed a commit that referenced this issue May 29, 2024
@griesemer @gopherbot
go/types, types2: report error when using uninstantiated signature alias
13c4909 
For #67547.
Fixes #67683.

Change-Id: I9487820ab4e2bd257d253a7016df45729b29f836
Reviewed-on: https://go-review.googlesource.com/c/go/+/588855
Auto-Submit: Robert Griesemer <gri@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
@griesemer
Copy link
Contributor

All type assertions and type switches in go/types, types2, have been reviewed and updated as needed.
Closing this as completed.

This was referenced Aug 8, 2024
Closed
Closed
@gabyhelp gabyhelp mentioned this issue Oct 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@griesemer griesemer

@findleyr findleyr

Labels
None yet
Projects
None yet
Milestone
Go1.23
Development

No branches or pull requests
3 participants
@griesemer @gopherbot @findleyr