[ParseableInterface] Standardize printing for accessors #20250

harlanhaskins · 2018-11-02T21:08:53Z

We want to make sure we are consistent when printing accessors.
@jrose-apple and I landed on the following table for disambiguating all
the possible combinations of accessors:

Computed Properties

Semantics	Syntax
Read-only	`var x: T { get }`
Read-write	`var x: T { get set }`

Stored Properties

Semantics	Syntax
Immutable	`let x: T`
Mutable (trivial setter)	`var x: T`
Mutable (custom setter)	`@_hasStorage var x: T { get set }`
Private(set)	`@_hasStorage var x: T { get }`

This patch introduces the @_hasStorage attribute which is printed for declarations that have storage and need to print custom accessors.

jrose-apple

Can you add some IRGen tests to demonstrate that we're computing layout correctly in all cases?

jrose-apple · 2018-11-02T21:52:03Z

lib/AST/ASTPrinter.cpp

@@ -838,6 +843,11 @@ static bool hasNonMutatingSetter(const AbstractStorageDecl *ASD) {
  return setter && setter->isExplicitNonMutating();
 }

+static bool isSimpleStoredWithPublicSetter(const AbstractStorageDecl *asd) {
+  return asd->getImplInfo().isSimpleStored() &&
+    !asd->getAttrs().hasAttribute<SetterAccessAttr>();


This doesn't seem right; it could be an open var with a public(set) (though I don't exactly know if we support that), or a public var with a @usableFromInline setter (though I don't know if we support that), or even a public var with a public(set) (which is only a warning).

it could be an open var with a public(set)

We do currently support this, so I'm gonna check for non-public-or-usableFromInline in the AccessLevel

a public var with a @usableFromInline setter

Technically we support this on internal computed properties, but those won't get printed in interfaces at all.

or even a public var with a public(set)

😢

The check I'm doing now is if the setter's access level is less than the access level of the storage decl itself. Is that right?

test/ParseableInterface/access-filter.swift

harlanhaskins · 2018-11-03T01:31:04Z

@jrose-apple For the IRGen tests, should I do it as a SIL test instead since this attribute doesn't exist outside of interfaces and interfaces don't reach IRGen?

harlanhaskins · 2018-11-03T01:36:17Z

lib/Parse/ParseDecl.cpp

@@ -4699,6 +4699,42 @@ static void diagnoseAndIgnoreObservers(Parser &P,
  }
 }

+static StorageImplInfo classifyWithHasStorageAttr(


@rjmccall This patch introduces a @_hasStorage attribute that allows us to break an ambiguity in parseable interfaces.

The mapping we've settled on is an understanding that

public private(set) var x: Int

maps to

@_hasStorage public var x: Int { get }

and

public var x: Int { willSet/didSet {} }

maps to

@_hasStorage public var x: Int { get set }

With that in mind, is this mapping correct?

You should be able to rename the @_sil_stored attribute for this purpose.

If you're changing this, could you change it to list/expect all the public accessors, i.e. including modify and (eventually) _read? You might need to (1) move the logic for deciding the opaque accessors into Sema and then (2) change StorageImplInfo to store that bitset.

I'm not sure how to handle _modify and _read if @_hasStorage is provided. Should they diagnose?

What are the semantics of @sil_stored var x: T { get set } vs. just @sil_stored var x: T? Or for @sil_stored let x: T { get } vs. just @sil_stored let x: T?

Currently it declares that the accessors are actually available. I'm not sure that's important to know in SIL. I do think there's a strong argument for it being useful to unambiguously list out the available opaque accessors, but it should be the full set, not just {get set} and then inferring modify and read and so on. However, to express that properly, you might have to change the representation a bit.

I'm going to do this in a future patch, because testing parseable interfaces is currently blocked on this representational change. For now, there's a loud FIXME explaining how it should eventually work.

Sounds good.

jrose-apple · 2018-11-05T18:07:25Z

For the IRGen tests, should I do it as a SIL test instead since this attribute doesn't exist outside of interfaces and interfaces don't reach IRGen?

I was thinking that you'd IRGen a client that imports an interface.

harlanhaskins · 2018-11-15T18:38:32Z

@jrose-apple Does the IRGen test look good?

harlanhaskins · 2018-11-15T18:39:54Z

test/SILOptimizer/accessed_storage_analysis.sil

@@ -340,7 +340,7 @@ class C {

 // CHECK-LABEL: @readIdentifiedClass
 // CHECK: [read] Class %0 = argument of bb0 : $C
-// CHECK: Field: @_hasStorage var property: Int


These are changed because this printer doesn't print for SIL. I could tell it to print for SIL, but that would cause other tests to print slightly differently, and this output isn't intended to convey full semantics.

harlanhaskins · 2018-11-15T18:59:22Z

Addresses rdar://43815861

jrose-apple · 2018-11-15T19:04:34Z

test/ParseableInterface/stored-properties-client.swift

+// RUN: %target-swift-frontend -emit-ir %s -I %t -enable-parseable-module-interface | %FileCheck %s -check-prefix CHECK -check-prefix COMMON
+
+// RUN: %target-swift-frontend -typecheck %S/stored-properties.swift -enable-resilience -module-name StoredProperties -emit-parseable-module-interface-path %t/StoredProperties.swiftinterface
+// RUN: %target-swift-frontend -emit-ir %s -I %t -enable-parseable-module-interface | %FileCheck %s -check-prefix RESILIENT -implicit-check-not CHECK -check-prefix COMMON


-implicit-check-not adds a negative pattern, not a negative prefix. What do you actually want to check here?

I just wanted to make sure the // CHECK lines ensure those lines don't appear in the resilient check.

I don't think there's a way to do that today, but I think it's okay for this test.

harlanhaskins · 2018-11-15T21:21:24Z

@swift-ci please test

harlanhaskins · 2018-11-15T21:30:27Z

@swift-ci please test

swift-ci · 2018-11-15T21:30:55Z

Build failed
Swift Test OS X Platform
Git Sha - 3453bf5fc0d0a4fce0eb8ce934e7dd8a46a4adbd

swift-ci · 2018-11-15T21:32:38Z

Build failed
Swift Test Linux Platform
Git Sha - 3453bf5fc0d0a4fce0eb8ce934e7dd8a46a4adbd

harlanhaskins · 2018-11-16T01:00:22Z

@swift-ci please test

swift-ci · 2018-11-16T01:02:12Z

Build failed
Swift Test Linux Platform
Git Sha - 3efd393ae2041ceb4f2080417ebe8fde7b6448bc

swift-ci · 2018-11-16T01:02:26Z

Build failed
Swift Test OS X Platform
Git Sha - 3efd393ae2041ceb4f2080417ebe8fde7b6448bc

harlanhaskins · 2018-11-16T02:58:46Z

@swift-ci please test

swift-ci · 2018-11-16T02:59:28Z

Build failed
Swift Test OS X Platform
Git Sha - 893b07c9082749c9fe189d3a0a062b608fbe4e61

swift-ci · 2018-11-16T03:00:41Z

Build failed
Swift Test Linux Platform
Git Sha - 893b07c9082749c9fe189d3a0a062b608fbe4e61

jrose-apple · 2018-11-16T17:49:05Z

test/ParseableInterface/stored-properties-client.swift

@@ -12,7 +12,7 @@ import StoredProperties
 /// layout and use the right access patterns in the presence of a
 /// .swiftinterface file, in both resilient and non-resilient cases.

-// COMMON: %[[BAGOFVARIABLES:T16StoredProperties14BagOfVariablesV]] = type <{ %TSi, %TSb, [7 x i8], %TSi }>
+// COMMON: %[[BAGOFVARIABLES:T16StoredProperties14BagOfVariablesV]] = type <{ %TSi, %TSb, [{{[0-9]}} x i8], %TSi }>


This is the thing we're supposed to be checking, though…can you do it as {{7|9}} or something like that? Or use FileCheck's fancy new -D feature?

I think we'd want it to be {{3|7}}, because it's going to pad to word boundaries after a Bool.

I just made up numbers, go for it.

harlanhaskins · 2018-11-26T22:27:29Z

@swift-ci please test

swift-ci · 2018-11-26T22:29:30Z

Build failed
Swift Test Linux Platform
Git Sha - 5a501fc277ed91fe4089396756c7a502619539ff

swift-ci · 2018-11-26T22:46:58Z

Build failed
Swift Test OS X Platform
Git Sha - 5a501fc277ed91fe4089396756c7a502619539ff

@jrose-apple

We want to make sure we are consistent when printing accessors. @jrose-apple and I landed on the following table for disambiguating all the possible combinations of accessors: \### Computed Properties | Semantics | Syntax | |------------|------------------------| | Read-only | var x: T { get } | | Read-write | var x: T { get set } | \### Stored Properties | Semantics | Syntax | |--------------------------|-------------------------------------| | Immutable | let x: T | | Mutable (trivial setter) | var x: T | | Mutable (custom setter) | @_hasStorage var x: T { get set } | | Private(set) | @_hasStorage var x: T { get } | This first commit enables printing @_hasStorage outside of SIL mode.

harlanhaskins · 2018-11-27T02:42:26Z

@swift-ci please test

swift-ci · 2018-11-27T02:44:34Z

Build failed
Swift Test Linux Platform
Git Sha - f5eb9a5e5d75db1d579ed32afad80c41c205cd6c

swift-ci · 2018-11-27T02:46:14Z

Build failed
Swift Test OS X Platform
Git Sha - f5eb9a5e5d75db1d579ed32afad80c41c205cd6c

[ParseableInterface] Standardize printing for accessors (cherry picked from commit 245109d)

jrose-apple · 2018-12-12T19:53:06Z

lib/Serialization/Deserialization.cpp

+      if (*setterAccess < *accessLevel) {
+        AddAttribute(
+          new (ctx) SetterAccessAttr(SourceLoc(), SourceLoc(),
+                                     *setterAccess, /*implicit*/true));


This is unfortunate. I wish I had noticed it at the time, but it's weird to be different from the regular access.

I agree that it's unfortunate, but I'm not sure how else to preserve this. The rest of the compiler assumes setter access is determined by the presence of this attribute. If there's a better way than synthesizing this here, I'm happy to change it.

jrose-apple reviewed Nov 2, 2018

View reviewed changes

harlanhaskins requested a review from rjmccall November 3, 2018 01:31

harlanhaskins force-pushed the accessory-to-a-git-crime branch from 260def1 to 7aca737 Compare November 3, 2018 01:32

harlanhaskins commented Nov 3, 2018

View reviewed changes

harlanhaskins force-pushed the accessory-to-a-git-crime branch from 7aca737 to 12c8673 Compare November 8, 2018 20:04

harlanhaskins mentioned this pull request Nov 12, 2018

[NFC] Rename @sil_stored to @_hasStorage #20512

Merged

harlanhaskins force-pushed the accessory-to-a-git-crime branch 3 times, most recently from ac54aca to 5283507 Compare November 15, 2018 02:44

harlanhaskins commented Nov 15, 2018

View reviewed changes

jrose-apple reviewed Nov 15, 2018

View reviewed changes

harlanhaskins force-pushed the accessory-to-a-git-crime branch from 0a60559 to 3efd393 Compare November 15, 2018 21:30

harlanhaskins force-pushed the accessory-to-a-git-crime branch from 3efd393 to 893b07c Compare November 16, 2018 01:00

jrose-apple reviewed Nov 16, 2018

View reviewed changes

harlanhaskins force-pushed the accessory-to-a-git-crime branch from 5a501fc to f5eb9a5 Compare November 26, 2018 22:21

Harlan Haskins added 8 commits November 26, 2018 18:40

[Sema] Add implicit @_hasStorage attribute for printing

a64a4e1

[Parse] Parse @_hasStorage attribute outside of SIL

dd163f0

[Sema] Allow explicit @_hasStorage attribute outside of SIL

9048954

[ASTPrinter] Print property observers/private(set) with @_hasStorage

1345310

[Serialization] Add @_hasStorage and private(set) while deserializing

a20c31a

[ParseableInterface] Test accessor printing with @_hasStorage

ea3e02b

[test] Ensure layout and accessors are correct with @_hasStorage

ac249e6

harlanhaskins force-pushed the accessory-to-a-git-crime branch from f5eb9a5 to ac249e6 Compare November 27, 2018 02:42

harlanhaskins merged commit 245109d into swiftlang:master Nov 27, 2018

harlanhaskins deleted the accessory-to-a-git-crime branch November 27, 2018 18:31

jrose-apple pushed a commit to jrose-apple/swift that referenced this pull request Dec 12, 2018

Merge pull request swiftlang#20250 from harlanhaskins/accessorizing

a1ef331

[ParseableInterface] Standardize printing for accessors (cherry picked from commit 245109d)

jrose-apple reviewed Dec 12, 2018

View reviewed changes

jrose-apple mentioned this pull request Dec 12, 2018

[5.0] Cherry-pick parseable interface generation improvements #21259

Merged

[ParseableInterface] Standardize printing for accessors #20250

[ParseableInterface] Standardize printing for accessors #20250

Uh oh!

Conversation

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

Uh oh!

Computed Properties

Stored Properties

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!

Uh oh!

harlanhaskins commented Nov 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 commented Nov 5, 2018

Uh oh!

harlanhaskins commented Nov 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Nov 15, 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!

harlanhaskins commented Nov 15, 2018

Uh oh!

harlanhaskins commented Nov 15, 2018

Uh oh!

swift-ci commented Nov 15, 2018

Uh oh!

swift-ci commented Nov 15, 2018

Uh oh!

harlanhaskins commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Uh oh!

harlanhaskins commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 2018

Uh oh!

swift-ci commented Nov 16, 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!

harlanhaskins commented Nov 26, 2018

Uh oh!

swift-ci commented Nov 26, 2018

Uh oh!

swift-ci commented Nov 26, 2018

harlanhaskins commented Nov 2, 2018 •

edited

Loading

harlanhaskins Nov 9, 2018 •

edited

Loading