Resolved: Insert "!" is a bad fixit

[CharlesS (Charles Srstka)]

+Int.max

If you go through the Swift tab on Stack Overflow and take a drink every time you see a blatantly inappropriate use of ! in a questioner's code due to them just mashing the fix-it, you will soon be dead of alcohol poisoning.

[torquato]

I'm not following SO that closely, but often !s are fine in example code to keep the code example short and to keep focus on the issue in question. The problem is rather that newbies to the language think that these ! are the correct way to do things in real live and copy such code without reasoning. But then, a fix-it is already too late… o_O

[CharlesS (Charles Srstka)]

That's why I added the "blatantly inappropriate" qualifier; "shortened for simplicity" examples are explicitly not what I'm talking about.

[hooman (Hooman Mehr)]

Lets go back to your original post. The existing fix-it in the screen capture reads:

Value of optional type 'URL?' not unwrapped; did you mean to use '!' or '?'?

I think the first step in correcting the title issue is improving the message itself which I find very confusing for novice developers.

How about rewriting the message to be more helpful for novice developers, for example:

Error message:

'url' has optional type 'URL?' which can be 'nil'. Did you mean to use '??' or '!'?

Fix-it options:

Insert '??' to provide a default value to use when it is 'nil'.
Insert '!' to assert it is impossible to be 'nil'.

This would be pretty easy to do, and has a chance of nudging the novice developer in the right direction.

[Erica_Sadun]

@hooman

How about rewriting the message to be more helpful for novice developers, for example:

Error message:

‘url’ has optional type ‘URL?’ which can be ‘nil’. Did you mean to use ‘??’ or ‘!’?

Fix-it options:

Insert ‘??’ to provide a default value to use when it is ‘nil’.

Insert ‘!’ to assert it is impossible to be ‘nil’.

This would be pretty easy to do, and has a chance of nudging the novice developer in the right direction.

As mentioned upthread, the problem is often that

1. The developer is unaware that the variable is optional
2. The place to unwrap may not be at the point of use but at the point of declaration, using guard-let, if-let, the occasional !, or !!.
3. If the item is created in-place, returning an optional, that the creation/declaration should be separated out to another statement or clause

I think it's too much to ask the compiler to differentiate between items that should have been unwrapped earlier at declaration and in-context items (for example: URL(string: "swift.org")) that return an unexpected optional value.

[Erica_Sadun]

I've updated the !! proposal with an assist from @Paul_Cantrell: https://github.com/erica/swift-evolution/blob/bangbang/proposals/XXXX-unwrap-or-die.md

Comments welcome and appreciated. Thanks

[hooman (Hooman Mehr)]

@Erica_Sadun

I think it’s too much to ask the compiler to differentiate between items that should have been unwrapped earlier at declaration and in-context items (for example: URL(string: "swift.org")) that return an unexpected optional value.

I disagree.

The current diagnostic (called diag::missing_unwrap_optional) does not distinguish between two important cases: Optional in the middle of the chain, and the last (or sole) optional value.

Given these declarations:

struct Company { var url: URL?; func getURL() -> URL? { return url } }
struct Contact { var company: Company? }

var contact: Contact?
var company = Company()
var url: URL?

func f(url: URL) {}

The following code creates diag::missing_unwrap_optional three times:

f(url: contact.company.url)

The error message:

Value of ... not unwrapped; did you mean to use '!' or '?'?

only makes sense for the first two chaining operations (as it is implying optional chaining). But at the end of the chain (third step) we need a value to pass (or assign to a non-optional variable, etc.)

If you type the above code in Xcode, it will auto-insert '?' for the first two cases:

f(url: contact?.company?.url)

and we get the message only once with a horrible fix-it of:

Replace 'contact?.company?.url' with '(contact?.company?.url)!'

(The less horrible way to assert would be: f(url: contact!.company!.url!))

But that is not my point here. My point is, the last element of the chain along with the following cases

f(url: company.getURL())
f(url: url)

need a distinct diagnostic; not the same diag::missing_unwrap_optional.

Once we have that, we need novice friendly wording for the new diagnostic (and corresponding 'fix-it's).

The new fix-it should result one of these:

f(url: contact!.company!.url!) // Assert all the way
f(url: contact?.company?.url ?? <#default value#>) // Optional all the way

An experienced developer may mix some asserting with optional chaining, but definitely not the currently offered (contact?.company?.url)!.

I think this does not need a pitch and should be reported as a bug.

After we have the above, I would go after adding static analysis to produce warnings to offer correct placement of assertions or replacement with proper guard let/if let.

[xwu (Xiaodi Wu)]

@hooman

The new fix-it should result one of these:

f(url: contact!.company!.url!) // Assert all the way
f(url: contact?.company?.url ?? <#default value#>) // Optional all the way

An experienced developer may mix some asserting with optional chaining, but definitely not the currently offered (contact?.company?.url)!.

I think this does not need a pitch and should be reported as a bug.

Huh? contact?.company?.url! is superior to asserting at each step in every scenario; this is the entire purpose of optional propagation. No fixit should ever suggest contact!.company!.url!.

[hooman (Hooman Mehr)]

The last assertion implies the others along the chain. How else can we get a guaranteed value at the end?

[hooman (Hooman Mehr)]

@xwu

No fixit should ever suggest contact!.company!.url!.

I know it is horrible, but I find it less horrible than (contact?.company?.url)! which is downright a lie. Making those implied assertions explicit may help the programmer realize maybe that is not a good idea after all.

Also, any current Insert '!' fixit should be revised to read: Insert '!' to assert a 'nil' value is impossible. or something to that effect.

[CharlesS (Charles Srstka)]

@hooman

Error message:

‘url’ has optional type ‘URL?’ which can be ‘nil’. Did you mean to use ‘??’ or ‘!’?

Fix-it options:

Insert ‘??’ to provide a default value to use when it is ‘nil’.

Insert ‘!’ to assert it is impossible to be ‘nil’.

This would be pretty easy to do, and has a chance of nudging the novice developer in the right direction.

I'd go further and make ?? <#default value#> the only fix-it. If a developer knows enough to be able to guarantee that something will never be nil, he/she also likely knows about ! without needing it to be suggested. In the majority of cases where the developer has forgotten to unwrap the optional, ! will not be the correct choice, and furthermore, inserting the IDE placeholder will force the developer to at least think about it.

[xwu (Xiaodi Wu)]

@hooman

I know it is horrible, but I find it less horrible than (contact?.company?.url)! which is downright a lie. Making those implied assertions explicit may help the programmer realize maybe that is not a good idea after all.

Then when, in your opinion, is optional chaining not horrible? Or is it your opinion that Swift should just remove optional chaining entirely?

[xwu (Xiaodi Wu)]

@CharlesS

I’d go further and make ?? <#default value#> the only fix-it. If a developer knows enough to be able to guarantee that something will never be nil, he/she also likely knows about ! without needing it to be suggested. In the majority of cases where the developer has forgotten to unwrap the optional, ! will not be the correct choice, and furthermore, inserting the IDE placeholder will force the developer to at least think about it.

I would hope that inserting ! will also prompt a developer to think about it.

If your goal is for compiler fix-its to protect unthinking developers from themselves, then you’re fighting a losing battle. The point of compiler diagnostics is to help the user do what they really want to do, not to stop the user from doing anything.

[CharlesS (Charles Srstka)]

Quite to the contrary, the primary thought that will be provoked by inserting !, to a developer who doesn't understand optionals, is "Oh, Swift is complaining about some weird thing again, I'll just hit 'fix' and make whatever it is go away." Again, just spend 5 minutes browsing the Swift questions on Stack Overflow and you'll see just how common this attitude is amongst new Swift developers.

The goal isn't to protect unthinking developers from themselves, but it should be to help them learn, especially when it's something this fundamental to the language. Putting a compiler placeholder in there forces them to see what the optional is there for before being able to proceed, whereas inserting ! not only obstructs that goal, but usually isn't the right thing to use anyway. It's just a bad fix-it. IMO of course.

[xwu (Xiaodi Wu)]

@CharlesS

The goal isn’t to protect unthinking developers from themselves, but it should be to help them learn. Putting a compiler placeholder in there forces them to see what the optional is there for before being able to proceed, whereas inserting ! not only obstructs that goal, but usually isn’t what you want anyway. It’s just a bad fix-it. IMO of course.

That is pedagogically suspect, to put it mildly. If a user “doesn’t understand optionals” and would choose to mash buttons in order to use the ! fix-it, what makes you think that ?? will “help them learn” rather than increase their frustration?

[CharlesS (Charles Srstka)]

Because they'll actually have to do something to make it compile, rather than just mashing a button. And in order to figure out what to do to make it compile, they'll need to at least understand the basic concept of optionals.

[xwu (Xiaodi Wu)]

@CharlesS

Because they’ll actually have to do something to make it compile, rather than just mashing a button. And in order to figure out what to do to make it compile, they’ll need to at least understand the basic concept of optionals.

Or, they may choose to do nothing and instead stop coding in Swift. It is true that perfect code is unwritten code, but we shouldn’t be aiming for that goal.

[CharlesS (Charles Srstka)]

This assertion comes across to me as perfectly bizarre. By this logic, we should also eliminate all compiler errors, and use heuristics to try to guess what the user wants in all circumstances.

I don't think it's a bad thing to require learning the bare essentials in order to produce compiling code.

[hooman (Hooman Mehr)]

I am not against optional chaining at all. Optional chaining is a very powerful and useful feature. It is extremely powerful and useful when:

• The last member of the chain is a Void returning method and we only want to call it if the target of the method is not nil.
• The result of the chain has a reasonable default and we don't care which link of the chain is nil. The chain either gives us a value or we use the default.

What I am saying is: We should help a novice developer clearly see the ramifications of asserting !.

[xwu (Xiaodi Wu)]

@hooman

The last member of the chain is a Void returning method and we only want to call it if the target of the method is not nil.
The result of the chain has a reasonable default and we don’t care which link of the chain is nil. The chain either gives us a value or we use the default.

Interestingly, I’m of the opinion that those are two of the more misleading uses of optional chaining. Personally, I wouldn’t mind if the compiler warned or errored in both of those scenarios.