Sema: Fix some issues with overrides of materializeForSet #12614

slavapestov · 2017-10-25T05:11:11Z

When overriding storage with a forced static dispatch materializeForSet,
the override's materializeForSet should not override the base
materializeForSet.

This is the case where a dynamic property witnesses a protocol
requirement, and Sema synthesizes a materializeForSet for it.

In this case, the synthesized materializeForSet dynamically dispatches
to the dynamic property's getter and setter, and the protocol witness
thunk directly calls the synthesized materializeForSet.

The subclass only needs to override the getter and setter in this case,
since the base class's materializeForSet will already do the right
thing.

In fact, marking it as an override exposes a problem where we cannot
serialize an xref to an imported property's materializeForSet, since
it was not created by the importer.

Radar: rdar://problem/35138127

slavapestov · 2017-10-25T05:11:28Z

@swift-ci Please test

jrose-apple

Fix looks good, tests need a bit of work.

jrose-apple · 2017-10-25T16:35:16Z

lib/AST/ASTVerifier.cpp

-                 baseASD->getMaterializeForSetFunc() &&
-                 "Storage override but materializeForSet does not");
+            baseASD->isSetterAccessibleFrom(ASD->getDeclContext())) {
+          if (baseASD->getMaterializeForSetFunc()->hasForcedStaticDispatch()) {


For later: should this just be final?

jrose-apple · 2017-10-25T16:36:31Z

test/multifile/synthesized-accessors/materialize-for-set-1/library.swift

+  var generatesDecimalNumbers: Bool { get set }
+}
+
+extension NumberFormatter : GeneratesDecimalNumbers {}


Please use the mock SDK or a handwritten class rather than the system Foundation.

jrose-apple · 2017-10-25T16:37:01Z

test/multifile/synthesized-accessors/materialize-for-set-1/library.swift

@@ -0,0 +1,13 @@
+// RUN: true


Please use an Inputs/ subdirectory rather than a dummy RUN line.

jrose-apple · 2017-10-25T16:38:03Z

test/multifile/synthesized-accessors/materialize-for-set-1/main.swift

+// RUN: %target-build-swift %S/main.swift -I %t/onone/ -emit-ir > /dev/null
+
+// RUN: %target-build-swift -emit-module -emit-module-path %t/wmo/library.swiftmodule -module-name=library -emit-library -wmo %S/library.swift
+// RUN: %target-build-swift %S/main.swift -I %t/wmo/ -emit-ir > /dev/null


I'm a little antsy about none of the output being checked here. Seems like this would work better as an IRGen test. There are also no calls to the variable's materializeForSet shown, either through the protocol or with a member of the class hierarchy as a static type.

The crash was while deserializing the base class's materializeForSet, which happens when you define the override. But I can think of a way to make this more explicit.

Yeah, instead of subclassing the subclass, the 'client' can just do an inout access on the property I guess.

- Put input files in directories named Inputs/ to eliminate bogus 'RUN: true' lines - Remove executable_test requirement since they're not executable

slavapestov · 2017-10-26T22:47:11Z

@jrose-apple I couldn't get the mock SDK working. Ran into -enable-source-import issues, and the %clang-importer-sdk-nosource variant only works with %target-swift-frontend, not %target-build-swift.

slavapestov · 2017-10-26T22:47:16Z

@swift-ci Please test

slavapestov · 2017-10-26T22:47:28Z

@swift-ci Please test

jrose-apple · 2017-10-26T23:17:25Z

Okay, but even if you have to use the real Foundation, please make your own class rather than reusing NumberFormatter.

slavapestov · 2017-10-26T23:45:10Z

@jrose-apple What's the downside of using NumberFormatter (or some other Foundation class)? Are you suggesting using -import-objc-header with my own header, or something else? I actually want to build and link the result and there are multiple Swift modules in play so I'm not sure -import-objc-header makes sense either.

When overriding storage with a forced static dispatch materializeForSet, the override's materializeForSet should not override the base materializeForSet. This is the case where a dynamic property witnesses a protocol requirement, and Sema synthesizes a materializeForSet for it. In this case, the synthesized materializeForSet dynamically dispatches to the dynamic property's getter and setter, and the protocol witness thunk directly calls the synthesized materializeForSet. The subclass only needs to override the getter and setter in this case, since the base class's materializeForSet will already do the right thing. In fact, marking it as an override exposes a problem where we cannot serialize an xref to an imported property's materializeForSet, since it was not created by the importer.

slavapestov · 2017-10-27T00:13:26Z

@jrose-apple I updated the tests to define a new Clang module with a custom type.

slavapestov · 2017-10-27T00:13:30Z

@swift-ci Please test

slavapestov · 2017-10-27T00:13:37Z

@swift-ci Please test

slavapestov · 2017-10-27T00:14:52Z

@swift-ci Please test

slavapestov · 2017-10-27T00:16:38Z

@shahmishal CI is broken again?

slavapestov · 2017-10-27T00:16:41Z

@swift-ci Please test

slavapestov · 2017-10-27T00:19:40Z

For fuck's sake

slavapestov · 2017-10-27T00:19:43Z

@swift-ci Please test

jrose-apple

Thank you. It's much better when the tests don't depend on how the SDK declares its APIs.

swift-ci · 2017-10-27T00:22:13Z

Build failed
Swift Test Linux Platform
Git Sha - e719109268a29da2d19e0e13004dfb2dc20f652b

swift-ci · 2017-10-27T00:34:48Z

Build failed
Swift Test Linux Platform
Git Sha - 0f2da21

slavapestov · 2017-10-27T01:26:07Z

@swift-ci Please test Linux

shahmishal · 2017-10-27T01:27:10Z

@swift-ci Please test linux

shahmishal · 2017-10-27T01:30:36Z

Its due to number of systems we have available.

https://ci.swift.org/label/mac-pro/load-statistics

swift-ci · 2017-10-27T01:35:42Z

Build failed
Swift Test Linux Platform
Git Sha - 0f2da21

slavapestov · 2017-10-27T01:53:50Z

@shahmishal

<unknown>:0: error: module map file '/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift-corelibs-libdispatch/dispatch/module.modulemap' not found
<unknown>:0: error: module map file '/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-master/swift-corelibs-libdispatch/dispatch/module.modulemap' not found

slavapestov · 2017-10-27T01:53:57Z

@swift-ci Please test Linux

swift-ci · 2017-10-27T01:54:10Z

Build failed
Swift Test Linux Platform
Git Sha - 0f2da21

slavapestov · 2017-10-27T01:54:43Z

Building remotely on ubuntu-16.04-04 (Ubuntu-16.04 linux-pr executors-4) in workspace /home/buildnode/jenkins/workspace/swift-PR-Linux
java.util.concurrent.TimeoutException: Ping started at 1509064218500 hasn't completed by 1509064458500
Caused: java.io.IOException

slavapestov · 2017-10-27T01:54:48Z

@swift-ci Please test Linux

swift-ci · 2017-10-27T01:55:00Z

Build failed
Swift Test Linux Platform
Git Sha - 0f2da21

slavapestov · 2017-10-27T02:17:25Z

@swift-ci Please test Linux

slavapestov · 2017-10-27T02:19:11Z

@swift-ci Please test Linux

slavapestov requested a review from jrose-apple October 25, 2017 05:11

jrose-apple reviewed Oct 25, 2017

View reviewed changes

Re-organize multi-file tests a bit

a0c0146

- Put input files in directories named Inputs/ to eliminate bogus 'RUN: true' lines - Remove executable_test requirement since they're not executable

slavapestov force-pushed the imported-materialize-for-set-override-bugs branch from 4cc60b0 to e719109 Compare October 26, 2017 22:46

slavapestov force-pushed the imported-materialize-for-set-override-bugs branch from e719109 to 0f2da21 Compare October 27, 2017 00:13

jrose-apple approved these changes Oct 27, 2017

View reviewed changes

slavapestov merged commit 5c45f85 into swiftlang:master Oct 27, 2017

belkadan mentioned this pull request Nov 15, 2017

[SR-6093] Compiler fails with SegFault when using multiple modules and whole-module optimization in this scenario. #48648

Closed

Sema: Fix some issues with overrides of materializeForSet #12614

Sema: Fix some issues with overrides of materializeForSet #12614

Uh oh!

Conversation

slavapestov commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Oct 25, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

jrose-apple Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

slavapestov commented Oct 26, 2017

Uh oh!

slavapestov commented Oct 26, 2017

Uh oh!

slavapestov commented Oct 26, 2017

Uh oh!

jrose-apple commented Oct 26, 2017

Uh oh!

slavapestov commented Oct 26, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Oct 27, 2017

Uh oh!

swift-ci commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

shahmishal commented Oct 27, 2017

Uh oh!

shahmishal commented Oct 27, 2017

Uh oh!

swift-ci commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

swift-ci commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

Uh oh!

slavapestov commented Oct 27, 2017

slavapestov commented Oct 25, 2017 •

edited

Loading