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 type parameters as RHS of type declarations #45639

Closed
findleyr opened this issue Apr 19, 2021 · 29 comments

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Apr 19, 2021

Consider the following snippet, with annotated type checking error.

type xer interface {
        x() string
}

func Print[T xer](s []T) {
        type Y T
        for _, v := range s {
                fmt.Println(v.x())
        }
        var y Y
        y.x() // <-- ERROR "y.x undefined (interface xer has no method x)"
}

This error is both inaccurate and (as a secondary concern) fails to mention the named type Y.

Update: per discussion below, let's disallow such declarations (as well as type Y = T) for now.

CC @griesemer

@findleyr findleyr added this to the Go1.18 milestone Apr 19, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 19, 2021

Change https://golang.org/cl/311455 mentions this issue: go/types: support type parameters in NewMethodSet

Loading

@griesemer griesemer self-assigned this Apr 19, 2021
@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 19, 2021

Our proposals do not explain what Y should be. Should it be a type parameter? Or an interface? Maybe it shouldn't be permitted.

Loading

@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Apr 19, 2021

Yes, sorry to be clear the problem here may only be the misleading error message. I wasn't sure.

I think we should either allow Y to have a type parameter as its underlying type (and remove the guard against type parameters in rawLookupFieldOrMethod), or make it an error to declare such a Y. I think it would be more confusing to make Y an interface, as then we'd lose convertibility: var y Y; t := T(y) would be invalid.

Perhaps in the absence of any reason to write this code, we should err on the side of caution and disallow such a declaration (at least for now).

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Apr 19, 2021

Let's disallow it for now. Same for local aliases of type parameters (even though those shouldn't be a problem, I think).

Loading

@findleyr findleyr changed the title go/types, cmd/compile/internal/types2: bad method lookup for named type with type parameter underlying go/types, cmd/compile/internal/types2: disallow local declaration and aliasing of type parameters Apr 20, 2021
@findleyr findleyr changed the title go/types, cmd/compile/internal/types2: disallow local declaration and aliasing of type parameters go/types, cmd/compile/internal/types2: disallow local declarations and aliases of type parameters Apr 20, 2021
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Apr 20, 2021

Let's disallow it for now. Same for local aliases of type parameters

Sounds good. Updated the issue accordingly.

Loading

gopherbot pushed a commit that referenced this issue Apr 20, 2021
Add handling for TypeParams in NewMethodSet, to bring it in sync with
lookupFieldOrMethod. Also add a test, since we had none. I wanted this
fix to get gopls completion working with type params, but due to the
subtlety of lookupFieldOrMethod, I left a TODO to confirm that there are
no behavioral differences between the APIs.

Updates #45639

Change-Id: I16723e16d4d944ca4ecb4d87fc196815abb6fcff
Reviewed-on: https://go-review.googlesource.com/c/go/+/311455
Trust: Robert Findley <rfindley@google.com>
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@findleyr findleyr self-assigned this May 30, 2021
@findleyr
Copy link
Contributor Author

@findleyr findleyr commented Jun 8, 2021

As discussed in #43621, the case of type P[T any] T is equivalently problematic to the local declaration type Y T. Renaming this issue to track both cases.

Loading

@findleyr findleyr changed the title go/types, cmd/compile/internal/types2: disallow local declarations and aliases of type parameters go/types, cmd/compile/internal/types2: disallow type parameters as RHS of type declarations Jun 8, 2021
@griesemer
Copy link
Contributor

@griesemer griesemer commented Jul 2, 2021

Per discussion with @iant, we do want to permit (non-local) generic type declarations of the form

type T[P C] P

because those are useful, for instance to add methods to a type P.

But we don't want to permit (local) type declarations of the form

type T P

where P is a type parameter of an enclosing function.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 2, 2021

Change https://golang.org/cl/332411 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: disallow type parameters as RHS of type declarations

Loading

gopherbot pushed a commit that referenced this issue Jul 7, 2021
…rameter as RHS of a type declaration

For #45639.

Change-Id: I20e331b04f464db81e916af75f70ec8ae73eb989
Reviewed-on: https://go-review.googlesource.com/c/go/+/332411
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 14, 2021

Per discussion with @iant, we do want to permit (non-local) generic type declarations of the form

type T[P C] P

because those are useful, for instance to add methods to a type P.

As noted at #43621 (comment), it's not safe/possible to add methods to pointer or interface types. So C needs to at least be constrained to not allow either of those in its type set (edit:) to allow declaring methods on T[P].

What are some of the examples of where this is useful? Would they not be equally useful/applicable if P was replaced with either struct { p P } or struct { p *P }?

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 16, 2021

/cc @ianlancetaylor (apologies to @ iant)

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 16, 2021

At top level defining a type as its own parameter is useful as seen, for example, in the code (still using old syntax) at https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#absolute-difference .

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 16, 2021

At top level defining a type as its own parameter is useful as seen, for example, in the code (still using old syntax) at https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#absolute-difference .

Thanks.

It seems like the consequence of disallowing type P[T ...] T would be that:

type ComplexAbs[T Complex] T

func (a ComplexAbs[T]) Abs() ComplexAbs[T] {
	d := math.Hypot(float64(real(a)), float64(imag(a)))
	return ComplexAbs[T](complex(d, 0))
}

func ComplexAbsDifference[T Complex](a, b T) T {
	return T(AbsDifference(ComplexAbs[T](a), ComplexAbs[T](b)))
}

needs to be written instead as:

type ComplexAbs[T Complex] struct{ X T }

func (a ComplexAbs[T]) Abs() ComplexAbs[T] {
	d := math.Hypot(float64(real(a.X)), float64(imag(a.X)))
	return ComplexAbs[T](complex(d, 0))
}

func ComplexAbsDifference[T Complex](a, b T) T {
	return T(AbsDifference(ComplexAbs[T]{a}, ComplexAbs[T]{b}))
}

That doesn't seem substantively different to me.

I'm still currently leaning for blanket disallowing type P[T C] T, at least in Go 1.18. Otherwise, I think we'll need to specify a lot of corner cases around handling/preventing promoted methods and also reflection API issues like #43621 (comment). The cases that make type P[T C] struct { T } tricky also apply to type P[T C] T.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 18, 2021

I don't feel strongly about it.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 19, 2021

On CL 294469, @ianlancetaylor wrote:

Yes, for "type L io.Writer", L has a method. But I am arguing that if we write

   func F[P io.Writer]() {
       type L P
   }

then L does not have a method. In this case, P has a Write method, and so does a value of type P. But if F is instantiated with a type like bytes.Buffer, then although a value of type P has a Write method, a value of type L should not have a Write method. Or so it seems to me.

My point is if you instantiate F[io.Writer] then regardless how we statically treat L at compile-time, we need to reconcile with the fact that at run-time it's going to be an interface type with a non-empty method set. In particular, I worry about how this affects users of package reflect, esp. in the presence of method promotion.

I think a reasonable rule could be something like "it's invalid to embed a type into a struct if its method set depends upon the type arguments used for instantiation," but that's fairly subtle both to specify and implement. I'd rather defer to post-1.18 to worry about specifying a rule like that, assuming there's user demand.

Loading

@randall77
Copy link
Contributor

@randall77 randall77 commented Jul 19, 2021

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 19, 2021

I'm not quite following you, sorry. My way of thinking is that compile-time and execution-time handling of type parameters are two different things. At compile time we have various complexities where we are trying to work through exactly which type arguments are permitted and exactly which operations are permitted on those type arguments. I don't think we're there yet when it comes to things like promoted methods.

At execution time, though, we know what we want: the function should act as though the type parameters are consistently replaced throughout by the type arguments. We know here are some cases there where we have to be different, as in unsafe.Sizeof can't always be a constant, and type switches must permit duplicate cases. But those are also by and large compilation-time details.

With this view, I'm not sure where confusion about method promotion arises. I'm not saying that it doesn't arise, I just don't see it yet.

It's worth noting that it may be the case that some code is too complicated for dictionaries. For some code we may have to fall back to stenciling. I think that's fine, assuming that that code is not common. It's also of course fine to postpone handling some cases entirely.

But I don't know why we would want a rule like "it's invalid to embed a type into a struct if its method set depends upon the type arguments used for instantiation." Maybe we do. And it's quite possible that we want a compile-time rule like "if the method set of an embedded type depends on type arguments, then those methods may not be called." But I'm not sure why we need a rule like this at execution time.

Loading

@danscales
Copy link

@danscales danscales commented Jul 20, 2021

I had shown this example to Keith and Robert a few weeks ago. I added it as a test in go/test/typeparam/boundmethod.go. It illustrates that the String method (enabled by the bound Stringer) in the generic function stringify() can be satisfied either by a method of a concrete type (myint) or of one of these instantiated types that adds methods (StringInt[myint]):

package main

import (
        "fmt"
        "reflect"
        "strconv"
)

type myint int

func (m myint) String() string {
        return strconv.Itoa(int(m))
}

type Stringer interface {
        String() string
}

func stringify[T Stringer](s []T) (ret []string) {
        for _, v := range s {
                ret = append(ret, v.String())
        }
        return ret
}

type StringInt[T any] T

func (m StringInt[T]) String() string {
        return "aa"
}

func main() {
        x := []myint{myint(1), myint(2), myint(3)}

        got := stringify(x)
        want := []string{"1", "2", "3"}
        if !reflect.DeepEqual(got, want) {
                panic(fmt.Sprintf("got %s, want %s", got, want))
        }

        x2 := []StringInt[myint]{StringInt[myint](1), StringInt[myint](2), StringInt[myint](3)}

        got2 := stringify(x2)
        want2 := []string{"aa", "aa", "aa"}
        if !reflect.DeepEqual(got2, want2) {
                panic(fmt.Sprintf("got %s, want %s", got2, want2))
        }
}

This is implementable for dictionaries, but does add a bit of complexity. In one case, v.String() is calling a method of a regular type, so doesn't need a dictionary argument, whereas in the other case, v.String() is calling a method of a generic type, so it does need a dictionary argument. One way to deal with this is to make sure that an instantiated (but concrete) type and a non-instantiated type are always in different gcshape groupings, even if they have exactly the same type underneath, so we will create separate shape functions for them (one which does not pass a dictionary arg for v.String() and one that creates and passes a appropriate dictionary arg for v.String())

Matthew's idea to disallow "type StringInt[T any] T" in favor of "type StringInt[T] struct { x T }" may be a good simplification for the semantics. But, it will not immediately simplify the implementation, since T and 'struct { x T }' currently have the same gcshape as we define them, and we will still need to distinguish cases where a bound is satisfied by a method of a concrete type vs. by a method of an instantiated type.

I'll add the struct case to the boundmethod.go test.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 20, 2021

My way of thinking is that compile-time and execution-time handling of type parameters are two different things.

Ack. My concern is they're not easily teased apart like that.

With this view, I'm not sure where confusion about method promotion arises. I'm not saying that it doesn't arise, I just don't see it yet.

What's the correct behavior for this program? https://go2goplay.golang.org/p/WIsSjVAVe25

Is it valid? If so, what should it print? If not, why is it invalid?

Same questions if the v.F = 3 line are uncommented.

(I'm using package-scope L[P] instead of function-scope L, because only unified IR supports the latter currently; but I think the same issues apply to either.)

But I don't know why we would want a rule like "it's invalid to embed a type into a struct if its method set depends upon the type arguments used for instantiation."

To disallow embedding L[P] in the example above. In #43621, we agreed to disallow embedding type parameter types (e.g., P) but L[P] is an instantiated defined type.

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 20, 2021

This is implementable for dictionaries, but does add a bit of complexity. In one case, v.String() is calling a method of a regular type, so doesn't need a dictionary argument, whereas in the other case, v.String() is calling a method of a generic type, so it does need a dictionary argument.

FWIW, this is why I was looking at using closure variables to pass dictionaries: then you just pass the dictionary unconditionally; the callee just ignores it if it's unneeded; and SSA can even optimize away the dictionary argument if after devirtualization it detects passing a context pointer to a non-NEEDCTXT function.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 20, 2021

What's the correct behavior for this program? https://go2goplay.golang.org/p/WIsSjVAVe25

Thanks for the example. I guess the concern here is that the field reference at compile time may not be the same as the field reference as returned by reflect.Value.FieldByName. Personally I don't have a problem with that. But now I understand where your suggestion of "it's invalid to embed a type into a struct if its method set depends upon the type arguments used for instantiation" comes from; thanks. We could also consider what might be a simpler rule: "if an embedded field is or uses a type parameter, it does not contribute any promoted fields or methods to the struct." Or, of course, "it's invalid to embed a type that is or uses a type parameter."

Loading

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jul 21, 2021

We could also consider what might be a simpler rule: "if an embedded field is or uses a type parameter, it does not contribute any promoted fields or methods to the struct."

How do reflect users distinguish at runtime whether a particular embedded field contributed fields/methods to the struct? I believe that requires extending the reflect API, and when I suggested that last month, you replied "That sounds like an argument that we should not permit embedding type parameters."

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jul 21, 2021

I'm still inclined to think that we should not permit embedding type parameters. And perhaps we should indeed consider not permitting embedding any type based on type parameters.

Loading

@heschi
Copy link
Contributor

@heschi heschi commented Sep 29, 2021

Ping -- this issue is marked as release blocking and hasn't had any activity in a while.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Sep 29, 2021

We're working on this - it's connected with other moving pieces. It will be trivial to disallow if we go that route.

Loading

@toothrot
Copy link
Contributor

@toothrot toothrot commented Oct 13, 2021

Friendly ping that this is a release-blocking issue for Go 1.18.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 13, 2021

Again, we are actively working on this.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Oct 27, 2021

The decision here is to disallow (for 1.18) type declarations of the form

type T[P xxx] P

and also

type T P

where P is a type parameter.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 27, 2021

Change https://golang.org/cl/359177 mentions this issue: cmd/compile/internal/types2: disallow lone type parameter on RHS of type declaration

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Oct 27, 2021

Change https://golang.org/cl/359274 mentions this issue: x/tools/cmd/stringer: adjust generics tests

Loading

gopherbot pushed a commit to golang/tools that referenced this issue Oct 28, 2021
Per the issue below, a lone type parameter is not permitted
on the the RHS of a type declaration, at least for Go 1.18.

Slightly modified the generics tests so that they pass for
now.

For golang/go#45639.

Change-Id: I3c5dc0ff65bfdc268c372e5e3fdfe00a8547f17e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359274
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
@gopherbot gopherbot closed this in a91d0b6 Oct 28, 2021
adamhassel pushed a commit to adamhassel/stringer that referenced this issue Nov 19, 2021
Per the issue below, a lone type parameter is not permitted
on the the RHS of a type declaration, at least for Go 1.18.

Slightly modified the generics tests so that they pass for
now.

For golang/go#45639.

Change-Id: I3c5dc0ff65bfdc268c372e5e3fdfe00a8547f17e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/359274
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants