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: self-reference within type parameter constraints #46461

Closed
mdempsky opened this issue May 30, 2021 · 16 comments
Closed

go/types: self-reference within type parameter constraints #46461

mdempsky opened this issue May 30, 2021 · 16 comments
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

mdempsky commented May 30, 2021

This test case sends go/types into an infinite recursion: https://go2goplay.golang.org/p/fpWup_pjUzi

type T[U interface{ M() T }] int

type X int
func (X) M() T[X] { return 0 }

Two questions:

  1. Are recursive type parameters like this supposed to work? If so, this has implications on extending the go/types API and export data format to support type parameters (e.g., see also proposal: go/types: lazy resolution of imported objects #46449 (comment)).

  2. Also if so, shouldn't interface{ M() T } actually be interface{ M() T[U] }? However, go/types explicitly rejects the latter.

/cc @griesemer @ianlancetaylor @findleyr @bcmills

@mdempsky mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 30, 2021
@mdempsky mdempsky added this to the Go1.18 milestone May 30, 2021
@mdempsky mdempsky changed the title go/types: self-reference within type parameters go/types: self-reference within type parameter constraints May 30, 2021
@findleyr
Copy link
Contributor

See also #45550, which is similar but manifests differently (stack overflow vs timeout).

My attempt to answer your questions:

  1. I'm not sure, but recursive constraints are sufficiently mind-bending that perhaps they should be explicitly disallowed.
  2. Yes, the use of the generic type T in a result parameter should be rejected. I suspect we never get to that point when type checking.

@ianlancetaylor
Copy link
Contributor

Certainly type T[U interface{ M() T }] int is incorrect.

It seems to me that type T[U interface{ M() T[U] }] int should be accepted. A constraint should be able to refer to the type being defined at least in cases where a type can refer to itself. I also think it's OK if we give an error for this for now.

@griesemer
Copy link
Contributor

types2 accepts

type T[U interface{ M() T[U] }] int

type X int
func (X) M() T[X] { return 0 }

which I believe is correct code and (eventually) should be accepted. Currently we have an endless recursion.

@griesemer griesemer self-assigned this Jun 2, 2021
@findleyr findleyr self-assigned this Jun 22, 2021
@griesemer
Copy link
Contributor

@mdempsky Is there anything left to do here?

The compiler now accepts

package p

type T[U interface{ M() T[U] }] int

type X int
func (X) M() T[X] { return 0 }

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/348738 mentions this issue: test: add compiler regress tests for #46461

@mdempsky
Copy link
Member Author

mdempsky commented Sep 9, 2021

@mdempsky Is there anything left to do here?

CL 348738 adds a test. It looks like types2 and unified IR handle this correctly now, but iexport fails with:

/home/mdempsky/wd/go/test/typeparam/issue46461b.dir/b.go:7:8: could not import ./a (cannot import "a" (type parameter bound more than once), possibly version skew - reinstall package)

/cc @danscales

@mdempsky
Copy link
Member Author

mdempsky commented Sep 9, 2021

Relatedly, this package still crashes types2:

package p

type A[T interface{ A[T] }] interface{}

More complex example:

package p

type A[U interface{ A[U] }] interface{ M() A[U] }

type I interface{ A[I]; M() A[I] }

Caveat: I'm not certain whether these should be valid, but the type checker should at least gracefully report an error if not. (That said, I also know from experience how painful it is dealing with recursive and/or embedded interfaces.)

gopherbot pushed a commit that referenced this issue Sep 9, 2021
gri@ reports that types2 now correctly handles when type parameters
recursively refer back to the parameterized type, so we might as well
add tests to exercise that. Unified IR also correctly handles
importing and exporting these types, but -G=3 currently does not.

Updates #46461.

Change-Id: I272102aa08c40c980b9aeeca9f834291dfbbcc3e
Reviewed-on: https://go-review.googlesource.com/c/go/+/348738
Trust: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@griesemer
Copy link
Contributor

I have a tentative fix for these, and they appear to be valid.

@cuonglm
Copy link
Member

cuonglm commented Sep 10, 2021

@mdempsky Is there anything left to do here?

CL 348738 adds a test. It looks like types2 and unified IR handle this correctly now, but iexport fails with:

/home/mdempsky/wd/go/test/typeparam/issue46461b.dir/b.go:7:8: could not import ./a (cannot import "a" (type parameter bound more than once), possibly version skew - reinstall package)

/cc @danscales

This seems issues #48280.

@mdempsky
Copy link
Member Author

This seems issues #48280.

Yeah, that does look similar.

@danscales
Copy link
Contributor

Do anyone know of a reasonable use case for a generic type that references itself in its own constraint?

If there is no obvious use case, I wonder if we should just disallow types that self-reference themselves in this way in Go 1.18, in order to cut down on complexity and the number of test cases. At the very least, seems like we should keep that as an option if the number of test cases multiplies.

cc @randall77

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/350330 mentions this issue: [dev.fuzz] all: merge master (b1bedc0) into dev.fuzz

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/350994 mentions this issue: [dev.fuzz] all: merge master (79159f2) into dev.fuzz

gopherbot pushed a commit that referenced this issue Sep 20, 2021
This now includes the fix in CL 350729, which means
we no longer need to skip the test in dev.fuzz.

Conflicts:

- src/cmd/compile/internal/noder/unified_test.go

Merge List:

+ 2021-09-20 af72ddf cmd/compile: extend dump-to-file to handle "genssa" (asm) case.
+ 2021-09-20 3c764ba cmd/go: write go.mod requirements more consistently for go 1.17+
+ 2021-09-20 6268468 cmd/link: generate DIE for types referenced only through dictionaries
+ 2021-09-20 6acac8b cmd/compile: delay all transforms for generic funcs/methods
+ 2021-09-20 988f18d go/types: export Named._Orig as Named.Origin
+ 2021-09-20 b6dddac cmd/compile: fix transform.AssignOp to deal with tricky case
+ 2021-09-20 9e60c37 cmd/compile: document register-based ABI for ppc64
+ 2021-09-20 79159f2 cmd/compile: fix simplification rules on arm/arm64
+ 2021-09-20 eff27e8 cmd/compile: ensure constant shift amounts are in range for arm
+ 2021-09-20 9ebe7c8 go/test: add a test for issue 48344
+ 2021-09-20 6f35430 cmd/compile: allow rotates to be merged with logical ops on arm64
+ 2021-09-20 2d9b486 cmd/compile: update doc at top of iexport.go on the changes for typeparams
+ 2021-09-20 a81b0dc cmd/compile: rename instType -> instanceType
+ 2021-09-20 1192135 cmd/cgo: remove hardcoded '-pie' ldflag for linux/arm
+ 2021-09-20 a83a558 cmd/compile: fix export/import of range loop.
+ 2021-09-19 315dbd1 cmd/compile: fold double negate on arm64
+ 2021-09-19 83b36ff cmd/compile: implement constant rotates on arm64
+ 2021-09-19 771b8ea cmd/compile: fix missing markHiddenClosureDead in deadcode pass
+ 2021-09-18 c894b44 net/rpc: remove warnings on incompatible methods at registration
+ 2021-09-17 4b654c0 cmd/compile: SSA ".this" variable
+ 2021-09-17 f01721e cmd/compile: remove self copies in tail-call wrappers
+ 2021-09-17 163871f time: re-add space-padded day of year to docs
+ 2021-09-17 ac7c347 time: support fractional timezone minutes in MarshalBinary
+ 2021-09-17 07b30a4 cmd/compile: delay transformAssign if lhs/rhs have typeparam
+ 2021-09-17 c10b980 cmd/compile: restore tail call for method wrappers
+ 2021-09-17 50e4508 cmd/compile: fix import/export of Init and Def fields.
+ 2021-09-17 3fa35b5 go/types: ensure that we always get a new signature in expandNamed
+ 2021-09-17 3fa7dbe cmd/go: fix GOARCH value in GOAMD64 docs
+ 2021-09-17 974b016 syscall: implement Pipe using pipe2 syscall on all linux platforms
+ 2021-09-17 1a49dcb syscall: remove //sysnb comment generating Setreuid for linux/arm64
+ 2021-09-17 cea7a71 cmd/compile: fix generic type handling in crawler
+ 2021-09-17 74e384f internal/poll: inject a hook into the runtime finalizer to count the closed pipes
+ 2021-09-17 323c6f7 log: don't format if writing to io.Discard
+ 2021-09-17 7f36ef0 cmd/compile/internal/noder: hide TestUnifiedCompare behind -cmp flag
+ 2021-09-17 70493b3 runtime/cgo: save and restore X3 (aka GP) for crosscall1 on riscv64
+ 2021-09-17 6d02ce8 runtime: fix prettyprinting of parametric types in gdb
+ 2021-09-17 6602c86 cmd/internal/obj/riscv: improve instruction validation
+ 2021-09-17 14e812b syscall: do not use handle lists on windows when NoInheritHandles is true
+ 2021-09-16 8d2a9c3 all: remove incorrectly repeated words in comments
+ 2021-09-16 af9da13 A+C: update name to real name and add to AUTHORS
+ 2021-09-16 265b59a cmd/cgo: for godefs, don't let field prefix removal cause duplicates
+ 2021-09-16 4efdaa7 testing: skip panics when picking the line number for decoration
+ 2021-09-16 e09dcc2 go/types, types2: add an additional shift test case
+ 2021-09-16 5402b43 spec: fix incorrect type in a shift example
+ 2021-09-16 d09e09b cmd/compile: fixing writebarrier.go for -G=3
+ 2021-09-16 bcdc61d cmd/compile: preserve statements better in expandCalls
+ 2021-09-16 48e2b1e cmd/compile: fix LocResults formatting
+ 2021-09-16 b1bedc0 cmd/go: add GOAMD64 environment variable
+ 2021-09-16 04f5116 cmd/go: clean paths before checking same directory
+ 2021-09-16 e7dbe39 cmd/cgo: add missing tab in exports for a result of void
+ 2021-09-15 cfa233d cmd/compile: remove unneeded early transforms, with dictionary change
+ 2021-09-15 59a9a03 cmd/compile: switch to computing dict format on instantiated functions
+ 2021-09-15 0edc6c4 cmd/internal/obj/ppc64: generate prologue code compatible with new ABI
+ 2021-09-15 03df68d runtime: fix setting of cpu features for amd64
+ 2021-09-15 6196979 cmd/go/internal/modload: prevent tidy downgrading disambiguating modules
+ 2021-09-15 72bb818 cmd/compile: emit DWARF info about dictionary entries
+ 2021-09-15 5b48fca cmd/compile: mark wrapper functions with DW_AT_trampoline
+ 2021-09-15 e4dfd78 go/internal/gcimporter,cmd/compile: minor clean-up in iimport.go
+ 2021-09-15 4847c47 cmd/compile/internal/types2: eliminate Named.instPos
+ 2021-09-15 3100f54 cmd/compile/internal/types2: merge Named type loading and expansion
+ 2021-09-15 738cebb cmd/compile/internal/types2: implement Identical for *Union types
+ 2021-09-15 b26d325 cmd/compile/internal/types2: remove some unnecessary loading/expansion of Named types
+ 2021-09-15 9fc2889 cmd/compile/internal/types2: export TypeHash, return value without blanks
+ 2021-09-15 2da3375 runtime: in adjustTimers back up as far as necessary
+ 2021-09-15 c7f2f51 cmd/go: remove subcommand prefix from error messages
+ 2021-09-15 0bb40b0 go/types: implement Identical for *Union types
+ 2021-09-15 cb4e1de go/types: minor cleanup of instantiation
+ 2021-09-15 a0f3129 go/types: instantiate methods when instantiating Named types
+ 2021-09-14 bf26e43 go/types: eliminate Named.instPos
+ 2021-09-14 2933c45 go/types: merge Named type loading and expansion
+ 2021-09-14 137543b cmd/compile: set IsShape based on type being in the Shapes pkg
+ 2021-09-14 3a72175 cmd/compile: fix test/typeparam/mdempsky/4.go for -G=3
+ 2021-09-14 b2c04f0 runtime: avoid loop variable capture in test
+ 2021-09-14 181e8cd go/internal/gcimporter: remove outdated comment
+ 2021-09-14 8699425 syscall: remove use of IN_KUBERNETES in test
+ 2021-09-14 b3c6de9 cmd/internal/obj/ppc64: allow VR register arguments to VS registers
+ 2021-09-14 ee91bb8 cmd/compile: prevent typecheck importer reading type parameter twice
+ 2021-09-14 2953cd0 go/internal/gcimporter: prevent importReader reading type parameter twice
+ 2021-09-14 b8c802b cmd/compile: prevent importReader reading type parameter twice
+ 2021-09-14 4a4221e all: remove some unused code
+ 2021-09-14 71adc65 runtime: change time.now to ABIInternal
+ 2021-09-14 146e8d4 reflect: use Value.Len instead of conversion to slice header
+ 2021-09-13 9a58aa2 spec: fix prose about terminating statements
+ 2021-09-13 42057e9 cmd/compile: save the note of fields when translating struct
+ 2021-09-13 960d036 cmd/go: add missing parenthesis in a call to "PrintVersion"
+ 2021-09-13 81a4fe6 cmd/link/internal/ld: re-enable DWARF tests on solaris/illumos
+ 2021-09-13 f93a63a reflect: add a floating point section to DeepEqual tests
+ 2021-09-13 a0c409c reflect: add fast paths for common, simple Kinds to DeepEqual
+ 2021-09-13 ac40c98 reflect: fix _faststr optimization
+ 2021-09-13 c8a58f2 cmd/go: add test to check for a potential workspace loading issue
+ 2021-09-13 e74e363 strings: add Clone function
+ 2021-09-13 bced369 cmd/link: minor code cleanup in dwarf gen
+ 2021-09-13 c3b217a cmd/go: document 'go install cmd@version' ignores vendor directories
+ 2021-09-12 ad97d20 go/types: remove some unnecessary loading/expansion of Named types
+ 2021-09-12 0d8a4bf bufio: add Writer.AvailableBuffer
+ 2021-09-11 23832ba reflect: optimize for maps with string keys
+ 2021-09-11 a50225a bufio: make Reader.Reset and Writer.Reset work on the zero value
+ 2021-09-10 cf2fe5d doc/asm: fix HTML markup
+ 2021-09-10 1bf2cd1 debug/elf: retain original error message when getSymbols fails.
+ 2021-09-10 5a4b9f9 time: reference -tags=timetzdata in testing panic
+ 2021-09-10 025308f testing: increase alternation precedence
+ 2021-09-10 5a94a90 cmd/compile/internal/types2: better error message for invalid array decls
+ 2021-09-10 da1aa65 cmd/compile/internal/syntax: correct follow token for type parameter lists
+ 2021-09-10 96ab854 cmd/compile/internal: better AST line highlight in ssa.html
+ 2021-09-10 90c5660 embed: guarantee the returned file of FS.Open implements io.Seeker
+ 2021-09-10 c69f5c0 cmd/compile: add support for Abs and Copysign intrinsics on riscv64
+ 2021-09-10 2091bd3 cmd/compile: simiplify arm64 bitfield optimizations
+ 2021-09-09 b32209d cmd/compile: fix test case for unified IR (fix build)
+ 2021-09-09 1a708bc cmd/compile: don't crash while reporting invalid alias cycle
+ 2021-09-09 426ff37 cmd/cgo, runtime/cgo: avoid GCC/clang conversion warnings
+ 2021-09-09 73483df cmd/compile/internal/syntax: better error message for missing type constraint
+ 2021-09-09 e1c3f21 time: propagate "," separator for fractional seconds into Format
+ 2021-09-09 c981874 cmd/compile: fix implement for closure in a global assignment
+ 2021-09-09 2c4f389 cmd/link: enable internal linker in more cases for ppc64le
+ 2021-09-09 fb84e99 test: add compiler regress tests for #46461
+ 2021-09-09 b9e1a24 cmd/compile: fix case where init info of OAS node is dropped
+ 2021-09-09 f9271e4 go/types, types2: rename RParams -> RecvTypeParams
+ 2021-09-09 ea43445 reflect: add hooks for dealing with narrow width floats
+ 2021-09-09 a53e3d5 net: deprecate (net.Error).Temporary
+ 2021-09-09 19457a5 cmd/compile: stenciled conversions might be NOPs
+ 2021-09-09 a295b3c test: re-enable AsmCheck tests for types2-based frontends
+ 2021-09-09 66f0d35 go/types: reduce number of delayed functions
+ 2021-09-09 d2a77f1 go/types: handle recursive type parameter constraints
+ 2021-09-09 9e1eea6 go/types: detect constraint type inference cycles
+ 2021-09-09 b86e8dd test/typeparam: fix issue48094b test build
+ 2021-09-09 c84f3a4 syscall: drop fallback to pipe in Pipe on linux/arm
+ 2021-09-09 376a079 cmd/compile: fix unified IR panic when expanding nested inline function
+ 2021-09-09 6edc579 internal/poll: report open fds when TestSplicePipePool fails
+ 2021-09-09 2481f6e cmd/compile: fix wrong instantiated type for embedded receiver
+ 2021-09-09 d62866e cmd/compile: move checkptr alignment to SSA generation
+ 2021-09-09 8fad81c cmd/compile: fold handling OCONV logic to separate function
+ 2021-09-09 9cbc76b cmd/internal/obj/arm64: add checks for incorrect use of REGTMP register
+ 2021-09-09 42563f8 cmd/compile: remove 'ext' fields from unified IR reader/writer types
+ 2021-09-09 4c52eac cmd/compile: simplify value coding for unified IR
+ 2021-09-09 e30a090 cmd/compile: extrapolate $GOROOT in unified IR
+ 2021-09-08 a1f6208 go/types, types2: add Environment to Config
+ 2021-09-08 f5f8a91 cmd/compile/internal/types2: spell out 'Type' in type parameter APIs
+ 2021-09-08 bff39cf cmd/compile: add automated rewrite cycle detection
+ 2021-09-08 b61e1ed cmd/compile/internal/types2: temporarily pin the Checker to Interface during checking
+ 2021-09-08 47f3e1e cmd/compile/internal/types2: move NewTypeParam off of Checker
+ 2021-09-08 ccc927b cmd/compile/internal/types2: move typeHash to environment.go
+ 2021-09-08 30e9bfb cmd/compile/internal/types2: implement deduplication of instances using the Environment
+ 2021-09-08 0406d3a go/ast: rename MultiIndexExpr to IndexListExpr

Change-Id: I7f917d45b0507c122c212305144b0b455618ff54
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/351969 mentions this issue: cmd/compile/internal/types2: delay union element checks

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/351971 mentions this issue: go/types: delay union element checks

gopherbot pushed a commit that referenced this issue Sep 24, 2021
This is a clean port of CL 351969 from types2 to go/types
with a minor adjustment for error handling (provide an error
code).

For #46461.

Change-Id: I493dde12d8ccf86aa33f4913ac6e82f2eb459088
Reviewed-on: https://go-review.googlesource.com/c/go/+/351971
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/361774 mentions this issue: [dev.boringcrypto] all: merge master (fa16efb) into dev.boringcrypto

@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 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

7 participants