[SE implementation] Ownership keyword removal in protocols #11744

gspiers · 2017-09-02T12:48:14Z

This is an implementation of a draft Swift Evolution proposal apple/swift-evolution#707 to remove support for 'weak' and 'unowned' keywords for property declarations in protocols.

As suggested on the evolution pull request this implementation warns in existing Swift 3 and 4 modes and offers a fixit to remove the keyword. For Swift 5 mode it errors and of course also offers a fixit.

I'm new to contributing to Swift, I'm happy to take feedback and any direction to improve the implementation or proposal.

Resolves SR-479.

CodaFi · 2017-09-02T16:06:57Z

Neat

@swift-ci please smoke test

CodaFi · 2017-09-03T15:45:39Z

@jrose-apple Do you know of a similar warning for this in Clang for Objective-C protocols? I know of authors that assume a @property (..., weak) declaration in a protocol fully implies the ref-counting convention for the witness.

slavapestov · 2017-09-04T20:45:17Z

include/swift/AST/DiagnosticsSema.def

@@ -3102,6 +3102,11 @@ ERROR(invalid_weak_ownership_not_optional,none,
      "'weak' variable should have optional type %0", (Type))
 ERROR(invalid_weak_let,none,
      "'weak' must be a mutable variable, because it may change at runtime", ())
+ERROR(protocol_property_declarations_cannot_specify_ownership,none,
+      "ownership cannot be specified in protocols", ())


I would prefer if the diagnostic mentioned the keyword in question, like "'weak' properties are not supported in protocols"

I think Greg's wording is better, actually. This way people won't be concerned about using a weak property to satisfy a requirement.

(which, admittedly, might be dangerous, but let's say unowned instead)

Would the following be clearer error messages?

ownership keyword 'weak' is not supported in protocols
ownership keyword 'unowned' is not supported in protocols

@gspiers My meta-suggestion is to look for existing diagnostics concerning ownership keywords, and try to make the new ones consistent with the existing ones.

slavapestov · 2017-09-04T20:45:52Z

lib/Sema/TypeCheckAttr.cpp

@@ -2063,6 +2063,20 @@ void TypeChecker::checkOwnershipAttr(VarDecl *var, OwnershipAttr *attr) {

    diagnose(var->getStartLoc(), D, (unsigned) ownershipKind, underlyingType);
    attr->setInvalid();
+    return;
+  } else if (var->getDeclContext()->getAsProtocolOrProtocolExtensionContext()) {


This will also be true for protocol extensions. I don't think there should be any prohibition against defining weak or unowned properties in protocol extensions, and I don't see anything similar in the proposal. Please add a test that this case continues to work.

We don't actually let you put stored properties in a protocol extension today (instance or type properties), so it won't make much of a difference, but Slava's right that it shouldn't get this diagnostic.

Do we not allowed computed weak/unowned properties? I guess that doesn't make much sense, but I wasn't aware of the restriction.

Huh, it looks like we do. In that case, I'm not sure whether this is a good idea or not, but that's an issue for the review, not for the implementation.

Perhaps we should broaden the SE proposal to ban computed properties with ownership qualifiers also. It doesn't really make any sense to declare a weak or unowned computed property in my opinion, since this is a property of the implementation of the underlying storage and not of the contract of the property itself. The latter is already captured wholly by giving the property an optional or IUO type, in my opinion.

Yay for implementing proposals during review and teasing out odd corner cases!

slavapestov · 2017-09-04T20:46:20Z

test/Compatibility/ownership_protocol.swift

+  unowned var foo2: SomeClass { get set } // expected-warning {{ownership should not be specified in protocols and will be disallowed in future versions}}
+  weak var foo3: Int? { get set } // expected-error {{'weak' may only be applied to class and class-bound protocol types, not 'Int'}}
+  unowned var foo4: Int { get set } // expected-error {{'unowned' may only be applied to class and class-bound protocol types, not 'Int'}}
+}


Please add a trailing newline here

jrose-apple · 2017-09-05T16:42:11Z

Objective-C does check that the declaration of the property matches how it was declared in the protocol, for a strong/weak mismatch but not a weak/unsafe-unretained mismatch. ~~But~~ Objective-C also allows a property declared weak to be satisfied by a getter method, and Swift ~~currently does not~~ does as well.

slavapestov · 2017-09-06T05:02:45Z

@jrose-apple While we're bike-shedding, there's a JIRA bug out there pointing out that 'unowned' and 'weak' lazy properties fail to type check with odd diagnostics. I wonder if these should also be banned. Can you think of reasonable semantics we could give them? The real reason it fails is that we try to form an optional type whose payload type is weak or unowned, which doesn't type check, but the diagnostic is really bad because the failure manifests when we check the underlying storage of the lazy property and not the lazy property itself.

jrose-apple · 2017-09-06T16:11:13Z

Well, it's certainly possible to lazily construct something on first access that someone else owns, but that'd definitely be weird and unusual. That seems like it's separate from Greg's work, though, and if it doesn't work at all today then it'd be fine to ban it properly without going through swift-evolution.

gspiers · 2017-09-06T16:56:06Z

@jrose-apple and @slavapestov, thank you both for the feedback. As you've identified, it seems there's at least two other cases where ownership keywords don't really work as expected, lazy properties and computed properties.

I'm not sure what is best to do, I can expand the initial proposal to cover these two cases if you think that's best?

When using lazy properties and unowned/weak I can't actually produce code that will compile, so I agree that might just need fixing regarding the warning/error messages the compiler produces and might be separate to the evolution process.

For computed properties they are confusing when used with weak/unowned, as I'm not sure what you'd expect in this case:

class B {}
class A {
    weak var p1: B? {
        return B()
    }
    
    unowned var p2: B {
        return B()
    }
}

The ownership keywords above are meaningless I believe, so a fix might fit nicely in the current SE proposal.

I'm currently at a conference without my personal laptop so haven't been able to implement any of the changes you've identified above yet.

jrose-apple · 2017-09-06T17:05:51Z

Anyway, this seems good enough as a proof-of-concept for review. Agreed, Slava?

slavapestov · 2017-09-07T08:20:30Z

@jrose-apple Agreed.

While I do like the idea of addressing ownership keywords on computed properties and possibly lazy properties as part of implementing this proposal, it shouldn't block the swift-evolution discussion from moving forward.

gspiers · 2017-09-10T18:29:34Z

I've pushed changes to address initial feedback. The error messages should be consistent with other existing related errors. Correct error message is used when using weak/unowned property in a protocol extension.

I didn't do any changes regarding using ownership keywords with lazy or computed properties. I'll wait for guidance on if that's something I should put into the SE proposal.

@jrose-apple and @slavapestov let me know if there's anything else you'd like to see added to this implementation or the SE proposal, thanks again for your help 👍

jrose-apple

Thanks, Greg. A few more comments since last time.

jrose-apple · 2017-09-12T01:49:13Z

include/swift/AST/DeclContext.h

@@ -255,6 +255,10 @@ class alignas(1 << DeclContextAlignInBits) DeclContext {
  /// EnumDecl, otherwise return null.
  EnumDecl *getAsEnumOrEnumExtensionContext() const;

+  /// If this DeclContext is a protocol and not an extension on a
+  /// protocol, return the ProtocolDecl, otherwise return null.
+  ProtocolDecl *getAsProtocolContext() const;


The LLVM casting infrastructure already gives us a way to do this: dyn_cast<ProtocolDecl>(declContext). So we don't need a new method.

jrose-apple · 2017-09-12T01:51:56Z

lib/Sema/TypeCheckAttr.cpp

@@ -2063,6 +2067,22 @@ void TypeChecker::checkOwnershipAttr(VarDecl *var, OwnershipAttr *attr) {

    diagnose(var->getStartLoc(), D, (unsigned) ownershipKind, underlyingType);
    attr->setInvalid();
+    return;


This return doesn't really make sense when we're about to explicitly check for invalid and return below. Either all of the conditions in this function should be changed (in which case you don't need the "else" in else if), or you shouldn't worry about it. I'd lean towards the latter since it's less of a change from what's there.

jrose-apple · 2017-09-12T01:52:35Z

test/decl/protocol/ownership_protocol.swift

+
+extension P {
+  weak var foo5: SomeClass? // expected-error {{extensions may not contain stored properties}}
+  unowned var foo6: SomeClass // expected-error {{extensions may not contain stored properties}}


Please include some computed properties as well, since we've established that that's currently permitted.

Thanks, I've added some tests below of what should still be permitted and not produce an error.

tkremenek · 2017-09-30T23:18:04Z

SE-0186 has been accepted.

tkremenek · 2017-09-30T23:18:11Z

@swift-ci test

swift-ci · 2017-09-30T23:31:21Z

Build failed
Swift Test Linux Platform
Git Sha - ef50ed66f23dff5f7595e3c996824d5a0c2168f0

swift-ci · 2017-10-01T00:19:11Z

Build failed
Swift Test OS X Platform
Git Sha - ef50ed66f23dff5f7595e3c996824d5a0c2168f0

…in protocols.

gspiers · 2017-10-01T13:25:17Z

I've just rebased as I see some of the error messages were cleaned up recently. The tests pass on my machine now.

gspiers · 2017-10-05T13:14:03Z

@jrose-apple and @slavapestov now that the evolution proposal has been approved do you have any changes or additional feedback for this implementation? Thanks

jrose-apple · 2017-10-10T00:47:52Z

Seems fine to me! I'll merge if Slava has no objections.

@swift-ci Please test

jrose-apple · 2017-10-10T00:47:59Z

@swift-ci Please test source compatibility

slavapestov · 2017-10-15T04:14:00Z

@gspiers Do you mind submitting a PR to add this to CHANGELOG.md as well? Thanks!

gspiers · 2017-10-15T06:40:27Z

@slavapestov I'll create a PR Monday when I'm next near my computer.

gspiers mentioned this pull request Sep 2, 2017

Add draft proposal for ownership keyword removal in protocols. swiftlang/swift-evolution#707

Merged

slavapestov reviewed Sep 4, 2017

View reviewed changes

slavapestov self-assigned this Sep 5, 2017

jrose-apple added the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Sep 5, 2017

jessesquires mentioned this pull request Sep 5, 2017

[86] Issue #86 - Sept 7, 2017 SwiftWeekly/swiftweekly.github.io#305

Closed

gspiers force-pushed the bugfix/SR-479-ownership-in-protocols branch from 7fedcdb to b07eca7 Compare September 10, 2017 18:22

jrose-apple reviewed Sep 12, 2017

View reviewed changes

gspiers force-pushed the bugfix/SR-479-ownership-in-protocols branch from b07eca7 to ef50ed6 Compare September 16, 2017 15:04

Do not allow ‘weak’ and ‘unowned’ keywords for property declarations …

c5d6e17

…in protocols.

gspiers force-pushed the bugfix/SR-479-ownership-in-protocols branch from ef50ed6 to c5d6e17 Compare October 1, 2017 13:19

CodaFi added the swift evolution approved Flag → feature: A feature that was approved through the Swift evolution process label Oct 10, 2017

CodaFi removed the swift evolution pending discussion Flag → feature: A feature that has a Swift evolution proposal currently in review label Oct 10, 2017

slavapestov merged commit d866ebf into swiftlang:master Oct 10, 2017

gspiers mentioned this pull request Oct 25, 2017

Add SE-0186 to changelog. #12619

Merged

davezarzycki mentioned this pull request Apr 22, 2018

[Diag] QoI: ownership in protocol check should not be 'else' clause #16093

Merged

swift-ci mentioned this pull request Oct 10, 2017

[SR-479] Weak properties in protocols not checked in concrete classes #43096

Closed

MikeWeller mentioned this pull request Nov 19, 2022

[SR-7182] Ownership keyword removal from protocols breaks ObjC implementations #49730

Closed

[SE implementation] Ownership keyword removal in protocols #11744

[SE implementation] Ownership keyword removal in protocols #11744

Conversation

gspiers commented Sep 2, 2017 • edited Loading

CodaFi commented Sep 2, 2017

CodaFi commented Sep 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrose-apple commented Sep 5, 2017 • edited Loading

slavapestov commented Sep 6, 2017

jrose-apple commented Sep 6, 2017

gspiers commented Sep 6, 2017 • edited Loading

jrose-apple commented Sep 6, 2017

slavapestov commented Sep 7, 2017

gspiers commented Sep 10, 2017 • edited Loading

jrose-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkremenek commented Sep 30, 2017

tkremenek commented Sep 30, 2017

swift-ci commented Sep 30, 2017

swift-ci commented Oct 1, 2017

gspiers commented Oct 1, 2017

gspiers commented Oct 5, 2017

jrose-apple commented Oct 10, 2017

jrose-apple commented Oct 10, 2017

slavapestov commented Oct 15, 2017

gspiers commented Oct 15, 2017

gspiers commented Sep 2, 2017 •

edited

Loading

jrose-apple commented Sep 5, 2017 •

edited

Loading

gspiers commented Sep 6, 2017 •

edited

Loading

gspiers commented Sep 10, 2017 •

edited

Loading