SE-0047


(Michael M. Mayer) #1

https://github.com/apple/swift-evolution/blob/master/proposals/0047-nonvoid-warn.md

I really like the way Swift currently works in this regard. Return values can be and are routinely ignored and that is ok and what most people are used to. Except when it isn’t okay, and that’s when we add a warning/annotation. Perfect!

It is far more common to be able to ignore a return value than to have it be required to be handled. To flip when we add the annotation, will require far more effort and will clutter our code. Why do I want annotations to tell me that I can ignore something. I want it there when I need to be warned. You wave a red flag when there is something to be concerned about, not when the results are inconsequential.

I strongly urge for the disapproval of this proposal.

Regards, Michael

···

==================
Michael M. Mayer
Hanover, MD
m.mayer6@gmail.com


(Brent Royal-Gordon) #2

It is far more common to be able to ignore a return value than to have it be required to be handled.

Do you really think that's true?

I took a look at the Swift standard library and extracted just the function, method, and operator names and return values, with Void functions excluded (since they're automatically discardable), even if they throw. Here's the data I got:

  https://gist.github.com/brentdax/11d78a3d8e1df70f21f3

It looks a little funky, but you can pretty much tell what each method does.

Then I asked TextMate to shuffle the lines in that file and took a look at the first twenty:

      multiplyWithOverflow -> (Self, overflow: Bool)
  - -> Float80
  == -> Bool
      withUnsafeMutableBufferPointer -> R
      withUnsafeMutableBufferPointer -> R
      samePositionIn -> String.UTF8View.Index
      withUnsafeMutableBufferPointer -> R
      member -> AnyObject?
      indexForKey -> DictionaryIndex?
  >> -> UInt64
  prefix ~ -> UInt
  != -> Bool
  >= -> Bool
      popLast -> Self.Generator.Element?
      addWithOverflow -> (Self, overflow: Bool)
      successor -> AnyBidirectionalIndex
  - -> Int
      subtractWithOverflow -> (Self, overflow: Bool)
  postfix -- -> Double
  prefix ++ -> UInt32

Of these lines, the three `withUnsafeMutableBufferPointer` methods would sometimes be used with discardable results—except those are examples of a pattern where the return type comes from an inner closure, so if that return value is Void, it's automatically discardable. `popLast`, `--`, and `++` *are* genuinely things that should be discardable; you might use them just for their side effects. For the others, really the only thing the function does is calculate a return value; if you don't use that value, you've wasted CPU cycles for no reason.

  Generic: 3
  Discardable: 3
  Important: 14

Okay, but a lot of those are operators. Maybe that's just an operator thing and functions are a different story? Let's filter out the operators and again select the top twenty:

      multiplyWithOverflow -> (Self, overflow: Bool)
      withUnsafeMutableBufferPointer -> R
      withUnsafeMutableBufferPointer -> R
      samePositionIn -> String.UTF8View.Index
      withUnsafeMutableBufferPointer -> R
      member -> AnyObject?
      indexForKey -> DictionaryIndex?
      popLast -> Self.Generator.Element?
      addWithOverflow -> (Self, overflow: Bool)
      successor -> AnyBidirectionalIndex
      subtractWithOverflow -> (Self, overflow: Bool)
      removeAtIndex -> Self.Generator.Element
  max -> T
      samePositionIn -> UnicodeScalarIndex?
      withUnsafeMutablePointerToValue -> R
      predecessor -> UInt
      generate -> Self.Generator
      advancedBy -> Self
  withVaList -> R
      next -> Element?

We've got two more of those "generic return type" cases—`withUnsafeMutablePointerToValue` and `withVaList`. But in this sample we only have `popLast` and `removeAtIndex in the "should be discardable" category.

  Generic: 5
  Discardable: 2
  Important: 13

Maybe it's just this sample, though, so I ran a few more (on the "no operator" set):

  Generic Discardable Important
  2 0 18
  0 1 19
  1 2 17
  2 0 18
  1 1 18
  ----- ----- -----
  6 4 90

Yeah, it was that sample. That sample had far fewer important return values than average.

I think it's fair to say that the vast majority of standard library functions ought to return values. But maybe the standard library is a little weird, and other frameworks would have different behavior?

Fine. I'll do it again with Core Data. (For reference, Core Data's generated interface is about half the size of Stdlib's in terms of number of lines.)

  https://gist.github.com/brentdax/87e3f86adaf1ed7b11c8

Core Data had 68 non-Void functions to evaluate, which was short enough that I decided I could just look at all of the methods. I wanted to be pretty broad here, but not overbroad, so I'm counting anything with a side effect *other* than merely throwing an error or precalculating some data. That includes some really stupid things, like calling `insertNewObjectForEntityForName` and then not doing anything with it, but it doesn't count calling `executeFetchRequest` and ignoring the result.

So, how did Core Data do?

  Discardable: 9
  Important: 59

That includes, by the way, some *stupendously* bad ideas, such as calling `tryLock` without seeing if the lock succeeded. I would guess that more than half of the "discardable" category are the sort of thing that would leave you gaping at the screen if you saw them in a piece of code.

Now, I've only looked at frameworks here, so maybe application code is different. Maybe *your* code is different for some reason. But I suspect that, if you really take a look at your code, you'll realize that most functions and methods are either Void (and possibly throwing), or they exist entirely to calculate and return some piece of data, and if you're not doing something with that data you shouldn't be calling them. But I think the evidence suggests that warn-by-default is the right choice for most Swift code.

···

--
Brent Royal-Gordon
Architechies


(Tino) #3

I don't think that counting discardable results in existing Cocoas libraries is a good metric for the usefulness of warn-unused-result: Other libraries may follow a different style, and Objective-C is a very bad fit for fluent interfaces.
So far, I haven't seen any real world examples of problems caused by accidentally discarded results, and changing the default would be be a real pain that discourages established patterns.
The imho easiest solution would be a module-specific setting for the default.

That includes, by the way, some *stupendously* bad ideas, such as calling `tryLock` without seeing if the lock succeeded. I would guess that more than half of the "discardable" category are the sort of thing that would leave you gaping at the screen if you saw them in a piece of code.

I see this as a strong argument to only slightly modify the current behavior:
- Add a @pure-annotation which implies warn-unused-result
- Change the name/syntax of warn-unused-result to something shorter and less ugly (already discussed, but no proposals yet)

When nearly all functions trigger warnings, it becomes harder to spot the cases where it is really likely that someone made an error by ignoring a result.

Tino