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

unique: panic when calling unique.Make with string casted as any #68990

Closed
ALX99 opened this issue Aug 21, 2024 · 9 comments
Closed

unique: panic when calling unique.Make with string casted as any #68990

ALX99 opened this issue Aug 21, 2024 · 9 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@ALX99
Copy link

ALX99 commented Aug 21, 2024

Go version

go version go1.23.0 linux/amd64

Output of go env in your module/workspace:

Go playground

What did you do?

Casting a string to any and passing it to unique.Make seems to cause issues.

package main

import "unique"

func main() {
	unique.Make(any(""))
}

Example crash 1 unexpected fault address 0x912c08: https://go.dev/play/p/vZWwiXU6YXL
Example crash 2 panic: interface conversion: interface {} is *unique.uniqueMap[string], not *unique.uniqueMap[interface {}]: https://go.dev/play/p/U8JvXsqXpJQ

What did you see happen?

For it not to crash

What did you expect to see?

I expected to see nothing and the program not to crash

@ianlancetaylor
Copy link
Contributor

CC @mknyszek

@mknyszek
Copy link
Contributor

Looking into it.

@mknyszek mknyszek self-assigned this Aug 21, 2024
@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 21, 2024
@mknyszek mknyszek added this to the Backlog milestone Aug 21, 2024
@mknyszek
Copy link
Contributor

I see the general problem -- it's a classic reflection mistake when trying to get the type of a value (via TypeOf). This package doesn't use reflection directly, but does something reflect-like to extract the type.

Easy fix, apologies for the breakage.

@mknyszek mknyszek modified the milestones: Backlog, Go1.24 Aug 21, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/607355 mentions this issue: unique: use TypeFor instead of TypeOf to get type in Make

@mknyszek
Copy link
Contributor

@gopherbot Please open a backport issue for Go 1.23.

This problem accidentally limits what types the unique package can work with, and results in panics. Also, the fix is very simple and low risk.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #68992 (for 1.23).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@ALX99
Copy link
Author

ALX99 commented Aug 21, 2024

Appreciate the insanely quick response.
Thank you for your contributions to Go!

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/607435 mentions this issue: [release-branch.go1.23] unique: use TypeFor instead of TypeOf to get type in Make

@dmitshur dmitshur 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 Aug 21, 2024
gopherbot pushed a commit that referenced this issue Aug 21, 2024
…type in Make

Currently the first thing Make does it get the abi.Type of its argument,
and uses abi.TypeOf to do it. However, this has a problem for interface
types, since the type of the value stored in the interface value will
bleed through. This is a classic reflection mistake.

Fix this by implementing and using a generic TypeFor which matches
reflect.TypeFor. This gets the type of the type parameter, which is far
less ambiguous and error-prone.

For #68990.
Fixes #68992.

Change-Id: Idd8d9a1095ef017e9cd7c7779314f7d4034f01a7
Reviewed-on: https://go-review.googlesource.com/c/go/+/607355
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 755c18e)
Reviewed-on: https://go-review.googlesource.com/c/go/+/607435
Reviewed-by: Ian Lance Taylor <iant@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants