[codable] When generated encode for a property, distinguish in betwee… #16139
Conversation
| /// lookup that we perform /could/ succeed multiple times, we assert. Since the | ||
| /// only purpose of this is testing the assert, this is only enabled in asserts | ||
| /// builds. | ||
| static VarDecl *lookupNonStaticProperty(NominalTypeDecl *targetDecl, |
There was a problem hiding this comment.
This is overkill in my opinion. Please just add an 'isStatic()' check below.
There was a problem hiding this comment.
I disagree very strongly here. The lack of such things in the general case led to the bug here.
There was a problem hiding this comment.
Also you assertion will fire on invalid code. We only perform re-declaration checking on the primary file, and even in primary files, a re-declaration will set the 'invalid' flag on the re-declaration but not physically remove it from its parent decl. So if you want this to be 100% robust, you should also filter out 'invalid' declarations, and if there is more than one lookup result, just return the first one since we will diagnose the error elsewhere.
However, I disagree that adding a new 50-line method is a 'general case'. It's only used in one place, in one file. Unless you plan on re-working all existing callers of lookupDirect(), please do not do this.
lib/Sema/DerivedConformances.h
Outdated
| @@ -18,19 +18,24 @@ | |||
| #ifndef SWIFT_SEMA_DERIVEDCONFORMANCES_H | |||
| #define SWIFT_SEMA_DERIVEDCONFORMANCES_H | |||
|
|
|||
| #include "swift/AST/Identifier.h" | |||
There was a problem hiding this comment.
Headers should not include other headers if possible. Please put this include in DerivedConformanceCodable.cpp.
There was a problem hiding this comment.
In this case b/c Identifier is passed by value, it can not be forward declared.
There was a problem hiding this comment.
I'm not sure what you mean. The header file does not reference anything from Identifier.h, only DerivedConformanceCodable.cpp does. So DerivedConformanceCodable.cpp should include Identifier.h. You don't need to forward-declare anything in DerivedConformances.h.
There was a problem hiding this comment.
/// Declare a read-only property.
std::pair<VarDecl *, PatternBindingDecl *>
declareDerivedProperty(TypeChecker &tc,
Decl *parentDecl,
NominalTypeDecl *typeDecl,
Identifier name, <==== THIS
Type propertyInterfaceType,
Type propertyContextType,
bool isStatic,
bool isFinal);
There was a problem hiding this comment.
Oh ok, so the reference was not introduced by your changes, you're just cleaning it up. Got it. A separate commit titled "header gardening" or similar would've made that clearer.
There was a problem hiding this comment.
I agree. TBH this was just clang formatting fall out that I quickly worked around.
|
@swift-ci smoke test |
|
I am going to slim this down to make it easier to cherry-pick. |
…n static/instance properties. Previously, we just took the first match so: 1. We would try to emit a metatype lookup of the property but had prepared an instance lookup. 2. Could get the wrong type if the static/instance property had different types. rdar://39669212
|
@swift-ci smoke test |
|
@jckarter Can you take a quick look at this? |
|
Or actually @slavapestov your good as well ; ). Sorry for the confusion. |
|
Or @DougGregor would be great as well. |
|
Thanks Doug! |
…n static/instance properties.
Previously, we just took the first match so:
rdar://39669212