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

go/types,cmd/compile/internal/types2: inconsistent handling of len on pointer-to-array type parameters #47991

Closed
mdempsky opened this issue Aug 26, 2021 · 7 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Milestone

Comments

@mdempsky
Copy link
Member

This package currently fails to typecheck in H:

package p

func F[Ptr interface{ ~*Elem }, Elem interface { [1]int }](p Ptr) int {
  return len(p) // ok: types2 allows implicit dereference here
}

func G[Ptr interface{ ~*Elem }, Elem interface { [1]int | [2]int }](p Ptr) int {
  return len(*p) // ok(?): types2 allows explicit dereference and length of Elem
}

func H[Ptr interface{ ~*Elem }, Elem interface { [1]int | [2]int }](p Ptr) int {
  return len(p) // FAIL(?): types2 says len(p) is not allowed
}

It's unclear to me the correct behavior here, but the current behavior at least seems inconsistent to me.

/cc @findleyr

@mdempsky mdempsky added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 26, 2021
@mdempsky mdempsky added this to the Go1.18 milestone Aug 26, 2021
@griesemer
Copy link
Contributor

The "problem" here is that in F the Elem type parameter does have a structural constraint and thus (in the implementation) an operational type which is an array, and then implicit dereferencing works. In H, the Elem type parameter does not have a structural constraint (because it's type set has two types), and therefore no operational type, and then implicit dereferencing doesn't work anymore. This is probably an inconsistency we should try to address.

@griesemer
Copy link
Contributor

griesemer commented Oct 28, 2021

With the latest decisions/implementation the status here is as follows. For:

package p

func F[Ptr ~*Elem, Elem [1]int](p Ptr) int {
	return len(p) // invalid
}

func G[Ptr ~*Elem, Elem [1]int | [2]int](p Ptr) int {
	return len(*p)
}

func H[Ptr ~*Elem, Elem [1]int | [2]int](p Ptr) int {
	return len(p) // invalid
}

we get:

x.go:4:13: invalid argument: p (variable of type Ptr constrained by ~*Elem) for len
x.go:12:13: invalid argument: p (variable of type Ptr constrained by ~*Elem) for len

That is, implicit pointer-to-array dereferencing only happens if we have a pointer to an array (or a type parameter constrained by pointers to arrays, see below), not if we have a pointer to a type parameter.

For comparison, this is valid at the moment:

package p

func _[A *[0]int | [0]int | chan int](a A) {
	_ = len(a)
}

@ianlancetaylor any thoughts on the "correct" behavior?

@griesemer griesemer added okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker labels Oct 28, 2021
@griesemer
Copy link
Contributor

The current rules are:

  • oldlen(p) is permitted if p is an array or a pointer to an array (plus all the other non-array types for which len is permitted) (existing rule)
  • len(p) is permitted if p is a type parameter and len(t) is permitted for all types in the type set of p (additional rule)

This seems consistent with the generalization we apply to other operations on type parameters: an operation is permitted on a value of type parameter if it is permitted on all types in the type set of the type parameter

@mdempsky Are you ok with this? If so, I am inclined to close this as working as intended at this point.

@mdempsky
Copy link
Member Author

len(p) is permitted if p is a type parameter and len(t) is permitted for all types in the type set of p (additional rule)

I think this rule sounds reasonable; but my interpretation of it would be that F and H should both be valid, because Ptr's type set only contains pointer-to-array types and len is valid on all of those. But above you indicated those are still rejected?

Am I missing something? If not, can you elaborate how you interpret that rule to disallow F and H?

@griesemer
Copy link
Contributor

I'm taking the rules fairly literally:
len(p) is a pointer to a type parameter (so not an array, nor a pointer to an array), hence F and H won't be valid.

Type-set computation doesn't extend past the boundaries of an interface. Doing that would have all kinds of implications that we've not explored at all. So the type set of Ptr is the set of all types whose underlying type is *Elem, and Elem is not an array, it's a type parameter.

@mdempsky
Copy link
Member Author

Okay, I see. Thanks for elaborating.

I think rejecting both F and H is consistent and an improvement over the original behavior. I think it's the more conservative option too, so it makes sense to go with it at least for Go 1.18.

I'm inclined to think type-set computation should extend recursively, to be analogous to how defined types work; e.g., len is allowed on defined-pointer-to-defined-array types. But I agree we should think out the implications of that first before committing to it so we don't paint ourselves into a corner.

@griesemer
Copy link
Contributor

Since the implementation of len (and related functions) has been updated to be consistent with these rules, I am going to consider this now "fixed". We can reconsider in the future.

@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 release-blocker
Projects
None yet
Development

No branches or pull requests

3 participants