[cxx-interop] Fix ambiguous methods in long chains of inheritance

[susmonteiro]

Let's suppose we have 3 structs, where S3 inherits from S2 and S2 inherits from S1. If S1 has a method foo, that is not overridden by their children structs, we would previously clone this method in S3 twice: once from S1 (the original method) and the other from S2 (which would be a clone of a clone). This resulted in 2 definitions of the method foo in S3, which would be diagnosed as ambiguous methods.

This fixes the above problem by making sure that we never clone a method that was already a clone.

rdar://126229717

[susmonteiro]

@swift-ci please smoke test

[susmonteiro]

@swift-ci please smoke test

[susmonteiro]

@swift-ci please smoke test

[hnrklssn]

Could you explain why the current check in importBaseMemberDecl does not prevent this already? I'm not familiar with the method import process

[hnrklssn]

clonedBaseMembers is a DenseMap. Is there no way to do a lookup rather than linear scan over the values here?

[susmonteiro]

The problem is that the key of clonedBaseMembers is a std::pair<origDecl, newContext>, where origDecl is the original method that was cloned and newContext is the record origDecl was cloned into. The value is the cloned method of origDecl in newContext.
In this case, we want to check if the method decl was cloned or if it is original. In other words, if the method decl is present in clonedBaseMembers. As far as I know, there isn't a way to lookup by value in a DenseMap and the only way to obtain the original method of a cloned decl is to lookup in clonedBaseMembers, or find all the methods in the base classes with the same name as decl and check if any of them is present in clonedBaseMembers.

Having said that, I don't love this solution, so I'm open to suggestions. I guess the alternative is to:

• use a different data structure
• have an additional data structure, where this search would be more efficient

However, most times we are searching by key (in importBaseMemberDecl), hence this solution.

[hnrklssn]

I can think of a few options:

• add an isClonedFromBaseClass bit inside ValueDecl that we can check here. For this use case it doesn't matter where it was cloned from, but I don't know what effect adding another bit field to every ValueDecl, regardless of whether it was cloned or not, would have.
• record the equivalent information using a set in the clang importer
• inside importBaseMemberDecl, when we add clonedBaseMembers.insert({{decl, newContext, cloned}), we could also add clonedBaseMembers.insert({{cloned, newContext}, cloned}). I can't think of a reason we wouldn't have access to newContext in isClonedBaseMemberDecl, but clonedBaseMembers.insert({{decl, nullptr}, cloned}) could work as well I guess. This is essentially just a way to repurpose the map as a set data structure - not sure if that's better or worse than introducing a separate, explicit set.

[egorzhdan]

record the equivalent information using a set in the clang importer

I think this would be the easiest approach.

[susmonteiro]

Thanks for the suggestions! All these ideas sound good to me, but they prioritize time efficiency at the expense of memory and I'm not sure what's better in this case. Having said that, I prefer the second option instead of adding a field to all ValueDecls.

I can't think of a reason we wouldn't have access to newContext in isClonedBaseMemberDecl

We do have access to newContext. We don't have access to the original decl, but your solution wouldn't need it anyways. Since clonedBaseMembers is only used here, I guess we can repurpose clonedBaseMembers and this is also probably less bug-prone than having two different data structures and always having to update both. What do you think?

[Xazax-hun]

Do we know why we don't have a clang node for the cloned base member?
If we had a Clang node we could check if the parent of the member (the containing record decl) is the same as the record for which we want to clone this member.

[susmonteiro]

@swift-ci please smoke test

[susmonteiro]

Could you explain why the current check in importBaseMemberDecl does not prevent this already? I'm not familiar with the method import process

Sure! Considering the example in the description where S3 : S2 : S1 and only S1 has the method foo. In the first few calls to importBaseMemberDecl, we start by cloning the method foo from S1 into S2 and then the method foo from S1 into S3. After this clonedBaseMembers will be the following:

{
  (S1_foo, S2) : S2_foo_cloned,
  (S1_foo, S3) : S3_foo_cloned
}

(S1_foo, S2_foo_cloned, S3_foo_cloned are all called foo, these names are just to distinguish them in this example)

The thing is that, when we clone these methods, we add them to the swift decl of S2 and S3. That means that, at some point, we will call importBaseMemberDecl again for the method S2_foo_cloned to be cloned into S3. Since [S2_foo_cloned, S3] is not present in clonedBaseMembers, we end up creating a new clone of the original method foo, which is exactly the same as S3_foo_cloned, except for its context. Since the constraint system has no way to choose between these two methods, we run into an ambiguous use of method foo from S3.

[hnrklssn]

Could you explain why the current check in importBaseMemberDecl does not prevent this already? I'm not familiar with the method import process

Sure! Considering the example in the description where S3 : S2 : S1 and only S1 has the method foo. In the first few calls to importBaseMemberDecl, we start by cloning the method foo from S1 into S2 and then the method foo from S1 into S3. After this clonedBaseMembers will be the following:

{
  (S1_foo, S2) : S2_foo_cloned,
  (S1_foo, S3) : S3_foo_cloned
}

(S1_foo, S2_foo_cloned, S3_foo_cloned are all called foo, these names are just to distinguish them in this example)

The thing is that, when we clone these methods, we add them to the swift decl of S2 and S3. That means that, at some point, we will call importBaseMemberDecl again for the method S2_foo_cloned to be cloned into S3. Since [S2_foo_cloned, S3] is not present in clonedBaseMembers, we end up creating a new clone of the original method foo, which is exactly the same as S3_foo_cloned, except for its context. Since the constraint system has no way to choose between these two methods, we run into an ambiguous use of method foo from S3.

Thanks, that clarifies things!

[egorzhdan]

Thanks, I have similar thoughts to @hnrklssn regarding the dictionary lookup performance. Otherwise, this looks good to me.

[egorzhdan]

record the equivalent information using a set in the clang importer

I think this would be the easiest approach.

[susmonteiro]

record the equivalent information using a set in the clang importer

I think this would be the easiest approach.

Oh I didn't see this. Do you prefer this one over storing everything in the same data structure? I'm concerned it might be the cause of bugs in the future if we ever want to use clonedBaseMembers in a context, as we would have to keep them synchronized.

For the third option, we could call the DenseMap clonedMembers instead and have both {{decl, newContext}, cloned} for the original methods and {{cloned, newContext}, nullptr} for the cloned methods. Then when importBaseMemberDecl is called, we lookup once in this data structure. If the key is found but the result is nullptr, then we return from the function without cloning anything. Although lookups might be more efficient if we have two different structures with a smaller size each...

[egorzhdan]

@susmonteiro if it's possible to restructure the DenseMap to keep everything in one data structure, in a way that lookups would still be fast, that would be the best solution. If not, I'd say it's fine to add another DenseSet to store the cloned members.

[susmonteiro]

@hnrklssn @egorzhdan I implemented your feedback. One comment though: I decided to keep the check in a separate isClonedMemberDecl function, because this check is only required in one specific case, which is when we try to import inheriting members such as subscripts. In all other cases, we iterate through the original clang members and consequently they won't be clones. Let me know if you would prefer to have the check every time we call importBaseMemberDecl.

[susmonteiro]

@swift-ci please smoke test

[susmonteiro]

@swift-ci please smoke test Linux platform

[susmonteiro]

@swift-ci please test Windows platform

[egorzhdan]

LGTM, thanks!