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

types2, go/types: repeated identical external instantiations of the same type will lead to different types #47103

Closed
griesemer opened this issue Jul 9, 2021 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@griesemer
Copy link
Contributor

Reminder issue:

Because we don't have a global type map for external
instantiations, instantiating the same type twice, independently but
with the same type arguments, will result in two different types. This
is not correct. We need to provide some form of context for external
instantiations (which means the importers). This is a separate issue,
not yet addressed.

cc: @findleyr, @danscales @mdempsky

@griesemer griesemer added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Jul 9, 2021
@griesemer griesemer added this to the Go1.18 milestone Jul 9, 2021
@griesemer griesemer self-assigned this Jul 9, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/333383 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: recursive substitution must terminate (bug fix)

@CRYPTOENFORCER

This comment has been minimized.

@mdempsky
Copy link
Member

mdempsky commented Jul 9, 2021

Is this still an issue with the InstantiateLazy API? That already requires passing in the Checker as an argument, I think to allow deduplication.

@griesemer
Copy link
Contributor Author

griesemer commented Jul 9, 2021

No, this is not an issue with InstantiateLazy, only with Instantiate which doesn't provide a *Checker or any other kind of context/environment.

gopherbot pushed a commit that referenced this issue Jul 9, 2021
…must terminate (bug fix)

When types2.Instantiate is called externally, no *Checker is provided and
substitution doesn't have access to Checker.typMap; and instantiation of
recursive generic types leads to an infinite recursion in subst.

There was a local subster.cache but it was only set and never used.
Replaced subster.cache with subster.typMap, which is set to the global
Checker.typMap if available, and set to a local map otherwise. This
prevents such infinite recursions. Added a simple test.

More generally, because we don't have a global type map for external
instantiations, instantiating the same type twice, independently but
with the same type arguments, will result in two different types. This
is not correct. We need to provide some form of context for external
instantiations (which means the importers). This is a separate but
related issue which is not yet addressed (filed #47103).

Change-Id: I541556c677db54f7396fd0c88c7467894dfcf2e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/333383
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>
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/333569 mentions this issue: [dev.typeparams] cmd/compile/internal/types2: replace types2.Instantiate with Checker.Instantiate

gopherbot pushed a commit that referenced this issue Jul 13, 2021
…ate with Checker.Instantiate

Allow Checker.Instantiate to work with a nil *Checker receiver
(for now). This opens the door to passing in a *Checker at all
times.

Also, added a verify flag to Instantiate, InstantiateLazy, and
instance, to be able to control if constraint satisfaction should
be checked or not.

Removed types2.Instantiate.

For #47103.

Change-Id: Ie00ce41b3e50a0fc4341e013922e5f874276d282
Reviewed-on: https://go-review.googlesource.com/c/go/+/333569
Trust: Robert Griesemer <gri@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
@griesemer
Copy link
Contributor Author

@findleyr Is there anything left to do here now that we have the global Environment/Context?

@findleyr
Copy link
Contributor

findleyr commented Sep 9, 2021

I do think we need more testing. Want to repurpose this issue, or shall I open a new one?

@toothrot
Copy link
Contributor

@griesemer @findleyr Checking in as this is labeled a release-blocker for Go 1.18. Is there anything left on this issue?

@findleyr
Copy link
Contributor

I think we need to also cover repeated instantiations of *Signature types, which are not currently handled. So this should remain as a release blocker.

@findleyr findleyr self-assigned this Sep 22, 2021
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/362801 mentions this issue: go/types: deduplicate signatures with the context

gopherbot pushed a commit that referenced this issue Nov 13, 2021
Extend the type checking context to allow de-duplicating *Signature
instances, in addition to *Named instances.

Naively we would deduplicate instances of different-but-identical origin
*Signature types. That may be OK, but it seems a bit strange to get the
same signature when instantiating two different functions. For now,
differentiate *Signature types by prepending a unique identifier for the
origin pointer, thus guaranteeing that instances de-duplicated if they
come from the exact same (pointer identical) origin type.

Updates #47103

Change-Id: I93cc3cacad195267fe0a5801f9c5a3b1e61eb907
Reviewed-on: https://go-review.googlesource.com/c/go/+/362801
Trust: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Griesemer <gri@golang.org>
@findleyr
Copy link
Contributor

findleyr commented Nov 13, 2021

I am satisfied with this for 1.18. We may add additional testing during the freeze, but this is complete.

EDIT: these changes have landed in go/types, and will be in types2 shortly. Our convention is to close the issue once the initial CLs land, as we have external means to track ports.

@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. release-blocker
Projects
None yet
Development

No branches or pull requests

6 participants