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: disallow embedding type parameters #43621

Closed
mdempsky opened this issue Jan 11, 2021 · 16 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mdempsky
Copy link
Member

This program prints "A B", which I think is wrong: https://go2goplay.golang.org/p/QJiCMNb0YS3

The type parameters draft mentions that type parameters can be embedded and their methods will be promoted, but it doesn't explain how depth of methods on the original types influences this promotion, if at all, and the test case above demonstrates it's inconsistently handled by cmd/go2go.

I think two reasonable options here are either (1) to specify that for the purposes of method promotion, methods on type parameters are always considered to have depth 0, regardless of their depth on the actual type arguments; or (2) side-step the issue by disallowing embedding type parameters if method promotion could vary depending on the type parameter's methods' depths.

/cc @griesemer @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

The current design draft in effect chooses your option (1): https://go.googlesource.com/proposal/+/refs/heads/master/design/go2draft-type-parameters.md#embedded-type-parameter-methods . The program should print A A.

Please don't draw any conclusions from the behavior of the go2go tool. The support for embedding type parameters is a horrible bodge.

We should add this to our list of test cases, though.

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 11, 2021
@ianlancetaylor ianlancetaylor added this to the Unreleased milestone Jan 11, 2021
@mdempsky
Copy link
Member Author

I see. I read "any methods of the type parameter's constraint are promoted to be methods of the struct" as merely confirming that embedded type parameters participate in method promotion like any other embedded type, not as implying that type parameter methods effectively have depth 0 regardless of their depth on corresponding type arguments.

I think the current wording is fine for most Go developers, but I think advanced Go programmers (particularly Go tools developers) would benefit from an extra sentence clarifying this.

@mdempsky
Copy link
Member Author

I think it's also worth calling this out since it's a case where generic functions intentionally behave differently than a simple textual expansion of type arguments. For example, I think most Go programmers would intuitively expect M1[A3] and M2 here to have the same behavior: https://go2goplay.golang.org/p/EE6ep60Hnti

Granted I don't think they'd intentionally write code exactly like that, but I can imagine someone doing a refactoring where they either introduce or remove type parameters, and then they're bit by some subtle change in semantics like in #41685.

@gopherbot
Copy link

Change https://golang.org/cl/283113 mentions this issue: design: type parameters: clarify depth of embedded type constraint methods

gopherbot pushed a commit to golang/proposal that referenced this issue Jan 12, 2021
…thods

For golang/go#43621

Change-Id: Ice63bffb753a1c429ee3537cb3093f2903d499d6
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/283113
Trust: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@mdempsky
Copy link
Member Author

mdempsky commented Jun 7, 2021

I was thinking about this some more last night / this morning, and I think for type identity we need to distinguish: (1) normal embedding of a type into a struct and (2) embedding a type into a struct via a type parameter. In particular, I think reflect.StructField needs something like a Bound Type field (holding the pure-interface subset of the type parameter's constraint) so that at runtime users of the reflection API can reconstruct the method/field promotion rules.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 7, 2021

I think we can still allow conversions between those two cases, similar to how type identity rules for structs are relaxed for converting structs that have tagged fields. But I think they do need to be different types.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 7, 2021

/cc @bcmills @findleyr for more generics subtleties

@ianlancetaylor
Copy link
Contributor

That sounds like an argument that we should not permit embedding type parameters.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 7, 2021

Disallowing embedding of type parameters would nicely avoid the issue as well.

@bcmills
Copy link
Contributor

bcmills commented Jun 7, 2021

Disallowing embedding for now seems like a good idea. That would reduce the potential for mistakes in the initial spec, and we can always add support in a later version when we can dedicate more time to getting the (subtle) details right.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 8, 2021

Relatedly, I think we should disallow type P[T any] T then, or at least disallow declaring methods on it. go/types2 typechecks this package, but cmd/go2go generates invalid code for it: https://go2goplay.golang.org/p/m0ooQYSN5sc

package main

type A[T any] T
func (A[T]) M() {}

var _ A[*int]
var _ A[interface{}]

func main() {}

@bcmills
Copy link
Contributor

bcmills commented Jun 8, 2021

Yes, we should clearly reject methods with receiver type P[T] or *P[T] for type P[T any] T, since the constraint any allows T to be an interface type.

(That seems like a separate bug.)

@mdempsky
Copy link
Member Author

mdempsky commented Jun 8, 2021

I think the common issue here is that different types have different rules for how they contribute methods/fields when embedded / used as an underlying type, and when they're allowed to be used as the underlying type of a defined type with declared methods.

Note that the issue with A[*int] is that you can't declare methods on defined types whose underlying type is a pointer. Also, maybe you were just emphasizing any because it's what I used in the example code, but every type constraint that doesn't contain a type list/set is satisfiable by an interface.

Another issue I realized this morning is A[struct { M int }] will fail, because of collision for the M selector.

@findleyr
Copy link
Contributor

findleyr commented Jun 8, 2021

I agree with what appears to be the consensus here: we should at least initially reject these ambiguous cases.

We have #45639, which is similar to the case of type P[T any] T. Should we use that issue to track "disallow type parameters as the RHS of a type declaration", and use this issue to track "disallow embedding type parameters"? That way we have separate issues for the presumably separate fixes.

@mdempsky
Copy link
Member Author

mdempsky commented Jun 8, 2021

@findleyr SGTM.

@findleyr findleyr changed the title cmd/go2go: inconsistent method promotion with embedded type parameters go/types, cmd/compile/internal/types2: disallow embedding type parameters Jun 8, 2021
@gopherbot
Copy link

Change https://golang.org/cl/339651 mentions this issue: [dev.typeparams] go/types: embedded type cannot be a (pointer to) a type parameter

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants