[ClangImporter] Ignore redeclared properties with different types #6175
Conversation
|
@swift-ci Please test |
|
Hoping for @milseman to review the first commit and @slavapestov the second. |
| @@ -3987,6 +3987,13 @@ namespace { | |||
| return VisitObjCPropertyDecl(decl, dc); | |||
| } | |||
|
|
|||
| bool isExpectedTypeForPropertySetter(const FuncDecl *setter, | |||
There was a problem hiding this comment.
Can this be a static function outside of the class declaration?
| @@ -4020,7 +4027,7 @@ namespace { | |||
|
|
|||
| FuncDecl *setter = importAccessor(clangSetter, | |||
| original->getDeclContext()); | |||
| if (!setter) | |||
| if (!setter || !isExpectedTypeForPropertySetter(setter, original)) | |||
There was a problem hiding this comment.
When might this second part happen? Preferably a comment here at the use site, or else a comment on the function itself. Otherwise, it's hard to quickly think about when this check might trigger or why it is in here.
In the case of the nullability change, this snuck all the way past the type checker to result in an assertion failure in SILGen; if assertions were turned off, it continued all the way through IRGen before the LLVM verifier caught it. We get in this situation because ObjCPropertyDecls in Clang aren't considered "redeclared" like most other entities. Instead, they're just matched up by name in a few places. At first I considered trying to handle such canonicalization post hoc in Swift's importer, but that would have turned into plenty of work for something that rarely comes up at all. Instead, this patch just drops the setter from such a redeclared property if it doesn't match up. Clang itself should diagnose these kinds of mismatches; that's been filed as rdar://problem/29536751 for nullability and a similar rdar://problem/29222912 for changing types in general. rdar://problem/29422993
That is, a property in a protocol with type 'Self' doesn't promise that the result type always matches the type of 'self', just the type of the conforming type. (For a method, a result type of 'Self' /does/ mean you get back a type based on the dynamic 'self', like Objective-C's 'instancetype'.) This applies even to get-only properties, and so their accessors should be treated consistently. With this change, we can have the AST verifier check that getter and setter types always match up with their property.
jrose-apple
force-pushed
the
bad-property-redeclaration
branch
from
9f6b45e to
21dfa78
Compare
|
Updated for @milseman's feedback. Still waiting on @slavapestov to double-check the second change. @swift-ci Please smoke test |
|
@swift-ci Please smoke test |
|
:-/ @neonichu That one really did fail in llbuild. |
|
@swift-ci Please smoke test Linux |
| Type typeForAccessors = | ||
| var->getInterfaceType()->getReferenceStorageReferent(); | ||
| typeForAccessors = | ||
| ArchetypeBuilder::mapTypeIntoContext(var->getDeclContext(), |
There was a problem hiding this comment.
I think the mapTypeIntoContext() calls are unnecessary
There was a problem hiding this comment.
I tried just comparing the interface types and it actually failed, because sometimes the interface type of the property was a generic parameter but the interface type of an accessor was a concrete type. Or vice versa.
|
Looks good to me. |
In the case of the nullability change, this snuck all the way past the type checker to result in an assertion failure in SILGen; if assertions were turned off, it continued all the way through IRGen before the LLVM verifier caught it.
We get in this situation because ObjCPropertyDecls in Clang aren't considered "redeclared" like most other entities. Instead, they're just matched up by name in a few places. At first I considered trying to handle such canonicalization post hoc in Swift's importer, but that would have turned into plenty of work for something that rarely comes up at all. Instead, this patch just drops the setter from such a redeclared property if it doesn't match up.
Clang itself should diagnose these kinds of mismatches; that's been filed as rdar://problem/29536751 for nullability and a similar rdar://problem/29222912 for changing types in general.
rdar://problem/29422993