Skip to content

Codable synthesis: excluded vars shouldn’t need explicit value assignments #11854

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 11, 2017
Merged

Codable synthesis: excluded vars shouldn’t need explicit value assignments #11854

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion lib/Sema/DerivedConformanceCodable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,22 @@ validateCodingKeysEnum(TypeChecker &tc, EnumDecl *codingKeysDecl,
if (!properties.empty() &&
proto->isSpecificProtocol(KnownProtocolKind::Decodable)) {
for (auto it = properties.begin(); it != properties.end(); ++it) {
if (it->second->getParentInitializer() != nullptr) {
auto *varDecl = it->second;

// Optional vars (not lets!) have an implicit default value of nil.
if (!varDecl->isLet()) {
if (!varDecl->hasType())
tc.validateDecl(varDecl);

if (varDecl->hasType()) {
auto varTypeDecl = varDecl->getType()->getAnyNominal();
if (varTypeDecl == tc.Context.getOptionalDecl() ||
varTypeDecl == tc.Context.getImplicitlyUnwrappedOptionalDecl())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have TypeBase::getAnyOptionalObjectType for this, but really I'm leery of putting this check in more than one place. Is there any way to reuse the logic in TypeChecker::addImplicitConstructors?

Copy link
Contributor Author

@itaiferber itaiferber Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrose-apple That's likely a smarter solution. Are you interested in copying the logic into here, or factoring it out for reuse (e.g. adding isDefaultInitializable or similar to PatternBindingDecl)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect it belongs on VarDecl rather than PatternBindingDecl, but the latter sounds good to me. Even if it's mostly just "is optional and is not immutable".

Copy link
Contributor Author

@itaiferber itaiferber Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jrose-apple I'll adapt this, then. I mention PatternBindingDecl only because the logic in TypeChecker::addImplicitConstructors revolves around that:

if (auto pbd = dyn_cast<PatternBindingDecl>(member)) {
  if (pbd->hasStorage() && !pbd->isStatic() && !pbd->isImplicit())
    for (auto entry : pbd->getPatternList()) {
      if (entry.getInit()) continue;

      // If one of the bound variables is @NSManaged, go ahead no matter
      // what.
      bool CheckDefaultInitializer = true;
      entry.getPattern()->forEachVariable([&](VarDecl *vd) {
        if (vd->getAttrs().hasAttribute<NSManagedAttr>())
          CheckDefaultInitializer = false;
      });
      
      // If we cannot default initialize the property, we cannot
      // synthesize a default initializer for the class.
      if (CheckDefaultInitializer && !isDefaultInitializable(pbd))
        SuppressDefaultInitializer = true;
    }
  continue;
}

(isDefaultInitializable takes a PatternBindingDecl)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, though I guess the implementation we really care about takes a TypeRepr *:

static bool isDefaultInitializable(TypeRepr *typeRepr) {
  // Look through most attributes.
  if (auto attributed = dyn_cast<AttributedTypeRepr>(typeRepr)) {
    // Weak ownership implies optionality.
    if (attributed->getAttrs().getOwnership() == Ownership::Weak)
      return true;
    
    return isDefaultInitializable(attributed->getTypeRepr());
  }

  // Optional types are default-initializable.
  if (isa<OptionalTypeRepr>(typeRepr) ||
      isa<ImplicitlyUnwrappedOptionalTypeRepr>(typeRepr))
    return true;

  // Tuple types are default-initializable if all of their element
  // types are.
  if (auto tuple = dyn_cast<TupleTypeRepr>(typeRepr)) {
    // ... but not variadic ones.
    if (tuple->hasEllipsis())
      return false;

    for (auto elt : tuple->getElements()) {
      if (!isDefaultInitializable(elt.Type))
        return false;
    }

    return true;
  }

  // Not default initializable.
  return false;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. The existing logic is set up the way it is mostly because we don't have a way to get directly from a VarDecl to an initializer, but I guess it can stay that way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also care about the isNeverDefaultInitializable bit, so just looking at the TypeRepr isn't really sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. How much of this do we care to expose, then? Should this just be exposed on PatternBindingDecl, or is it helpful anywhere else to expose the intermediate layers too (on TypeRepr and Pattern)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just using PatternBindingDecl is fine. We don't currently default-initialize anything but variables.

continue;
}
}

if (varDecl->getParentInitializer() != nullptr) {
// Var has a default value.
continue;
}
Expand Down
64 changes: 64 additions & 0 deletions test/decl/protocol/special/coding/class_codable_excluded_optional_properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// RUN: %target-typecheck-verify-swift

class ClassWithNonExcludedLetMembers : Codable { // expected-error {{class 'ClassWithNonExcludedLetMembers' has no initializers}}
// expected-error@-1 {{type 'ClassWithNonExcludedLetMembers' does not conform to protocol 'Decodable'}}
// expected-error@-2 {{type 'ClassWithNonExcludedLetMembers' does not conform to protocol 'Encodable'}}

// Optional `let`s do not get an implicit nil assignment.
let p1: String? // expected-note {{stored property 'p1' without initial value prevents synthesized initializers}}
let p2: String! // expected-note {{stored property 'p2' without initial value prevents synthesized initializers}}

// AnyHashable does not conform to Codable.
let p3: AnyHashable? // expected-note {{stored property 'p3' without initial value prevents synthesized initializers}}
// expected-note@-1 {{cannot automatically synthesize 'Decodable' because 'AnyHashable?' does not conform to 'Decodable'}}
// expected-note@-2 {{cannot automatically synthesize 'Encodable' because 'AnyHashable?' does not conform to 'Encodable'}}
let p4: AnyHashable? // expected-note {{stored property 'p4' without initial value prevents synthesized initializers}}
// expected-note@-1 {{cannot automatically synthesize 'Decodable' because 'AnyHashable?' does not conform to 'Decodable'}}
// expected-note@-2 {{cannot automatically synthesize 'Encodable' because 'AnyHashable?' does not conform to 'Encodable'}}
}

class ClassWithExcludedLetMembers : Codable { // expected-error {{class 'ClassWithExcludedLetMembers' has no initializers}}
// expected-error@-1 {{type 'ClassWithExcludedLetMembers' does not conform to protocol 'Decodable'}}

// Optional `let`s do not get an implicit nil assignment.
let p1: String? // expected-note {{stored property 'p1' without initial value prevents synthesized initializers}}
let p2: String! // expected-note {{stored property 'p2' without initial value prevents synthesized initializers}}

// AnyHashable does not conform to Codable.
let p3: AnyHashable? // expected-note {{stored property 'p3' without initial value prevents synthesized initializers}}
// expected-note@-1 {{cannot automatically synthesize 'Decodable' because 'p3' does not have a matching CodingKey and does not have a default value}}
let p4: AnyHashable? // expected-note {{stored property 'p4' without initial value prevents synthesized initializers}}
// expected-note@-1 {{cannot automatically synthesize 'Decodable' because 'p4' does not have a matching CodingKey and does not have a default value}}

// Explicitly exclude non-Codable properties.
enum CodingKeys : String, CodingKey {
case p1, p2
}
}

class ClassWithNonExcludedVarMembers : Codable { // expected-error {{type 'ClassWithNonExcludedVarMembers' does not conform to protocol 'Decodable'}}
// expected-error@-1 {{type 'ClassWithNonExcludedVarMembers' does not conform to protocol 'Encodable'}}

var p1: String?
var p2: String!

// AnyHashable does not conform to Codable.
var p3: AnyHashable? // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable?' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable?' does not conform to 'Encodable'}}
var p4: AnyHashable! // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable!' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable!' does not conform to 'Encodable'}}
}

class ClassWithExcludedVarMembers : Codable {
var p1: String?
var p2: String!

// AnyHashable does not conform to Codable.
var p3: AnyHashable?
var p4: AnyHashable!

// Explicitly exclude non-Codable properties.
enum CodingKeys : String, CodingKey {
case p1, p2
}
}
69 changes: 69 additions & 0 deletions test/decl/protocol/special/coding/struct_codable_excluded_optional_properties.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// RUN: %target-typecheck-verify-swift

struct StructWithNonExcludedLetMembers : Codable { // expected-error {{type 'StructWithNonExcludedLetMembers' does not conform to protocol 'Decodable'}}
// expected-error@-1 {{type 'StructWithNonExcludedLetMembers' does not conform to protocol 'Encodable'}}

// Suppress the memberwise initializer. Optional `let`s do not get an implicit nil assignment.
init() {}

let p1: String?
let p2: String!

// AnyHashable does not conform to Codable.
let p3: AnyHashable? // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable?' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable?' does not conform to 'Encodable'}}
let p4: AnyHashable! // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable!' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable!' does not conform to 'Encodable'}}
}

struct StructWithExcludedLetMembers : Codable { // expected-error {{type 'StructWithExcludedLetMembers' does not conform to protocol 'Decodable'}}

// Suppress the memberwise initializer. Optional `let`s do not get an implicit nil assignment.
init() {}

let p1: String?
let p2: String!

// AnyHashable does not conform to Codable.
let p3: AnyHashable? // expected-note {{cannot automatically synthesize 'Decodable' because 'p3' does not have a matching CodingKey and does not have a default value}}
let p4: AnyHashable! // expected-note {{cannot automatically synthesize 'Decodable' because 'p4' does not have a matching CodingKey and does not have a default value}}

// Explicitly exclude non-Codable properties.
enum CodingKeys : String, CodingKey {
case p1, p2
}
}

struct StructWithNonExcludedVarMembers : Codable { // expected-error {{type 'StructWithNonExcludedVarMembers' does not conform to protocol 'Decodable'}}
// expected-error@-1 {{type 'StructWithNonExcludedVarMembers' does not conform to protocol 'Encodable'}}

// Suppress the memberwise initializer.
init() {}

var p1: String?
var p2: String!

// AnyHashable does not conform to Codable.
var p3: AnyHashable? // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable?' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable?' does not conform to 'Encodable'}}
var p4: AnyHashable! // expected-note {{cannot automatically synthesize 'Decodable' because 'AnyHashable!' does not conform to 'Decodable'}}
// expected-note@-1 {{cannot automatically synthesize 'Encodable' because 'AnyHashable!' does not conform to 'Encodable'}}
}

struct StructWithExcludedVarMembers : Codable {

// Suppress the memberwise initializer.
init() {}

var p1: String?
var p2: String!

// AnyHashable does not conform to Codable.
var p3: AnyHashable?
var p4: AnyHashable!

// Explicitly exclude non-Codable properties.
enum CodingKeys : String, CodingKey {
case p1, p2
}
}