Factor isDefaultInitializable logic for reuse #11902

itaiferber · 2017-09-13T20:51:29Z

What's in this pull request?
Factors isDefaultInitializable logic out from TypeChecker::addImplicitConstructors (and others) so it can be reused for Codable synthesis. See discussion in comments on #11854.

itaiferber · 2017-09-13T20:51:37Z

@swift-ci Please test

itaiferber · 2017-09-13T20:53:03Z

include/swift/AST/Pattern.h

@@ -191,6 +191,9 @@ class alignas(8) Pattern {
  /// Does this binding declare something that requires storage?
  bool hasStorage() const;

+  /// Can this binding be initialized with a default value?
+  bool isDefaultInitializable() const;


This is the negation of isNeverDefaultInitializable which is used across multiple files and had to be extracted. isNeverDefaultInitializable seemed strange to expose, though; if there's a better name for this, I think we should use it. I don't think that negation is strong enough to promise default initializability...

itaiferber · 2017-09-13T20:53:31Z

lib/AST/Decl.cpp

@@ -1132,6 +1132,69 @@ VarDecl *PatternBindingDecl::getSingleVar() const {
  return nullptr;
 }

+/// Check whether the given type representation will be
+/// default-initializable.
+static bool isDefaultInitializable(TypeRepr *typeRepr) {


We said we didn't really want to expose this on TypeRepr directly, so this is copied as-is to here.

itaiferber · 2017-09-13T20:54:27Z

lib/Sema/TypeCheckDecl.cpp

@@ -4204,7 +4122,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
            TC.checkTypeModifyingDeclAttributes(var);

          // Decide whether we should suppress default initialization.
-          if (isNeverDefaultInitializable(PBD->getPattern(i)))
+          if (!PBD->getPattern(i)->isDefaultInitializable())


This is the one remaining call to isNeverDefaultInitializable on a Pattern outside of PatternBindingDecl.cpp

swift-ci · 2017-09-13T21:52:49Z

Build failed
Swift Test Linux Platform
Git Sha - b1f0c3b7fd7a8c8cc1de4820683fad755a047749

itaiferber · 2017-09-13T21:55:45Z

Hmm, looks like dispatch test failure on Linux

jrose-apple · 2017-09-13T21:58:50Z

Hm, I see the problem. Maybe it should look like this:

bool isDefaultInitializable(unsigned i) const;
bool isDefaultInitializable() const {
  for (unsigned i = 0, e = getNumPatternEntries(); i < e; ++i)
    if (!isDefaultInitializable(i))
      return false;
  return true;
}

Then you can fold all the checks together, and still have something reasonable for initializer synthesis.

itaiferber · 2017-09-13T22:02:17Z

@jrose-apple That's cleaner — thanks! 🙂 I'll put that together now.

itaiferber · 2017-09-14T18:36:17Z

@jrose-apple Cleaned this up a bit — let me know if there's anything you think can be further improved

itaiferber · 2017-09-14T18:36:25Z

@swift-ci Please test

swift-ci · 2017-09-14T18:37:37Z

Build failed
Swift Test Linux Platform
Git Sha - b1f0c3b7fd7a8c8cc1de4820683fad755a047749

swift-ci · 2017-09-14T18:40:27Z

Build failed
Swift Test OS X Platform
Git Sha - b1f0c3b7fd7a8c8cc1de4820683fad755a047749

jrose-apple · 2017-09-14T18:48:52Z

include/swift/AST/Decl.h

+  /// Can this binding be default initialized?
+  bool isDefaultInitializable() const {
+    if (!hasStorage())
+      return false;


I'm not sure this is correct. If the binding doesn't have storage, it doesn't need to be default-initialized. That seems closer to a true than a false for me, but honestly I'd just leave it as an assertion.

If whether the binding has storage or not doesn't bear on whether default initialization can happen, does asserting have any value over just taking this check out altogether? Because this was private implementation before the assertion was reasonable, but I don't know whether it makes that much sense now, no?

The assertion would indicate that it is a programmer error to call it in a context where you don't already know whether the PBD has storage. It's also questionable whether true or false would be the right answer in that scenario anyway, so an assertion gets you out of having to answer the question.

Fair enough 👍 I'll bring that back

jrose-apple

Looks good, apart from the one early exit comment.

itaiferber · 2017-09-14T21:46:30Z

@jrose-apple Hmm, in practice, it looks like exposing isDefaultInitializable(unsigned i) isn't quite right — what we really need to expose is something like isNeverDefaultInitializable(unsigned i). isDefaultInitializable makes too strong of a promise that we don't really need, and this screws up synthesis for lazy vars:

class X {
    lazy var abc: String? = {
        return "hello!"
    }()
}

produces

computed.swift:1:7: error: return from initializer without initializing all stored properties
class X {
      ^
computed.swift:1:7: note: 'self.abc.storage' not initialized
class X {
      ^

The underlying storage for X.abc (X.abc.storage) is implicit, so its TypeLoc has a type but no TypeRepr, so the following condition fails:

if (auto typedPattern = dyn_cast<TypedPattern>(entry.getPattern()))
  if (auto typeRepr = typedPattern->getTypeLoc().getTypeRepr()) // has a TypeLoc but the TypeLoc doesn't have a TypeRepr
    if (::isDefaultInitializable(typeRepr))
      return true;

The original code only checked whether the pattern using isNeverDefaultInitializable, which is much weaker.

You understand the nuances here better than I do. Is it more appropriate to simply expose isNeverDefaultInitializable and use that when checking patterns during a first pass (like the original code did), or is it okay to skip isDefaultInitializable(TypeRepr *) on implicit patterns and simply return true? What kind of implicit patterns might show up in a PatternBindingDecl that are not default initializable?

itaiferber · 2017-09-18T22:30:13Z

@swift-ci Please test

swift-ci · 2017-09-18T22:31:24Z

Build failed
Swift Test Linux Platform
Git Sha - 789ae9ddd86537590fc1f2424326c97af8199634

swift-ci · 2017-09-18T22:34:26Z

Build failed
Swift Test OS X Platform
Git Sha - 789ae9ddd86537590fc1f2424326c97af8199634

jrose-apple · 2017-09-18T23:59:46Z

lib/AST/Decl.cpp

+      if (auto *varDecl = typedPattern->getSingleVar())
+        // Lazy storage is never user accessible.
+        if (!varDecl->isUserAccessible()) {
+          auto type = typedPattern->getTypeLoc().getType()->getCanonicalType();


No need for the getCanonicalType here; getAnyOptionalObjectType will already do it.

(In general that's true of nearly all our type operations; the ones where you actually need to get the canonical type manually are the exception rather than the rule.)

Ah, makes sense; I'll remove that. These changes still look reasonable to you otherwise, yes?

itaiferber · 2017-09-19T00:08:30Z

@swift-ci Please test

swift-ci · 2017-09-19T00:09:45Z

Build failed
Swift Test Linux Platform
Git Sha - 832b03f6d0caf2e004c92c0bb8f9c02e0f077bd8

swift-ci · 2017-09-19T00:12:39Z

Build failed
Swift Test OS X Platform
Git Sha - 832b03f6d0caf2e004c92c0bb8f9c02e0f077bd8

jrose-apple

Yep, looks good. Thanks for pulling this out; it's an improvement even without your use case.

jrose-apple · 2017-09-19T21:10:51Z

lib/AST/Decl.cpp

@@ -1132,6 +1132,94 @@ VarDecl *PatternBindingDecl::getSingleVar() const {
  return nullptr;
 }

+/// Check whether the given type representation will be
+/// default-initializable.
+static bool isDefaultInitializable(TypeRepr *typeRepr) {


This and the Pattern * below could be using const.

Will fix before merging. Thanks for taking the time to answer all of my questions — I appreciate the patience. 😄

Factors isDefaultInitializable logic from TypeChecker::addImplicitConstructors (and others) so it can be reused for Codable synthesis.

itaiferber · 2017-09-20T16:47:33Z

@swift-ci Please test and merge

itaiferber requested a review from jrose-apple September 13, 2017 20:51

itaiferber commented Sep 13, 2017

View reviewed changes

itaiferber force-pushed the factor-out-isdefaultinitializable branch 2 times, most recently from 8c9d9f8 to 789ae9d Compare September 14, 2017 18:35

jrose-apple reviewed Sep 14, 2017

View reviewed changes

jrose-apple reviewed Sep 18, 2017

View reviewed changes

itaiferber force-pushed the factor-out-isdefaultinitializable branch from 832b03f to 6fa8788 Compare September 19, 2017 00:05

jrose-apple approved these changes Sep 19, 2017

View reviewed changes

Factor isDefaultInitializable logic for reuse

cda3bf1

Factors isDefaultInitializable logic from TypeChecker::addImplicitConstructors (and others) so it can be reused for Codable synthesis.

itaiferber force-pushed the factor-out-isdefaultinitializable branch from 6fa8788 to cda3bf1 Compare September 20, 2017 16:46

swift-ci merged commit abe34f8 into swiftlang:master Sep 20, 2017

Factor isDefaultInitializable logic for reuse #11902

Factor isDefaultInitializable logic for reuse #11902

Uh oh!

Conversation

itaiferber commented Sep 13, 2017

Uh oh!

itaiferber commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Sep 13, 2017

Uh oh!

itaiferber commented Sep 13, 2017

Uh oh!

jrose-apple commented Sep 13, 2017

Uh oh!

itaiferber commented Sep 13, 2017

Uh oh!

itaiferber commented Sep 14, 2017

Uh oh!

itaiferber commented Sep 14, 2017

Uh oh!

swift-ci commented Sep 14, 2017

Uh oh!

swift-ci commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 14, 2017

Uh oh!

itaiferber commented Sep 18, 2017

Uh oh!

swift-ci commented Sep 18, 2017

Uh oh!

swift-ci commented Sep 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 19, 2017

Uh oh!

swift-ci commented Sep 19, 2017

Uh oh!

swift-ci commented Sep 19, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itaiferber commented Sep 20, 2017

Uh oh!

Uh oh!