From 2bc31ee9f4995d12523e60816c645a0ee5239b34 Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Tue, 10 Mar 2020 22:14:19 -0700 Subject: [PATCH] go/types: de-parameterize instantiated contracts when embedding them More precisely, de-parameterize the corresponding interfaces when embedding them in the corresponding type bounds (interfaces) for the embedding contract. This prevents the embedded (and already instantiated) interfaces from being instantiated again when the outer interface is instantiated and avoids breaking an assertion. See also the test case in testdata/issues.go2 for more details. Change-Id: I70c9354849eda0c8a36905d0b80f4d3031542f30 --- src/go/types/NOTES | 3 +- src/go/types/check_test.go | 1 + src/go/types/contracts.go | 18 ++++++++---- src/go/types/testdata/issues.go2 | 49 ++++++++++++++++++++++++++++++++ src/go/types/testdata/tmp.go2 | 2 +- 5 files changed, 65 insertions(+), 8 deletions(-) create mode 100644 src/go/types/testdata/issues.go2 diff --git a/src/go/types/NOTES b/src/go/types/NOTES index ecb80d62e5..387550846b 100644 --- a/src/go/types/NOTES +++ b/src/go/types/NOTES @@ -4,7 +4,8 @@ so we have a better track record. I only switched to this file in Nov 2019, henc ---------------------------------------------------------------------------------------------------- TODO -- review embedding of contracts with multiple and different numbers of arguments - likely incorrect +- review handling of fields of instantiated generic types (do we need to make them non-parameterized, + similar to what we did for the embedded interfaces created by contract embedding?) - debug (and error msg) printing of generic instantiated types needs some work - improve error messages! - use []*TypeParam for tparams in subst? (unclear) diff --git a/src/go/types/check_test.go b/src/go/types/check_test.go index eb894aa234..f8045e4615 100644 --- a/src/go/types/check_test.go +++ b/src/go/types/check_test.go @@ -105,6 +105,7 @@ var tests = [][]string{ {"testdata/typeinst.go2"}, {"testdata/typeinst2.go2"}, {"testdata/contracts.go2"}, + {"testdata/issues.go2"}, {"testdata/todos.go2"}, // Go 2 examples from design doc diff --git a/src/go/types/contracts.go b/src/go/types/contracts.go index 818ca05484..d99c87407a 100644 --- a/src/go/types/contracts.go +++ b/src/go/types/contracts.go @@ -135,15 +135,21 @@ func (check *Checker) contractDecl(obj *Contract, cdecl *ast.ContractSpec) { continue } - // add the instantiated bounds as embedded interfaces to the respective - // embedding (outer) contract bound - // TODO(gri) This seems incorrect. We must accomodate the situation where - // the embedded interfaces have different type parameters than - // the embedding interfaces. + // Add the instantiated bounds (ebound) as embedded interfaces to the respective + // embedding (outer) contract bound. Because the ebounds are already instantiated + // with the outer bound's type parameters, and because they are embedded, there + // is no need to keep them in instantiated form; in fact it will lead to problems + // if the outer bound is instantiated again later. We can just keep the ebound's + // underlying interface instead. + // (Alternatively one could set ebound.tparam to nil, thus marking it as not + // parameterized. But because ebound may be shared one would have to make a + // copy first. Since interface embedding doesn't care whether the embedded + // interface is named or not - one may embed an interface alias, after all - + // it is simpler to just use the underlying unnamed interface.) for i, ebound := range eobj.Bounds { index := targs[i].(*TypeParam).index iface := bounds[index].underlying.(*Interface) - iface.embeddeds = append(iface.embeddeds, ebound) + iface.embeddeds = append(iface.embeddeds, ebound.underlying) // don't use Named form of ebound check.posMap[iface] = append(check.posMap[iface], econtr.Pos()) // satisfy completeInterface requirements } continue // success diff --git a/src/go/types/testdata/issues.go2 b/src/go/types/testdata/issues.go2 new file mode 100644 index 0000000000..fc21520a0f --- /dev/null +++ b/src/go/types/testdata/issues.go2 @@ -0,0 +1,49 @@ +// Copyright 2020 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// This file contains regression tests for bugs found. + +package p + +// This used to crash with an assertion failure when +// instantiating f(int, int). The assertion was checking +// that the number of type parameters and arguments was +// matching for the embedded contract anInt (rather, its +// corresponding interface bound) but it really compared +// the numbers for anInt and twoInt (which embedds anInt). +// The fix simply uses the instantiated non-parameterized +// underlying interface of atInt rather than anInt. +contract anInt(T) { + T int +} + +contract twoInt(K, _) { + anInt(K) +} + +func f(type K, V twoInt)() + +func _ () { + f(int, int)() +} + +// This is the original (simplified) program causing the same issue. +contract comparable(T) { + T int, int8, int16, int32, int64, + uint, uint8, uint16, uint32, uint64, uintptr, + float32, float64, + string +} + +contract twocomparable(K, V) { + comparable(K) + comparable(V) +} + +func Equal(type K, V twocomparable)(m1, m2 map[K]V) + +func _() { + var m map[int]int + Equal(m, nil) +} diff --git a/src/go/types/testdata/tmp.go2 b/src/go/types/testdata/tmp.go2 index 261cf2cb52..4d9fe75062 100644 --- a/src/go/types/testdata/tmp.go2 +++ b/src/go/types/testdata/tmp.go2 @@ -15,5 +15,5 @@ contract twoInt(K, _) { func f(type K, V twoInt)() func _ () { - // f(int, int)() // this fails at the moment + f(int, int)() } \ No newline at end of file