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: disallow not-in-heap types to be used as type arguments #54765

Closed
mdempsky opened this issue Aug 30, 2022 · 4 comments
Closed

cmd/compile: disallow not-in-heap types to be used as type arguments #54765

mdempsky opened this issue Aug 30, 2022 · 4 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@mdempsky
Copy link
Member

mdempsky commented Aug 30, 2022

I think we shouldn't allow not-in-heap types to be used as type arguments.

func f[T any]() { println(new(T)) }

is a valid generic function declaration, but instantiating it as f[nih] will (or should) fail. (We're inconsistent about whether we catch this today: https://go.dev/play/p/bRMtJouiaR5)

In theory, we could still allow instantiating f[nih] if f doesn't contain new(T), or allocate T on the stack, or any of the other problematic use cases; but this violates the generics principle that the type parameter constraints fully constrain what type arguments are valid.

I think the most likely case where this could come up today is with cgo and incomplete types; a user might want to write atomic.Pointer[C.some_incomplete_type], which would now fail to compile if we reject not-in-heap types as type arguments. However, internally atomic.Pointer is using an unsafe.Pointer to store the pointer, which may not be appropriate for a *C.some_incomplete_type value, so arguably this is already potentially broken.

/cc @ianlancetaylor @randall77

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 30, 2022
@mdempsky mdempsky added this to the Go1.20 milestone Aug 30, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Aug 30, 2022
@randall77
Copy link
Contributor

randall77 commented Aug 30, 2022

I agree, we should probably disallow this. I don't see how it would be terribly useful, and I don't see how we could implement it safely for almost any uses of T.

We should allow instantiating with a pointer-to-nih type, though. We need to be at least a bit careful there, as a pointer-to-nih must be treated as a uintptr, not a pointer. Which means when shapifying we can't unify pointer-to-nih types with *byte (like we do for most pointer types). I think we already have that special case?

@mdempsky
Copy link
Member Author

mdempsky commented Aug 30, 2022

We should allow instantiating with a pointer-to-nih type, though.

Ack, agreed. Pointer-to-nih types are not themselves nih, so they're not subject to the proposed restriction here.

I think we already have that special case?

Yeah, both nounified and unified have code to exempt pointer-to-nih from the current pointer shaping. I haven't spent too much time on it yet, but I'm pretty sure it's right.

@mknyszek
Copy link
Contributor

mknyszek commented Aug 31, 2022

@mdempsky assigning to you for now, but feel free to unassign it if you don't plan to work on it. Sounds like we should probably have some thing here for 1.20 (does this need a backport?), but I'm not sure.

@gopherbot
Copy link

gopherbot commented Aug 31, 2022

Change https://go.dev/cl/427235 mentions this issue: cmd/compile: reject not-in-heap types as type arguments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
Status: Done
Development

No branches or pull requests

4 participants