[InterfaceGen] Print property initializers in resilient, fixed-layout types #19619

harlanhaskins · 2018-09-29T00:16:14Z

Augment the ASTPrinter to print the name and text of initializer expressions if
a property has an initializer and the type is @_fixed_layout and resides in a resilient module, and serialize the text for partial modules.

With this change, all .swiftinterface files in the project (except for SwiftLang) compile to swiftmodules on macOS.

rdar://43774580
rdar://43812188

harlanhaskins · 2018-09-29T00:29:39Z

lib/AST/ASTPrinter.cpp

+    return false;
+  auto parent = dyn_cast<NominalTypeDecl>(vd->getDeclContext());
+  if (!parent) return false;
+  if (parent->isFormallyResilient())


@slavapestov This seemed like the right check to do, but is it? I'm trying to see if the type is effectively fixed-layout. Should I be guarding this with a check if the module is resilient?

slavapestov · 2018-09-29T01:11:43Z

lib/AST/ASTPrinter.cpp

+/// There's a very narrow case when we would:
+/// If the decl:
+///   - has an initializer expression
+///   - is @usableFromInline


Also if its public

slavapestov · 2018-09-29T01:12:01Z

lib/AST/ASTPrinter.cpp

+///   - is @usableFromInline
+/// If the parent type:
+///   - is @_fixed_layout (or effectively fixed-layout)
+///   - has at least one inlinable initializer in itself or any of its


I don't think this second check is necessary

slavapestov · 2018-09-29T01:13:10Z

lib/AST/ASTPrinter.cpp

+///   - has an initializer expression
+///   - is @usableFromInline
+/// If the parent type:
+///   - is @_fixed_layout (or effectively fixed-layout)


What does effectively fixed-layout mean? Only types that have an explicit @_fixed_layout attribute need this treatment. If the module is not resilient and the type is implicitly @_fixed_layout, you cannot print the initial value because it might reference private things. However in that case the initializer symbol is public, so you don't need to emit it yourself.

If the type is explicitly fixed_layout and the property is non-public but usable from inline, should it still print the value?

Yes, and if the property is public too. @usableFromInline makes things public for ABI purposes but not public in the language itself. So “@usableFromInline but not public” is not a useful category of declaration. It’s always “public” or “public or @usableFromInline”.

slavapestov · 2018-09-29T01:13:23Z

lib/AST/ASTPrinter.cpp

+  if (!parent) return false;
+  if (parent->isFormallyResilient())
+    return false;
+  return evaluateOrDefault(vd->getASTContext().evaluator,


I don't understand why this needs to be a request. In fact I don't think you need to check this condition at all.

If we are doing the check, it’s nice not to look through the members and extensions for each property of the type. Though, if we don’t need the check all of the request plumbing can go away.

slavapestov · 2018-09-29T01:14:40Z

test/Constraints/diagnostics.swift

@@ -953,7 +953,7 @@ func SR_6272_a() {
    case bar
  }

-  // expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{35-35=Int(}} {{42-42=)}}
+  // expected-error@+2 {{binary operator '*' cannot be applied to operands of type 'Int' and 'Float'}} {{35-35=Int(}} {{43-43=)}}


Why did this diagnostic change?

It was previously using an incorrect source location, it was inserting the right paren of the cast before the existing right paren, not after. This was fixed by not removing the outer CoerceExpr when we convert a constructor call to a literal initialization.

Then it should be in a separate PR. Tag @rudkx or @xedin for review.

harlanhaskins · 2018-10-03T01:23:42Z

@slavapestov @jrose-apple I think this is correct now.

harlanhaskins · 2018-10-03T01:31:51Z

test/ModuleInterface/fixed-layout-property-initializers.swift

+// RUN: %target-swift-frontend -emit-interface-path %t-resilient.swiftinterface -enable-resilience -emit-module -o /dev/null %s
+// RUN: %FileCheck %s --check-prefix RESILIENT --check-prefix COMMON < %t-resilient.swiftinterface
+
+// FIXME(rdar): %target-swift-frontend -emit-module -o %t/Test.swiftmodule %t.swiftinterface -disable-objc-attr-requires-foundation-module


I need to actually file this, hold on

jrose-apple · 2018-10-03T15:40:29Z

include/swift/AST/Decl.h

@@ -1883,6 +1883,9 @@ class PatternBindingEntry {
  /// The initializer context used for this pattern binding entry.
  DeclContext *InitContext = nullptr;

+  /// The text of the initializer expression if deserialized from a module.
+  StringRef InitStringRepresentation;


Bitpack nitpick: there are a lot of pattern binding entries in the program. Can we cram this in with the initializer expression, and move the flags to the other members?

jrose-apple · 2018-10-03T15:41:40Z

lib/AST/ASTPrinter.cpp

+    // Don't print @_hasInitialValue if we're in printing an
+    // @usableFromInline property.
+    if (auto vd = dyn_cast<VarDecl>(D)) {
+      if (vd->getAttrs().hasAttribute<UsableFromInlineAttr>())


This isn't the right check; it should be the same as how you decide whether to print the initializer expression.

jrose-apple · 2018-10-03T15:44:08Z

lib/AST/ASTPrinter.cpp

+  if (!parent) return false;
+  if (!vd->getAttrs().hasAttribute<HasInitialValueAttr>())
+    return false;
+  if (!parent->getAttrs().hasAttribute<FixedLayoutAttr>())


You also need to check that it's an instance property.

jrose-apple · 2018-10-03T15:45:21Z

lib/AST/Decl.cpp

-    if (ATD->getProtocol()->getAttrs().hasAttribute<UsableFromInlineAttr>())
+  if (auto *containingProto = dyn_cast<ProtocolDecl>(getDeclContext())) {
+    if (isProtocolRequirement() &&
+        containingProto->getAttrs().hasAttribute<UsableFromInlineAttr>())


This looks like one of my changes. I guess it'll get merged away.

Ah, yes, this was accidental.

jrose-apple · 2018-10-03T15:47:51Z

lib/Serialization/Serialization.cpp

+  StringRef initStr;
+  SmallString<128> scratch;
+  if (binding->hasInitStringRepresentation(bindingIndex)) {
+    initStr = binding->getInitStringRepresentation(bindingIndex, scratch);


This should probably not be serialized if we don't need it (the same conditions as printing).

I guess in general we need that configuration option that says whether we're making a partial module or a complete module, which would be good enough.

test/ModuleInterface/fixed-layout-property-initializers.swift

harlanhaskins · 2018-10-04T00:45:12Z

@swift-ci please smoke test

harlanhaskins · 2018-10-04T00:49:21Z

@swift-ci please smoke test

jrose-apple

Looks pretty good. All minor comments left.

jrose-apple · 2018-10-04T16:22:34Z

include/swift/AST/Decl.h

+  llvm::PointerIntPair<Pattern *, 3, OptionSet<Flags>> PatternAndFlags;
+
+  /// The location of the equal '=' token.
+  SourceLoc EqualLoc;


Do you think we should put this into the union too? It appeals to my "pack everything as small as possible" instincts, but I can also see us doing a better job preserving source locations through deserialization at some point.

Do we serialize any SourceLocs currently? I feel like we could get the win here by tying the EqualLoc with the InitExpr, and then pay for it later if we do end up serializing this loc. I don't think it would hurt to throw it in the union now.

We don't today, on the grounds that the serialized module might be distributed somewhere. Okay then.

jrose-apple · 2018-10-04T16:25:08Z

lib/AST/ASTPrinter.cpp

+    // Don't print @_hasInitialValue if we're printing an initializer
+    // expression.
+    if (auto vd = dyn_cast<VarDecl>(D)) {
+      if (vd->shouldSerializeInitializerText())


I wonder if this indicates that the name shouldn't mention "serialization". What do you think of "isInitExposedToClients" or something? That would also make sense as the condition being checked in Sema when deciding whether to enforce inlinable-like rules.

Yeah, I tried to find a better name that doesn't reference serialization or printing, but I didn't want the name to be something like hasInitInResilientFixedLayoutType. I like isInitExposedToClients.

jrose-apple · 2018-10-04T16:27:47Z

test/ModuleInterface/fixed-layout-property-initializers.swift

+// RUN: %FileCheck %s --check-prefix RESILIENT --check-prefix COMMON < %t-resilient.swiftinterface
+
+// FIXME(rdar44993525): %target-swift-frontend -emit-module -o %t/Test.swiftmodule %t.swiftinterface -disable-objc-attr-requires-foundation-module
+// FIXME(rdar44993525): %target-swift-frontend -emit-module -o /dev/null -merge-modules %t/Test.swiftmodule -module-name Test -emit-interface-path - | %FileCheck %s --check-prefix CHECK --check-prefix COMMON


For my own curiosity, what goes wrong here?

Since we don't print initializers in a non-resilient module, compiling the emitted interface still fails with:

error: return from initializer without initializing all stored properties

The presence of the @hasInitialValue attribute should be the same as an initializer in a non-resilient module, since the initializer expression has a public entry point in the dylib

Then I should teach SILGen to declare these public symbols when there's a @_hasInitialValue property. I can do that in a follow-up.

That…doesn't seem sufficient to me, because of private fields. How would you know the right private discriminator?

jrose-apple · 2018-10-04T16:28:00Z

test/ModuleInterface/fixed-layout-property-initializers.swift

@@ -0,0 +1,67 @@
+// RUN: %empty-directory(%t)
+
+// RUN: %target-swift-frontend -emit-interface-path %t.swiftinterface -emit-module -o /dev/null %s


I merged my other change, so you can switch these to -typecheck if you want.

jrose-apple · 2018-10-04T16:29:33Z

test/ModuleInterface/fixed-layout-property-initializers.swift

+
+  // COMMON: @inlinable internal init() {}
+  @inlinable init() {}
+}


Missing a test case for static and for top-level properties.

harlanhaskins · 2018-10-04T22:14:01Z

@swift-ci please smoke test

slavapestov · 2018-10-04T23:47:05Z

lib/AST/ASTPrinter.cpp

-          !isPublicOrUsableFromInline(decl))
+          !isPublicOrUsableFromInline(decl) &&
+          // FIXME: We need to figure out a way to generate an entry point
+          //        for the initializer expression without revealing the name.


I don't think that's possible, since the mangled name of the initializer contains the property name

We don't actually care what the name is, though, since it's not a public entry point. At least, we don't for resilient modules.

If we generate a name here it has to be consistent between invocations though, so that you can do a non-WMO build where multiple source files import the same textual interface (and the cached binary form got rebuild between invocations)

I would again advocate for the simpler approach, instead of figuring out the exact strategy for when we can elide the name, just be conservative and only elide it if there's no initializer

We're talking about a swiftinterface here; it's always one file.

And we can't just print the name because the name might conflict with extensions.

harlanhaskins · 2018-10-05T21:22:58Z

@swift-ci please smoke test

… types Augment the ASTPrinter to print the name and text of initializer expressions if a property has an initializer and the type is @_fixed_layout and resides in a resilient module, and serialize the text for partial modules. With this change, all .swiftinterface files in the project (except for SwiftLang) compile to swiftmodules on macOS. rdar://43774580 rdar://43812188

harlanhaskins · 2018-10-06T00:17:55Z

@swift-ci please smoke test

… types (swiftlang#19619) Augment the ASTPrinter to print the name and text of initializer expressions if a property has an initializer and the type is @_fixed_layout and resides in a resilient module, and serialize the text for partial modules. With this change, all .swiftinterface files in the project (except for SwiftLang) compile to swiftmodules on macOS. rdar://43774580 rdar://43812188

harlanhaskins force-pushed the interface-your-fears branch from 442026e to becb94d Compare September 29, 2018 00:17

This comment has been minimized.

Sign in to view

harlanhaskins requested review from jrose-apple, slavapestov and DougGregor September 29, 2018 00:19

harlanhaskins force-pushed the interface-your-fears branch 4 times, most recently from b221403 to ea9cb98 Compare September 29, 2018 00:29

harlanhaskins commented Sep 29, 2018

View reviewed changes

slavapestov reviewed Sep 29, 2018

View reviewed changes

harlanhaskins force-pushed the interface-your-fears branch 6 times, most recently from f944888 to 8d06ee8 Compare October 3, 2018 01:23

harlanhaskins changed the title ~~[WIP] [InterfaceGen] Print initializers for @usableFromInline properties~~ [InterfaceGen] Print property initializers in resilient, fixed-layout types Oct 3, 2018

harlanhaskins commented Oct 3, 2018

View reviewed changes

jrose-apple reviewed Oct 3, 2018

View reviewed changes

harlanhaskins force-pushed the interface-your-fears branch 3 times, most recently from 27cd7aa to 179ac81 Compare October 3, 2018 23:34

harlanhaskins force-pushed the interface-your-fears branch from 179ac81 to bcb2100 Compare October 4, 2018 00:48

jrose-apple reviewed Oct 4, 2018

View reviewed changes

harlanhaskins force-pushed the interface-your-fears branch from bcb2100 to cc51edb Compare October 4, 2018 16:48

slavapestov reviewed Oct 4, 2018

View reviewed changes

harlanhaskins force-pushed the interface-your-fears branch from f94b201 to 43c30f2 Compare October 5, 2018 18:54

jrose-apple approved these changes Oct 5, 2018

View reviewed changes

harlanhaskins force-pushed the interface-your-fears branch 3 times, most recently from 425c863 to 54baeed Compare October 5, 2018 23:32

harlanhaskins force-pushed the interface-your-fears branch from 54baeed to 88c3574 Compare October 6, 2018 00:17

harlanhaskins merged commit 2c86e32 into swiftlang:master Oct 6, 2018

		@@ -0,0 +1,67 @@
		// RUN: %empty-directory(%t)

		// RUN: %target-swift-frontend -emit-interface-path %t.swiftinterface -emit-module -o /dev/null %s

[InterfaceGen] Print property initializers in resilient, fixed-layout types #19619

[InterfaceGen] Print property initializers in resilient, fixed-layout types #19619

Uh oh!

Conversation

harlanhaskins commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

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!

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!

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!

harlanhaskins commented Oct 3, 2018

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!

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!

Uh oh!

harlanhaskins commented Oct 4, 2018

Uh oh!

harlanhaskins commented Oct 4, 2018

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Oct 4, 2018

Uh oh!

harlanhaskins commented Sep 29, 2018 •

edited

Loading