[Review] SE-0047 Defaulting non-Void functions so they warn on unused results


(William Shipley) #1

Strong +1, with some nitpicks to enclosed example:

I disagree that NSNotificationCenter’s returned token should be ignored without warning—this goes against the patterns I’ve seen in programming Cocoa the last 27 years, where needing to unregistering for notifications when an object is unloaded is more common than registering once and leaving it for the lifetime of the app.

For instance, if you have an NSViewController, you would register for notifications in your ‘loadView()’ or ‘awakeFromNib()’ method, or possibly you’d register with your superview (or enclosing scrollView) in ‘viewDidMoveToSuperview(_)', and it’d be very poor form to not store the notification’s handle and unregister later when the view was removed / deallocated. There are very few objects in a program that are 100% persistent throughout the life of the app — sandbox apps with a single window may have some, but those are the exception.

  • What is your evaluation of the proposal?

Yup.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yup.

  • Does this proposal fit well with the feel and direction of Swift?

Yup.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Nope.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading, but I’ve been programming for a while now and thought about the issue.


(Erica Sadun) #2

NSObservervationCenter example: should not be marked. Most observer lifetimes will be shorter than the application's lifetime and view controllers should clean up after themselves when dismissed, hidden, or otherwise put away. The compiler should emit warnings about this use that the developer must override.

Are you sure we are disagreeing?

-- E

···

On Mar 17, 2016, at 7:15 PM, William Shipley via swift-evolution <swift-evolution@swift.org> wrote:

Strong +1, with some nitpicks to enclosed example:

I disagree that NSNotificationCenter’s returned token should be ignored without warning—this goes against the patterns I’ve seen in programming Cocoa the last 27 years, where needing to unregistering for notifications when an object is unloaded is more common than registering once and leaving it for the lifetime of the app.


(William Shipley) #3

Uh, sorry, I read that backwards. You’re right, I agree with the recommendation as written.

-W

···

On Mar 17, 2016, at 6:36 PM, Erica Sadun <erica@ericasadun.com> wrote:

NSObservervationCenter example: should not be marked. Most observer lifetimes will be shorter than the application's lifetime and view controllers should clean up after themselves when dismissed, hidden, or otherwise put away. The compiler should emit warnings about this use that the developer must override.


(Andrew Bennett) #4

*What is your evaluation of the proposal?*
+1

*Is the problem being addressed significant enough to warrant a change to
Swift?*
Yes, the current annotation is present on most non-void functions, it adds
a large number of lines to files that are properly annotated.

*Does this proposal fit well with the feel and direction of Swift?*
Yes, it removes a lot of excess code, and defaults to the safer option.
It's rare in my experience for Swifty code to both mutate and return.

*If you have used other languages or libraries with a similar feature, how
do you feel that this proposal compares to those?*
N/A

*How much effort did you put into your review? A glance, a quick reading,
or an in-depth study?*
A quick read, I followed the discussion, I read other reviews.

···

----

It may also be worth considering adding an ObjC attribute so that
exceptional imported functions can be properly annotated.

Likewise, it may be worth considering deprecating @warn_unused_result
rather than removing it in a migration. A linter could ensure that audited
files have either @warn_unused_result or @discardableResult on every
function.

When @discardableResult becomes @discardable, the deprecation could become
final, and @warn_unused_result could be removed by migration. This
migration should probably only happen if all functions in the file are
annotated.

On Fri, Mar 18, 2016 at 12:36 PM, Erica Sadun via swift-evolution < swift-evolution@swift.org> wrote:

On Mar 17, 2016, at 7:15 PM, William Shipley via swift-evolution < swift-evolution@swift.org> wrote:

Strong +1, with some nitpicks to enclosed example:

I disagree that NSNotificationCenter’s returned token should be ignored

without warning—this goes against the patterns I’ve seen in programming
Cocoa the last 27 years, where needing to unregistering for notifications
when an object is unloaded is more common than registering once and leaving
it for the lifetime of the app.

NSObservervationCenter example: should not be marked. Most observer

lifetimes will be shorter than the application's lifetime and view
controllers should clean up after themselves when dismissed, hidden, or
otherwise put away. The compiler should emit warnings about this use that
the developer must override.

Are you sure we are disagreeing?

-- E

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution