Resolved: Insert "!" is a bad fixit


(Hooman Mehr) #86

How about starting a new thread for this?


(Xiaodi Wu) #87

I see it as useful to propagate optional intermediate results until the point they need to be unwrapped. It seems like this is precisely the use that you feel should be disallowed.


(Hooman Mehr) #88

Oh, of course. The obvious and preferred handling of an optional chain (along with any other optional) is checked unwrapping with if let/guard let.

I was focusing on valid cases where a simple fix-it of ‘?’ or ‘??’ would be enough to get a valid and reasonable result without force unwrapping or checked unwrapping (the preferred normal way to handle the optionals)

Back to the thread subject. Everybody agrees that a lone Insert '!' is not a good fix-it for novice developers. I don’t think we should remove the fix-it altogether but improve it in multiple ways:

  1. Provide a distinct diagnostic for the last part of a chain, instead of using the same diagnostic that is used in the middle of the chain.
  2. Provide a novice friendly message for the diagnostic.
  3. Provide ‘?? <#default value#>’ as the first suggested fix-it.
  4. Expand ‘!’ fix-it message to remind what ‘!’ asserts.
  5. (Much Later) Add static analysis to backtrack ‘!’ assertions to their originating operation and warn the user to move the assertion there or to perform a checked unwrap at that location to reduce unintended propagation of optional types.

(Nick Keets) #89

I couldn’t agree more. For some reason, this patronizing behavior of trying to save clueless programmers that have no idea what they are doing, keeps coming up.

And beyond that, there are absolutely cases where inserting “!” is a very appropriate fix it. For example when trying out stuff in Playgrounds and more importantly when using Swift as a scripting language.


(Erica Sadun) #90

Given how much of this thread appears within the proposal, I believe this is the right place for further discussion.


(Erica Sadun) #91

Please read the updated proposal and let me know if any parts are unduly patronizing. I will try to amend. I try to make clear the differentiation between language learners, those with experience (both in Swift and other Languages), and those who can do exactly as they need because they understand the consequence.

You bring up a larger, more important, point as well: not all Swift development is production code. Scripting, playgrounds, learning, and utility code with shorter life and more flexible safety requirements properly fall under the language umbrella.


(Félix Fischer) #92

I’ve been there myself. That fixit is a trap card waiting for novice users.

When that fixit was sprung on me, I only introduced crashes, and I didn’t know why until much, much later.


(Félix Fischer) #93

I’m sorry, but I don’t know what proposal are you guys discussing. Is it the “unwrap or die” operator’s proposal?


(Erica Sadun) #94

Here’s a direct link (you can see it also by going through the pull request, clicking Files Changed, clicking View: https://github.com/erica/swift-evolution/blob/bfdf9e245f862de6302a0d9810ba5c1d322df821/proposals/XXXX-unwrap-or-die.md


(Félix Fischer) #95

I understand now. I like the proposal! +1

Edit: and I think this criticism sums up the fixit issue very well:

Fixits should not be used as bandaids to make things compile


(Greg Titus) #96

Ok! I am going to argue the complete opposite of the thread title and say that RIGHT NOW, the main problem is that “!” is not offered as a fixit ENOUGH and that optional chaining with “?” is offered too often, and that this is the cause of the poor Apple dev forum / Stack Overflow example code.

This sort of horror, for instance cellDescriptors[(parentCellIndexPath?.section)!], as discussed already, is the result of optimistically offering optional chaining the first time, and only during the second compile when it is already there does the compiler realize that it still needs a force unwrap and offers ( )!. This code would be a big step better if it was just cellDescriptors[parentCellIndexPath!.section]. It is much more obvious what the actual optional thing is that needs to be handled, instead of sticking the ! unreadably and un-understandably far away.

So I did that. https://github.com/apple/swift/pull/15321

This should improve the fixit choices and make things compile correctly after the first round, and is, I think, an improvement over the status quo. It is also a necessary first step before trying to make smarter fixits at the point of the declaration rather than at each use, because it makes clear (to the programmer and to the compiler) the declaration that is being force unwrapped each time.


#97

I’m not a fan of the “unwrap or die” operator in isolation, but it doesn’t offend me to the extent of trying to prevent it entering the language. However, I find the motivation in the proposal troubling, and would like to explore the logic of that a bit.

The point about “die” is that we only want to do that in the case of what we call “programmer error”. Let’s start looking at cases:


(#1) We unwrap an optional that “cannot” be nil.

For example, the unwrap may be in a line immediately following a test for nil that leaves the scope if the optional’s value is nil. Since the error “can never happen”, there is no point in using !! instead of !.


(#2) But of course, programmers can make errors…

…so there are cases where we would actually “die” if there were a programmer error. The proposed solution seeks to “improve” this by providing an error message that “explains” what went wrong. That’s better than just crashing, isn’t it?

Well, no. If you can explain what went wrong, that means you know what the error was. If you know what the error was, the correct procedure is to go correct the error, not to code an error message and leave the error fatally lurking in the code.

An example of this scenario might be when you have a URL string that you want to turn into a URL object, and this string has already been validated as correctly-formed. If you can’t form a URL from the string, then something has gone horribly and inexplicably wrong somewhere.

But there’s really no point in using “unwrap or die” with a message like “A valid URL string failed to initialize a URL”, because you can generally craft a solution to the problem that doesn’t involve dying. For example, if URL initialization fails, it may be proper to treat the valid URL string as an invalid string, which is a class of non-fatal errors that already has an existing escape hatch (say via throw). Or you can invent a new escape hatch.


(#3) Actually, I went a bit too far in that previous paragraph. An escape hatch may not be feasible in practice.

Now you have an optional that’s nil, and it shouldn’t be, and you have no idea why, and you have no practical recovery mechanism. In this case, “die” is actually the only solution, but now the “unwrap or die” operator has a problem with the error message.

The message can’t explain anything (because you don’t know the explanation — if you did, this problem would eliminable under #2 above). What you’re left with is to use what is basically an arbitrary, unique string. All it’s good for is to locate the source line if the string ever shows up in a console log just before a crash.


That’s the trouble I have with the motivation for the “unwrap or die” operator. It purports to explain an error, but it’s only useful in a situation where no real explanation is available.

I have, of course, overstated my case in order to keep this post from being longer than it is. Things are never that simple, and the “unwrap or die” operator might be something we want in the Swift language anyway. But as a mitigation of the “bad” fixit problem, I don’t think it’s a strongly appealing candidate.

(Also, apologies for all the “air quotes”. Stylistically bad, I know.)


(Geordie J) #98

I strongly agree that inserting “!” Is a bad fixit. This was the most confusing thing about learning Swift for me when it was released.

At the same time, I think it’s a shame that adding yet another operator is the focus of the discussion here. Reading the evolution proposal I was struck by a couple of things that I found very confusing:

let something = array.last !! “Reason why Array isn’t empty”

And

let cast = xyz as? AnotherType !! “Here’s some text”

I find both cases confusing, especially from the perspective of a beginner. Where is the information as to what is optional in the array example? (What’d be worse is if this was not a standard library type where most experienced devs are aware what is optional). And where is the information as to why the second example should crash given the normally safe “?” in the second?

While I think this may be a “nifty” addition for people who know what’s going on here, in certain situations (not the ones above though!), this is just more noise and confusion amongst the sea of !s and ?s that seemingly motivated this proposal in the first place.

There have been some excellent suggestions above regarding improving the compiler diagnostics. One that seemed particularly helpful outlined:

  1. The name of the variable that has the optional type of Type?. If it’s not a variable, spelling out which function call or initialiser returns an optional result.
  2. Mentioning explicitly that its value can be nil and therefore needs to be unwrapped (possibly when it was created)
  3. Suggesting using if let, guard let, OR, asserting the value can never be nil by inserting ! either here or (in the case of a variable) when it was created.
  4. Alternatively adding ?? <> to provide a fallback value.

This is a very worthy topic but I really don’t think adding even more surface to the language does anything to help new users understand what’s happening here. Also as an experienced developer I can see situations (just two mentioned above) where !! significantly reduces code readability.


SE-0217 - The "Unwrap or Die" operator
(Alexey Kravchenko) #99

+1. I think that the proposal very well motivated and balanced. I like some sort of “Design by Contract” inspired solutions in Swift (guards, preconditions, etc.) and !! will improve this direction.


(Alexey Kravchenko) #100

I also like that !! covers as? and try? cases.


(Swift Studies) #101

I agree that in cocoa nil is a mild failure but in my pure Swift libraries and many that I see outside of the legacy platforms #1 is far more prevalent.


(Greg Titus) #102

I just wanted to follow up on this thread. Due to the discussion and suggestions here (thank you all!) and some work over the last month or so by @Douglas_Gregor and I and related general type check fix cleanup by @xedin and @rudkx, a lot of these issues are improved. The force unwrap fixit still exists, but it is now never the only or preferred fixit offered, and hopefully the explanations of the errors are a lot more beginner-friendly now.

More details in:




We should strengthen the thread necromancy warning
#103

This looks like a huge improvement! Many thanks to @Douglas_Gregor and @gregtitus!


(Goffredo Marocchi) #104

Great to see the better strategy in fix-it’s, but almost even better to see improved error messages. This is something super important :)!


(Jon Hull) #105

Great to see these improvements. This was one of the main confusion points when I was teaching Swift to beginners...