[ClangImporter] Put subscripts in the same DC as the found accessor #13470
Conversation
|
@DougGregor, @milseman, @rjmccall, do any of you know why the old code would have been doing this? |
|
@swift-ci Please test macOS |
|
@swift-ci Please smoke test Linux |
|
@swift-ci Please test source compatibility |
|
Doing it based on which declaration we're currently importing is incoherent, right? You can get different results depending on what source code you're processing. We have two DCs; can we find some way to merge them, e.g. picking the more-derived one? |
|
You can always get different results depending on what source code you're processing (private module adds a setter), but I guess we should try to minimize the chances of that. Relying on the fact that we import things in the original module first is fragile. |
|
Well, I mean the Swift source code you're processing, like two different files in the same Swift module could decide that the subscript has a different DC based on the function bodies they process. |
|
Ah. Yeah, that would be pretty icky, whether or not it resulted in problems. Okay, back to the drawing board. |
Objective-C subscripts don't have special declarations like properties; they're just specially-named methods (or method pairs), and we have to make an independent SubscriptDecl in Swift. This means that when a subclass wants to make a subscript settable, they just add the appropriately-named setter method. Swift handled this by detecting when the getter and setter weren't declared in the same type, and assuming this meant it was a subclass adding a setter. Unfortunately, the same condition /also/ picked up the case where the getter (and only the getter) is /redeclared/ in a subclass (perhaps to add an attribute), and the new subscript was getting added to the base class instead of the subclass. The fix relies on the fact that the original decl we provide is what we use to look up the other accessor. If the getter and setter are in different types, whichever one we started with must be the more-derived one. So the final change is just "did we start with the setter?" rather than "is there a setter at all?". I'm not sure why this is only just now causing problems, given that we seem to have been getting this wrong for years, but it definitely /was/ wrong and now it's not. rdar://problem/36033356
jrose-apple
force-pushed
the
suboptimal-subscripts
branch
from
c39c3bb to
310a025
Compare
|
Thanks for making me think about this a little more, John. I understand what the old code was trying to do now and this new version makes more sense. @swift-ci Please test macOS |
|
@swift-ci Please smoke test Linux |
|
@swift-ci Please test source compatibility |
|
Build failed |
| @@ -6345,8 +6345,6 @@ SwiftDeclConverter::importSubscript(Decl *decl, | |||
| ->getAsNominalTypeOrNominalTypeExtensionContext() == | |||
| setter->getDeclContext() | |||
| ->getAsNominalTypeOrNominalTypeExtensionContext()); | |||
| // TODO: Possible that getter and setter are different instantiations | |||
| // of the same objc generic type? | |||
There was a problem hiding this comment.
This is unrelated, but we never have this problem given the final design of ObjC generics: they erase the generic argument rather than instantiating multiple types.
|
@swift-ci Please test source compatibility |
…iftlang#13470) Objective-C subscripts don't have special declarations like properties; they're just specially-named methods (or method pairs), and we have to make an independent SubscriptDecl in Swift. This means that when a subclass wants to make a subscript settable, they just add the appropriately-named setter method. Swift handled this by detecting when the getter and setter weren't declared in the same type, and assuming this meant it was a subclass adding a setter. Unfortunately, the same condition /also/ picked up the case where the getter (and only the getter) is /redeclared/ in a subclass (perhaps to add an attribute), and the new subscript was getting added to the base class instead of the subclass. The fix relies on the fact that the original decl we provide is what we use to look up the other accessor. If the getter and setter are in different types, whichever one we started with must be the more-derived one. So the final change is just "did we start with the setter?" rather than "is there a setter at all?". I'm not sure why this is only just now causing problems, given that we seem to have been getting this wrong for years, but it definitely /was/ wrong and now it's not. rdar://problem/36033356 (cherry picked from commit e1f46b5)
Objective-C subscripts don't have special declarations like properties; they're just specially-named methods (or method pairs), and we have to make an independent SubscriptDecl in Swift. Previously, we tried to always put that SubscriptDecl in the same context as the setter, but that doesn't make sense if the setter was actually inherited from a superclass!
Give up on a consistent "always with the setter" or "always with the getter" rule, which doesn't really affect anything in practice, and instead just use the context we know we can trust: the one containing the declaration we started with, whether getter or setter.This all started because when an Objective-C subclass wants to make a subscript settable, they just add the appropriately-named setter method. Swift handled this by detecting when the getter and setter weren't declared in the same type, and assuming this meant it was a subclass adding a setter. Unfortunately, the same condition also picked up the case where the getter (and only the getter) is redeclared in a subclass (perhaps to add an attribute), and the new subscript was getting added to the base class instead of the subclass.
The fix relies on the fact that the original decl we provide is what we use to look up the other accessor. If the getter and setter are in different types, whichever one we started with must be the more-derived one. So the final change is just "did we start with the setter?" rather than "is there a setter at all?".
I'm not sure why this is only just now causing problems, given that we seem to have been getting this wrong for years, but it definitely was wrong and now it's not.
Unfortunately, it's been this way so long I can't trace back to a reason why it was this way.All the tests still pass.rdar://problem/36033356