[InterfaceGen] Only print 'mutating' and 'nonmutating' on accessors #19459

harlanhaskins · 2018-09-21T17:57:59Z

This PR restricts the allowed set of implicit contextual decl modifiers on accessors to just 'mutating' and 'nonmutating'.

This comes up because we'll print dynamic on accessors, so the previous patch to disallow final is insufficient.

jrose-apple · 2018-09-21T18:51:12Z

test/ModuleInterface/modifiers.swift

+  // CHECK-NEXT: {{^}} @objc get
+  // CHECK-NEXT:   set[[NEWVALUE]] {}
+  // CHECK-NEXT: }
+  public @objc dynamic var d: Int {


Uh. You shouldn't be able to mix attributes in the middle of modifiers like this…

Also, please do add tests for mutating get and nonmutating set, now that it's come up.

harlanhaskins · 2018-09-22T00:04:56Z

@swift-ci please smoke test

harlanhaskins · 2018-09-24T18:54:41Z

@swift-ci please smoke test

jrose-apple · 2018-09-24T19:44:21Z

lib/AST/ASTPrinter.cpp

+  auto ctxType = ASD->getDeclContext()->getDeclaredInterfaceType();
+  return !ASD->isSetterMutating() &&
+         ASD->isInstanceMember() &&
+         !ctxType->hasReferenceSemantics();


@slavapestov, there's not really a better way to do this check that'll handle constrained protocol extensions, right?

(Also, the ASD->isInstanceMember check isn't quite right; if it's a non-instance member, it might still have a nonmutating setter. I think it's !A && (!B || !C).)

This is the same logic that was factored out of PrintAST::printAccessors, but I think you're right about the logic being wrong.

Turns out several tests were relying on the existing behavior 😞

jrose-apple · 2018-09-24T19:51:24Z

lib/AST/ASTPrinter.cpp

+    if (asd->getGetter() == D && hasMutatingGetter(asd))
+      Printer << "mutating ";
+    if (asd->getSetter() == D && hasNonMutatingSetter(accessor->getStorage()))
+      Printer << "nonmutating ";


Nitpick: printKeyword.

harlanhaskins · 2018-09-25T21:16:50Z

test/IDE/print_ast_tc_decls.swift

-// PASS_RW_PROP_GET_SET-NEXT:    {{^}}  static var extStaticProp: Int { get set }{{$}}
-// PASS_RW_PROP_NO_GET_SET-NEXT: {{^}}  static var extStaticProp: Int{{$}}
+// PASS_RW_PROP_GET_SET-NEXT:    {{^}}  static var extStaticProp: Int { get nonmutating set }{{$}}
+// PASS_RW_PROP_NO_GET_SET-NEXT: {{^}}  static var extStaticProp: Int { get nonmutating set }{{$}}


@jrose-apple What do you think about fallout like this?

That's incorrect; those are mutating sets.

D'oh. You're right.

harlanhaskins · 2018-09-25T22:59:23Z

@swift-ci please smoke test

harlanhaskins · 2018-09-25T23:21:18Z

@jrose-apple I think this is correct now.

jrose-apple

Whew, I think this is finally it.

jrose-apple · 2018-09-25T23:39:39Z

lib/AST/Decl.cpp

+    case AccessorKind::Modify:
+      return false;
+  }
+  llvm_unreachable("bad addressor kind");


Copy/paste error:"addressor" instead of "accessor". Also, the switch statement is over-indented.

jrose-apple · 2018-09-25T23:41:33Z

test/ModuleInterface/modifiers.swift

 // RUN: %FileCheck %s < %t/Test.swiftinterface
 // RUN: %target-swift-frontend -emit-module -o /dev/null -merge-modules %t/Test.swiftmodule -disable-objc-attr-requires-foundation-module -emit-interface-path - -module-name Test | %FileCheck %s

+// REQUIRES: objc_interop


Instead of this, Saleem's been having us put -enable-objc-interop in the RUN lines. That works as long as you don't need the ObjC runtime.

harlanhaskins · 2018-09-26T00:19:56Z

@swift-ci please smoke test

test/attr/attr_dynamic_infer.swift

harlanhaskins · 2018-09-26T01:25:19Z

test/SILGen/dynamic_accessors.swift

+
+@inlinable public func foo() {
+    let x = MyObjCClass()
+    // CHECK: objc_method %{{[0-9]+}} : $MyObjCClass, #MyObjCClass.a!getter.1.foreign


@jrose-apple Do you think this test is sufficient? It runs on the SIL from both the original source and the generated interface.

I'd add a check for a subscript too, but other than that yes, this looks good.

jrose-apple · 2018-09-26T02:34:11Z

test/SILGen/dynamic_accessors.swift

+// RUN: %empty-directory(%t)
+// RUN: %target-swift-frontend -emit-module -o /dev/null -disable-objc-attr-requires-foundation-module -emit-interface-path %t/dynamic_accessors.swiftinterface %s
+// RUN: %target-swift-emit-silgen -disable-objc-attr-requires-foundation-module %s | %FileCheck %s
+// RUN: %target-swift-frontend -emit-silgen -disable-objc-attr-requires-foundation-module %t/dynamic_accessors.swiftinterface | %FileCheck %s


Missing -enable-objc-interop on these. (I'm checking because I just got bitten by it.)

harlanhaskins · 2018-09-26T03:01:30Z

@swift-ci please smoke test

harlanhaskins requested a review from jrose-apple September 21, 2018 17:58

harlanhaskins force-pushed the nsattributedgetter branch from 2ddf52b to bd1e0ff Compare September 21, 2018 17:59

jrose-apple reviewed Sep 21, 2018

View reviewed changes

harlanhaskins force-pushed the nsattributedgetter branch 3 times, most recently from 6f7fad5 to 1d66ef3 Compare September 21, 2018 23:57

harlanhaskins force-pushed the nsattributedgetter branch from 1d66ef3 to b53fd33 Compare September 24, 2018 18:53

jrose-apple reviewed Sep 24, 2018

View reviewed changes

harlanhaskins force-pushed the nsattributedgetter branch 2 times, most recently from 4a64948 to da75805 Compare September 25, 2018 21:12

harlanhaskins commented Sep 25, 2018

View reviewed changes

harlanhaskins force-pushed the nsattributedgetter branch 2 times, most recently from 1013ef1 to cd70f0f Compare September 25, 2018 22:58

jrose-apple reviewed Sep 25, 2018

View reviewed changes

[InterfaceGen] Only print 'mutating' and 'nonmutating' on accessors

b7ecb92

harlanhaskins force-pushed the nsattributedgetter branch from cd70f0f to b7ecb92 Compare September 26, 2018 00:19

jrose-apple reviewed Sep 26, 2018

View reviewed changes

test/attr/attr_dynamic_infer.swift Show resolved Hide resolved

harlanhaskins commented Sep 26, 2018

View reviewed changes

harlanhaskins force-pushed the nsattributedgetter branch from b7da13f to cafda8a Compare September 26, 2018 01:34

Add SILGen test for usage of dynamic accessors in and out of interfaces

161ea99

harlanhaskins force-pushed the nsattributedgetter branch from cafda8a to 161ea99 Compare September 26, 2018 01:35

jrose-apple reviewed Sep 26, 2018

View reviewed changes

Add -enable-objc-interop to dynamic_accessors test

ff7a264

harlanhaskins merged commit 2aac886 into swiftlang:master Sep 26, 2018

[InterfaceGen] Only print 'mutating' and 'nonmutating' on accessors #19459

[InterfaceGen] Only print 'mutating' and 'nonmutating' on accessors #19459

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harlanhaskins commented Sep 22, 2018

Uh oh!

harlanhaskins commented Sep 24, 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!

harlanhaskins commented Sep 25, 2018

Uh oh!

harlanhaskins commented Sep 25, 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!

harlanhaskins commented Sep 26, 2018

Uh oh!

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 Sep 26, 2018

Uh oh!

Uh oh!

harlanhaskins commented Sep 21, 2018 •

edited

Loading