From 70be636e2eb3e5144dab16cae62162420a4fe5a8 Mon Sep 17 00:00:00 2001 From: Romain Marcadier Date: Wed, 26 Jan 2022 19:56:50 +0100 Subject: [PATCH] fix(go): unable to reuse instances between child/parent interfaces (#3321) The go runtime was not tracking aliases in a way that enabled it to return an alternate proxy to an object instance when that had previously been returned as a different, singular interface. This change keeps tack of the "first ever" registered instance against a given InstanceID (as this is likely the one that was natively constructed), but also to all proxy aliases to the same InstanceID, so that it can later return an existing proxy that can be converted to a desired type. Fixes https://github.com/aws/jsii/issues/2688 --- By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license]. [Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0 --- .../specification/6-compliance-report.md | 4 +- .../project/compliance_test.go | 1 - .../internal/kernel/conversions.go | 2 +- .../internal/objectstore/objectstore.go | 52 ++++++++++++++++--- 4 files changed, 47 insertions(+), 12 deletions(-) diff --git a/gh-pages/content/specification/6-compliance-report.md b/gh-pages/content/specification/6-compliance-report.md index b68e816b40..ee54cee7a2 100644 --- a/gh-pages/content/specification/6-compliance-report.md +++ b/gh-pages/content/specification/6-compliance-report.md @@ -5,7 +5,7 @@ This section details the current state of each language binding with respect to our standard compliance suite. -| number | test | java (98.33%) | golang (78.33%) | Dotnet | Python | +| number | test | java (98.33%) | golang (79.17%) | Dotnet | Python | | ------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | -------------------------------------------- | ------ | ------ | | 1 | asyncOverrides_overrideCallsSuper | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) | ⭕ | ⭕ | | 2 | [arrayReturnedByMethodCanBeRead]("Array created in the kernel can be queried for its elements") | 🟢 | 🟢 | ⭕ | ⭕ | @@ -42,7 +42,7 @@ This section details the current state of each language binding with respect to | 33 | testLiteralInterface | 🟢 | 🟢 | ⭕ | ⭕ | | 34 | structs_nonOptionalhashCode | 🟢 | ⚪ | ⭕ | ⭕ | | 35 | propertyOverrides_set_throws | 🟢 | 🟢 | ⭕ | ⭕ | -| 36 | canLeverageIndirectInterfacePolymorphism | 🟢 | [🔴](https://github.com/aws/jsii/issues/2688) | ⭕ | ⭕ | +| 36 | canLeverageIndirectInterfacePolymorphism | 🟢 | 🟢 | ⭕ | ⭕ | | 37 | fluentApi | 🟢 | ⚪ | ⭕ | ⭕ | | 38 | staticListInClassCanBeReadCorrectly | 🟢 | 🟢 | ⭕ | ⭕ | | 39 | mapReturnedByMethodCannotBeModified | 🟢 | ⚪ | ⭕ | ⭕ | diff --git a/packages/@jsii/go-runtime-test/project/compliance_test.go b/packages/@jsii/go-runtime-test/project/compliance_test.go index afff67374c..529f596476 100644 --- a/packages/@jsii/go-runtime-test/project/compliance_test.go +++ b/packages/@jsii/go-runtime-test/project/compliance_test.go @@ -1462,7 +1462,6 @@ func (suite *ComplianceSuite) TestCanLeverageIndirectInterfacePolymorphism() { require := suite.Require() require.Equal(float64(1337), *provider.ProvideAsClass().Value()) - suite.FailTest("Unable to reuse instances between parent/child interfaces", "https://github.com/aws/jsii/issues/2688") require.Equal(float64(1337), *provider.ProvideAsInterface().Value()) require.Equal("to implement", *provider.ProvideAsInterface().Verb()) } diff --git a/packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/conversions.go b/packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/conversions.go index 16896047f4..1292bf0772 100644 --- a/packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/conversions.go +++ b/packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/conversions.go @@ -64,7 +64,7 @@ func (c *Client) castAndSetToPtr(ptr reflect.Value, data reflect.Value) { } // If it's currently tracked, return the current instance - if object, ok := c.objects.GetObject(ref.InstanceID); ok { + if object, ok := c.objects.GetObjectAs(ref.InstanceID, ptr.Type()); ok { ptr.Set(object) return } diff --git a/packages/@jsii/go-runtime/jsii-runtime-go/internal/objectstore/objectstore.go b/packages/@jsii/go-runtime/jsii-runtime-go/internal/objectstore/objectstore.go index f15828ac67..8e7ad217f5 100644 --- a/packages/@jsii/go-runtime/jsii-runtime-go/internal/objectstore/objectstore.go +++ b/packages/@jsii/go-runtime/jsii-runtime-go/internal/objectstore/objectstore.go @@ -22,11 +22,16 @@ type ObjectStore struct { // passed to the Register method. objectToID map[uintptr]string - // idToObject associates an instanceID with the reflect.Value that - // represents the top-level object that was registered with the instanceID - // via the Register method. + // idToObject associates an instanceID with the first reflect.Value instance + // that represents the top-level object that was registered with the + // instanceID first via the Register method. idToObject map[string]reflect.Value + // idToObjects associates an instanceID with the reflect.Value instances that + // represent the top-level objects that were registered with the instanceID + // via the Register method. + idToObjects map[string]map[reflect.Value]struct{} + // idToInterfaces associates an instanceID with the set of interfaces that it // is known to implement. // @@ -40,6 +45,7 @@ func New() *ObjectStore { return &ObjectStore{ objectToID: make(map[uintptr]string), idToObject: make(map[string]reflect.Value), + idToObjects: make(map[string]map[reflect.Value]struct{}), idToInterfaces: make(map[string]stringSet), } } @@ -73,15 +79,17 @@ func (o *ObjectStore) Register(value reflect.Value, objectRef api.ObjectRef) err aliases := findAliases(value) - if existing, found := o.idToObject[objectRef.InstanceID]; found { - if existing == value { + if existing, found := o.idToObjects[objectRef.InstanceID]; found { + if _, found := existing[value]; found { o.mergeInterfaces(objectRef) return nil } // Value already exists (e.g: a constructor made a callback with "this" - // passed as an argument). We make the current value an alias of the new + // passed as an argument). We make the current value(s) an alias of the new // one. - aliases = append(aliases, existing) + for existing := range existing { + aliases = append(aliases, existing) + } } for _, alias := range aliases { @@ -92,7 +100,14 @@ func (o *ObjectStore) Register(value reflect.Value, objectRef api.ObjectRef) err } o.objectToID[ptr] = objectRef.InstanceID - o.idToObject[objectRef.InstanceID] = value + // Only add to idToObject if this is the first time this InstanceID is registered + if _, found := o.idToObject[objectRef.InstanceID]; !found { + o.idToObject[objectRef.InstanceID] = value + } + if _, found := o.idToObjects[objectRef.InstanceID]; !found { + o.idToObjects[objectRef.InstanceID] = make(map[reflect.Value]struct{}) + } + o.idToObjects[objectRef.InstanceID][value] = struct{}{} for _, alias := range aliases { o.objectToID[alias.Pointer()] = objectRef.InstanceID } @@ -169,6 +184,27 @@ func (o *ObjectStore) GetObject(instanceID string) (value reflect.Value, found b return } +// GetObjectAs attempts to retrieve the object value associated with the given +// instanceID, compatible with the given type. Returns the existing value and a +// boolean informing whether a value was associated with this instanceID and +// compatible with this type or not. +// +// The GetObjectAs method is safe to call with an instanceID that was never +// registered with the ObjectStore. +func (o *ObjectStore) GetObjectAs(instanceID string, typ reflect.Type) (value reflect.Value, found bool) { + found = false + if values, exists := o.idToObjects[instanceID]; exists { + for value = range values { + if value.CanConvert(typ) { + value = value.Convert(typ) + found = true + return + } + } + } + return +} + // canonicalValue ensures the same reference is always considered for object // identity (especially in maps), so that we don't get surprised by pointer to // struct versus struct value versus opaque interface value, etc...