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, types2: type inference should report a better error message #48619

Closed
eliben opened this issue Sep 24, 2021 · 13 comments
Closed

go/types, types2: type inference should report a better error message #48619

eliben opened this issue Sep 24, 2021 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@eliben
Copy link
Member

eliben commented Sep 24, 2021

What version of Go are you using (go version)?

$  gotip version
go version devel go1.18-fe8347b Fri Sep 24 10:51:48 2021 +0000 linux/amd64

What did you do?

Trying to compile this sample: https://go2goplay.golang.org/p/MAplKhfhuKs

What did you expect to see?

The sample is incomplete (I tried to minimize it), so I don't expect it to compile. At the very least, there are calls to functions that don't exist.

The code is also incorrect. In the signature of quickSort_gen_func, a, b should be int, not Elem. Fixing this, the compiler produces an expected error message instead of going into infinite recursion.

What did you see instead?

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0xc0205e0318 stack=[0xc0205e0000, 0xc0405e0000]
fatal error: stack overflow

runtime stack:
runtime.throw(0x6aa2fc, 0xe)
	/usr/local/go-faketime/src/runtime/panic.go:1126 +0x74
runtime.newstack()
	/usr/local/go-faketime/src/runtime/stack.go:1069 +0x7de
runtime.morestack()
	/usr/local/go-faketime/src/runtime/asm_amd64.s:436 +0x8b

goroutine 1 [running]:
go/types.(*unifier).nifyEq(0xc00058a140, 0x6f32a0, 0xc0000105a0, 0x6f30e8, 0x817000, 0x0, 0x0)
	/usr/local/go-faketime/src/go/types/unify.go:197 +0xb8 fp=0xc0205e0328 sp=0xc0205e0320 pc=0x5a6c18
go/types.(*unifier).nify(0xc00058a140, 0x6f32a0, 0xc0000105a0, 0x6f30e8, 0x817000, 0x0, 0x0)
	/usr/local/go-faketime/src/go/types/unify.go:238 +0x10f6 fp=0xc0205e0488 sp=0xc0205e0328 pc=0x5a7d16
go/types.(*unifier).nifyEq(0xc00058a140, 0x6f32a0, 0xc0000105a0, 0x6f30e8, 0x817000, 0x0, 0x0)
	/usr/local/go-faketime/src/go/types/unify.go:198 +0x70 fp=0xc0205e04d0 sp=0xc0205e0488 pc=0x5a6bd0
go/types.(*unifier).nify(0xc00058a140, 0x6f32a0, 0xc0000105a0, 0x6f30e8, 0x817000, 0x0, 0x0)
	/usr/local/go-faketime/src/go/types/unify.go:238 +0x10f6 fp=0xc0205e0630 sp=0xc0205e04d0 pc=0x5a7d16
...
<SNIP>
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 25, 2021
@griesemer griesemer added this to the Go1.18 milestone Sep 25, 2021
@gopherbot gopherbot removed the NeedsFix The path to resolution is known, but the work has not been done. label Sep 25, 2021
@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Sep 25, 2021
@griesemer
Copy link
Contributor

griesemer commented Sep 25, 2021

Smaller reproducer:

package p

func f[P any](a, _ P) {
	f(a, int(0))
}

Compiling leads to an infinite recursion in type inference.

@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/352832 mentions this issue: cmd/compile/internal/types2: avoid infinite recursion in unification

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353029 mentions this issue: go/types: avoid infinite recursion in unification

gopherbot pushed a commit that referenced this issue Sep 29, 2021
This is an almost clean port of CL 352832 from types2 to go/types:
The nest files and unify.go where copied verbatim; unify.go was
adjusted with correct package name, a slightly different comment
was restored to what it was. The test files got adjustments for
error position. infer.go got a missing _Todo error code.

For #48619.
For #48656.

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

Change https://golang.org/cl/354690 mentions this issue: cmd/compile/internal/types2: fix unification (again)

@griesemer
Copy link
Contributor

The previous fix attempt was incorrect. Re-opening.

@griesemer griesemer reopened this Oct 8, 2021
gopherbot pushed a commit that referenced this issue Oct 8, 2021
…"fix"

The "fix" (CL 352832) for #48619 was incorrect and broke
the unification algorithm in some cases (e.g., #48695).

This CL reverts the changes made by CL 352832 to unify.go,
and comments out code in corresponding tests.

As a result, #48695 will be fixed, and we will re-open #48619.

Fixes #48695.
For #48619.
For #48656.

Change-Id: I91bc492062dbcc8dae7626f6b33f6dfabf48bcb8
Reviewed-on: https://go-review.googlesource.com/c/go/+/354690
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer griesemer added the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Nov 10, 2021
@cherrymui cherrymui removed the okay-after-beta1 Used by release team to mark a release-blocker issue as okay to resolve either before or after beta1 label Dec 14, 2021
@cherrymui
Copy link
Member

Any update on this? Thanks.
(checking in as this is a release blocker.)

@findleyr
Copy link
Contributor

I de-duped #50273 to this issue. Once this is fixed we should confirm that it also resolves the repro there.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/377954 mentions this issue: go/types, types2: prevent unification from recursing endlessly

gopherbot pushed a commit that referenced this issue Jan 12, 2022
This is a stop gap solution to avoid panics due to stack overflow
during type unification. While this doesn't address the underlying
issues (for which we are still investigating the correct approach),
it prevents a panic during compilation and reports a (possibly not
quite correct) error message.

If the programs are correct in the first place, manually providing
the desired type arguments is a viable work-around, resulting in
code that will continue to work even when the issues here are fixed
satisfactorily.

For #48619.
For #48656.

Change-Id: I13bb14552b38b4170b5a1b820e3172d88ff656ec
Reviewed-on: https://go-review.googlesource.com/c/go/+/377954
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@griesemer
Copy link
Contributor

With the above CL this doesn't crash anymore. Still needs a proper fix but not a release blocker anymore.

@griesemer griesemer changed the title go/types, types2: stack overflow while type-checking generic code go/types, types2: type inference should report a better error message Jan 12, 2022
SilverRainZ added a commit to SilverRainZ/exp that referenced this issue Jan 27, 2022
Due to golang/go#48619, constraints of quickSortOrdered and its
related functions are not changed. So cast S to []E for now.
SilverRainZ added a commit to SilverRainZ/exp that referenced this issue Jan 27, 2022
Like what we done in exp/maps, exp/slices would be better to accept a
type whose underlying type is slice.

Due to golang/go#48619, constraints of quickSortOrdered and its
related functions are not changed. So cast S to []E for now.
SilverRainZ added a commit to SilverRainZ/exp that referenced this issue Jan 27, 2022
Like what we done in exp/maps, exp/slices would be better to accept a
type whose underlying type is slice.

Due to golang/go#48619, constraints of quickSortOrdered and its
related functions are not changed. So cast S to []E for now.
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/381274 mentions this issue: slices: relax the constraints to type whose underlying type is slice

@griesemer
Copy link
Contributor

This produces an ok error message and doesn't crash. Leaving for 1.19.

@griesemer griesemer modified the milestones: Go1.18, Go1.19 Feb 1, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/385494 mentions this issue: go/types, types2: avoid infinitely recursive instantiation

gopherbot pushed a commit that referenced this issue Feb 14, 2022
Type inference uses type parameter pointer identity to keep track of the
correspondence between type parameters and type arguments. However, this
technique can misidentify type parameters that are used in explicit type
arguments or function arguments, as in the recursive instantiation
below:

  func f[P *Q, Q any](p P, q Q) {
  	f[P]
  }

In this example, the fact that the P used in the instantation f[P] has
the same pointer identity as the P we are trying to solve for via
unification is coincidental: there is nothing special about recursive
calls that should cause them to conflate the identity of type arguments
with type parameters. To put it another way: any such self-recursive
call is equivalent to a mutually recursive call, which does not run into
any problems of type parameter identity. For example, the following code
is equivalent to the code above.

  func f[P interface{*Q}, Q any](p P, q Q) {
  	f2[P]
  }

  func f2[P interface{*Q}, Q any](p P, q Q) {
  	f[P]
  }

We can turn the first example into the second example by renaming type
parameters in the original signature to give them a new identity. This
CL does this for self-recursive instantiations.

Fixes #51158
Fixes #48656
Updates #48619

Change-Id: I54fe37f2a79c9d98950cf6a3602335db2896dc24
Reviewed-on: https://go-review.googlesource.com/c/go/+/385494
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@griesemer
Copy link
Contributor

The type inference algorithm doesn't run into endless recursions anymore since 1.18, and for 1.19 we have disabled the catch-all in case of an infinite recursion and haven't seen any problems so far.

For the simple reproducer

package p

func f[P any](a, _ P) {
	f(a, int(0))
}

the error message is:

testdata/manual.go:4:7: type int of int(0) does not match inferred type P for P

which is exactly right (barring more information, the 2nd argument's type must be P) , and at the correct location.

There's nothing else to do here for now. Closing.

jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
This is a stop gap solution to avoid panics due to stack overflow
during type unification. While this doesn't address the underlying
issues (for which we are still investigating the correct approach),
it prevents a panic during compilation and reports a (possibly not
quite correct) error message.

If the programs are correct in the first place, manually providing
the desired type arguments is a viable work-around, resulting in
code that will continue to work even when the issues here are fixed
satisfactorily.

For golang#48619.
For golang#48656.

Change-Id: I13bb14552b38b4170b5a1b820e3172d88ff656ec
Reviewed-on: https://go-review.googlesource.com/c/go/+/377954
Trust: Robert Griesemer <gri@golang.org>
Run-TryBot: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
@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 NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants