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

fix(go): unable to reuse instances between child/parent interfaces #3321

Merged
merged 2 commits into from Jan 26, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions gh-pages/content/specification/6-compliance-report.md
Expand Up @@ -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") | 🟢 | 🟢 | ⭕ | ⭕ |
Expand Down Expand Up @@ -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 | 🟢 | ⚪ | ⭕ | ⭕ |
Expand Down
1 change: 0 additions & 1 deletion packages/@jsii/go-runtime-test/project/compliance_test.go
Expand Up @@ -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())
}
Expand Down
Expand Up @@ -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
}
Expand Down
Expand Up @@ -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.
//
Expand All @@ -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),
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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...
Expand Down