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

[itaiferber]

What's in this pull request?
Properties of Codable types which are optional and excluded via the CodingKeys enum should not require an explicit nil assignment, since in all other cases, optional vars get a default value of nil. The following code should get correct synthesis:

class Foo : Codable {
    // These all get a default value of `nil`.
    var p1: String?
    var p2: String!
    var p3: AnyHashable?
    var p4: AnyHashable!

    private enum CodingKeys : String, CodingKey {
        case p1, p2
    }
}

Instead, p3 and p4 currently prevent synthesis because they don't have an explicit nil assignment.

[itaiferber]

@swift-ci Please test

[jrose-apple]

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?

[itaiferber]

@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)?

[jrose-apple]

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".

[itaiferber]

@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)

[itaiferber]

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;
}

[jrose-apple]

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.

[jrose-apple]

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

[itaiferber]

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)?

[jrose-apple]

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